Skip to content

Conversation

chipspeak
Copy link
Contributor

@chipspeak chipspeak commented Sep 1, 2025

Issue link

Jira Link

What changes have been made

Updated CodeFlare SDK runtime image defaults to include the new Python 3.12 image. This has also been updated in the demo notebooks where referenced.

Additionally, I've moved our supported versions logic to constants.py as I feel this is a more intuitively central location for this. Similarly, a new utils.py file extracts the correct runtime image version to pull in place of the less central existing logic.

Verification steps

  • Checkout this branch.
  • Build the whl file by running poetry build.
  • Open any demo notebook and set the kernel to a Python 3.12 version.
  • Install this version of the SDK by running pip install <path_to_your_whl> --force-reinstall
  • Restart the kernel.
  • Create a Ray Cluster.
  • Check the head pod spec for the image tag or log in to its terminal and run python3 --version to verify that it is using Python 3.12.9.
  • Repeat the above with a kernel set to Python 3.11 to verify that the old image is also used as needed.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@chipspeak chipspeak requested a review from kryanbeane September 1, 2025 12:20
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2025
Copy link

codecov bot commented Sep 1, 2025

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.47%. Comparing base (573a431) to head (3847778).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/codeflare_sdk/common/utils/utils.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   92.47%   92.47%   -0.01%     
==========================================
  Files          24       25       +1     
  Lines        1382     1395      +13     
==========================================
+ Hits         1278     1290      +12     
- Misses        104      105       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chipspeak
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2025
@chipspeak
Copy link
Contributor Author

E2E has failed due to an assertion in the local_interactive_sdk_kind_test. The move to Python 3.12 has introduced a slight computational deviation which causes the assertion to fail. See the difference below.

assert 1789.4644387076728 == 1789.4644387076714

Given just how small the difference is, allowing a more approximate value might be a better solution than a hard-coded one.

@chipspeak
Copy link
Contributor Author

I've gotten the same error multiple times with the same 0.0000000000008% difference in the assertion. I've updated the value to match the Python 3.12 difference just to see if the tests pass etc. I don't think there'd be any harm in keeping this new value as we won't need to test Python 3.11 in this test going forward.

@chipspeak chipspeak marked this pull request as ready for review September 2, 2025 08:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2025
Copy link
Contributor

@kryanbeane kryanbeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great improvements to the runtime image selection thanks Pat, lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 2, 2025
Copy link
Contributor

openshift-ci bot commented Sep 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kryanbeane, pawelpaszki

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [kryanbeane,pawelpaszki]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2025
@chipspeak
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2025
@kryanbeane
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 2bb2376 into project-codeflare:main Sep 9, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants