Repository navigation
Conversation
|
@szaher: This pull request references RHOAIENG-97474 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1169 +/- ##
==========================================
- Coverage 96.50% 96.46% -0.05%
==========================================
Files 24 24
Lines 2319 2346 +27
==========================================
+ Hits 2238 2263 +25
- Misses 81 83 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
No Ray 2.58.0 image exists for Python 3.11. Remove CUDA_PY311_RUNTIME_IMAGE and the "3.11" entry from SUPPORTED_PYTHON_VERSIONS. Python 3.11 users must provide their own image (existing warning guides them). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pawelpaszki
left a comment
There was a problem hiding this comment.
Summary
I agree with removing the Python 3.11 default image from SUPPORTED_PYTHON_VERSIONS (no Ray 2.58.0 py311 image on quay.io/modh/ray, and the old pin was 2.52.1 vs RAY_VERSION). That part should land.
However, this PR mixes several concerns and leaves important follow-up undone. I’m requesting changes before merge.
1. Unrelated work bundled under RHOAIENG-97474
Most of the diff (~75%) is cluster CPU/memory request vs limit validation (_parse_cpu, _parse_memory, _validate_resource_requests in config.py + ~116 lines in test_config.py, tied to RHOAIENG-59338). That is a separate feature (behavior change, release notes, Jira target version).
Please split into a dedicated PR (or at minimum a separate commit + Jira reference in title/description). Reviewers and release tooling shouldn’t have to untangle image defaults from cluster validation.
2. Incomplete cleanup after dropping py311
Constants/tests were updated, but the repo still advertises py311 defaults elsewhere. None of these are touched in this PR:
.github/workflows/update-versions.yaml— still bumpsCUDA_PY311_RUNTIME_IMAGEdocs/sphinx/user-docs/cluster-configuration.rst— still documents py311 default image- Guided demo / additional notebooks — still list
quay.io/modh/ray:2.52.1-py311-cu121
Users and automation will believe 3.11 still has a supported default. Please include cleanup in this PR or an immediate follow-up tracked on the same ticket.
3. Fallback behavior for 3.11 is inconsistent and under-tested
get_ray_image_for_python_version() in utils.py (unchanged in this PR) still does:
warn_on_unsupported=True(used byupdate_image()): warn +Nonefor 3.11 — correct after this change.warn_on_unsupported=False: silent fallback toCUDA_PY312_RUNTIME_IMAGEfor any unsupported version, including explicit"3.11".
That means callers using warn_on_unsupported=False get a 3.12 Ray image while requesting 3.11 — a Python/Ray skew risk. The new test test_get_ray_image_for_python_version_explicit_311_unsupported actually asserts the py312 image, not None, but the docstring says “returns None” (see inline comment).
If the product intent is “only py312 supported,” consider whether silent fallback should still apply to 3.11 (or document clearly that 3.11 must always pass an explicit image). At minimum, fix the misleading test name/docstring and align tests with intended behavior.
4. Release / process
- openshift-ci noted RHOAIENG-97474 has no target version (expected 5.1.0 for
main) — please set before merge. - Breaking change: SDK on Python 3.11 without
imageno longer gets a default modh image; call out in release notes.
5. Validation change (if kept in this PR)
ClusterConfiguration(..., head_cpu_requests=4) with defaults now raises — good for 59338, but it’s a breaking config API change and should not ride only on the py311 image ticket.
Codecov also reports 2 uncovered lines in config.py on this patch.
Happy to re-review once scope is split and cleanup/fallback behavior is addressed.
| ) | ||
|
|
||
|
|
||
| _MEMORY_BINARY_UNITS = { |
There was a problem hiding this comment.
Unrelated to RHOAIENG-97474. This entire block (_MEMORY_*, _parse_cpu, _parse_memory, _validate_resource_requests) is cluster resource validation for RHOAIENG-59338. It dominates the PR (+62 LOC production, +116 tests) and changes ClusterConfiguration behavior (e.g. head_cpu_requests=4 with default limits now fails at init).
Please move to a separate PR with its own Jira + release note. Keeping it here makes review, rollback, and target-version tracking much harder.
| assert worker_group["maxReplicas"] == 3 | ||
|
|
||
|
|
||
| def test_resource_requests_exceed_limits_head_cpu(): |
There was a problem hiding this comment.
These tests belong with the config.py validation change (59338), not the py311 default-image removal. Please split with the production code above.
| image = get_ray_image_for_python_version("3.11") | ||
| assert image == CUDA_PY311_RUNTIME_IMAGE | ||
| def test_get_ray_image_for_python_version_explicit_311_unsupported(): | ||
| """Test get_ray_image_for_python_version() returns None for Python 3.11.""" |
There was a problem hiding this comment.
Incorrect / misleading test documentation. The docstring says this returns None for Python 3.11, but the test calls get_ray_image_for_python_version("3.11", warn_on_unsupported=False) and asserts CUDA_PY312_RUNTIME_IMAGE.
That documents the silent fallback to 3.12 path, not the unsupported path users hit via update_image() (warn_on_unsupported=True → warn + None).
Please fix the docstring (and consider renaming the test) to match behavior, or change behavior if product intent is that 3.11 should never silently get a 3.12 image.
| CUDA_PY312_RUNTIME_IMAGE = "quay.io/modh/ray@sha256:8183007c9b46b359bdc75ea74346f10d642e585bb4f22fa3aa02524b3c966fa0" | ||
|
|
||
| # Centralized image selection | ||
| SUPPORTED_PYTHON_VERSIONS = { |
There was a problem hiding this comment.
Dropping "3.11" here is the right fix, but cleanup is incomplete in this PR:
.github/workflows/update-versions.yamlstill maintainsCUDA_PY311_RUNTIME_IMAGE- Sphinx docs + multiple notebooks still document the old py311 default (
2.52.1-py311-cu121)
Please update or remove those references so docs/CI match runtime behavior.
| """Test get_ray_image_for_python_version() with explicit Python 3.11.""" | ||
| image = get_ray_image_for_python_version("3.11") | ||
| assert image == CUDA_PY311_RUNTIME_IMAGE | ||
| def test_get_ray_image_for_python_version_explicit_311_unsupported(): |
There was a problem hiding this comment.
Related to fallback semantics: after removing 3.11 from SUPPORTED_PYTHON_VERSIONS, explicit "3.11" with warn_on_unsupported=False still returns the 3.12 image (utils.py unchanged). If we mean “only py312 supported,” we should either stop falling back for 3.11 or add a test that matches the primary path (update_image / warn_on_unsupported=True → None), which you already cover in test_update_image_with_*_python_311_unsupported — this test should not contradict that story via a wrong docstring.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
CUDA_PY311_RUNTIME_IMAGEand"3.11"fromSUPPORTED_PYTHON_VERSIONSTest plan
🤖 Generated with Claude Code