[CI] Add format_platform_tag and build_platform_reverse_map to supported_images#62200
[CI] Add format_platform_tag and build_platform_reverse_map to supported_images#62200andrew-anyscale wants to merge 2 commits intoandrew/revup/master/rayci-39from
Conversation
|
Reviews in this chain: |
There was a problem hiding this comment.
Code Review
This pull request refactors platform tag formatting by moving format_platform_tag to a centralized supported_images module and introduces build_platform_reverse_map to map short tags back to full platform strings. Corresponding tests were updated or moved to reflect these changes. Feedback was provided to handle potential ambiguities in the reverse map generation by raising an error if multiple full platform strings resolve to the same short tag, ensuring deterministic behavior.
ci/ray_ci/supported_images.py
Outdated
| if short not in reverse_map: | ||
| reverse_map[short] = platform |
There was a problem hiding this comment.
The current implementation for building the reverse map might lead to silent and non-deterministic behavior if ray-images.json ever contains different full platform strings that shorten to the same tag (e.g., cu12.1.1 and cu12.1.2 would both become cu121). The if short not in reverse_map: logic means the first one encountered wins, and which one is first can be arbitrary as it depends on dict iteration order which is not guaranteed to be stable from json.loads.
To make this more robust, I suggest raising an error if an ambiguous mapping is detected. This will prevent subtle bugs in the future.
| if short not in reverse_map: | |
| reverse_map[short] = platform | |
| if short in reverse_map: | |
| if reverse_map[short] != platform: | |
| raise ValueError( | |
| f"Ambiguous short platform tag '{short}': " | |
| f"could be '{platform}' or '{reverse_map[short]}'" | |
| ) | |
| else: | |
| reverse_map[short] = platform |
…ted_images Move format_platform_tag() from image_tags_lib.py to supported_images.py (zero external deps) and add build_platform_reverse_map() for mapping short platform tags back to full platform strings. Create narrow //ci/ray_ci:supported_images Bazel target visible to //release:__subpackages__. Re-import format_platform_tag in image_tags_lib.py. Update tests for ValueError (was ImageTagsError). Remove duplicate test classes from test_push_release_test_image.py. Topic: shared-lib Relative: rayci-39 Labels: draft Signed-off-by: andrew <andrew@anyscale.com>
…_map If two different full platform strings shorten to the same tag (e.g., cu12.1.1 and cu12.1.2 both → cu121), raise ValueError instead of silently using whichever appeared first. Topic: shared-lib Relative: rayci-39 Labels: draft Signed-off-by: andrew <andrew@anyscale.com>
d5e92cc to
3fced16
Compare
Move format_platform_tag() from image_tags_lib.py to supported_images.py (zero external deps) and add build_platform_reverse_map() for mapping short platform tags back to full platform strings.
Create narrow //ci/ray_ci:supported_images Bazel target visible to //release:subpackages. Re-import format_platform_tag in image_tags_lib.py. Update tests for ValueError (was ImageTagsError). Remove duplicate test classes from test_push_release_test_image.py.
Topic: shared-lib
Relative: rayci-39
Labels: draft
Signed-off-by: andrew andrew@anyscale.com