-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[ci] Add RayImagePushContext for publishing wanda Ray images #60197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Adds new script `push_ray_image.py` for publishing Wanda-cached Ray images to Docker Hub. Focus on this is replicating the tagging logic from ci/ray_ci/docker_container.py. Many test cases were added to try to replicate the existing publishing cases, but it'll be good to hear if any others would be helpful. Topic: push-script Signed-off-by: andrew <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new script push_ray_image.py and its corresponding tests test_push_ray_image.py for publishing Wanda-cached Ray images to Docker Hub. The new RayImagePushContext class encapsulates the logic for generating image tags based on various parameters like branch, commit, Ray type, Python version, platform, and architecture. The accompanying tests are comprehensive and cover a wide range of scenarios, which is excellent for ensuring the correctness of the complex tagging logic. The Bazel build rules have also been updated to include the new binary and test.
One area for improvement is the use of assert statements for input validation in assert_published_image_type. While useful for development, assert statements are removed in optimized Python builds, which could lead to unexpected behavior if these validations are critical for the script's operation in a production environment.
| assert self.python_version in PYTHON_VERSIONS_RAY_ML, invalid_python_version | ||
| assert self.platform in PLATFORMS_RAY_ML, invalid_platform | ||
| assert self.architecture in ARCHITECTURES_RAY_ML, invalid_architecture | ||
| elif self.ray_type in [RayType.RAY_LLM, RayType.RAY_LLM_EXTRA]: | ||
| assert ( | ||
| self.python_version in PYTHON_VERSIONS_RAY_LLM | ||
| ), invalid_python_version | ||
| assert self.platform in PLATFORMS_RAY_LLM, invalid_platform | ||
| assert self.architecture in ARCHITECTURES_RAY_LLM, invalid_architecture | ||
| else: | ||
| # ray or ray-extra | ||
| assert self.python_version in PYTHON_VERSIONS_RAY, invalid_python_version | ||
| assert self.platform in PLATFORMS_RAY, invalid_platform | ||
| assert self.architecture in ARCHITECTURES_RAY, invalid_architecture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using assert statements for input validation that relies on external parameters (like command-line arguments or environment variables) is not robust for production code. Python's assert statements are typically compiled out when running with the -O (optimize) flag, meaning these critical checks would be skipped. It's better to explicitly raise PushRayImageError for invalid inputs to ensure consistent behavior across all environments.
if self.ray_type in [RayType.RAY_ML, RayType.RAY_ML_EXTRA]:
if self.python_version not in PYTHON_VERSIONS_RAY_ML:
raise PushRayImageError(invalid_python_version)
if self.platform not in PLATFORMS_RAY_ML:
raise PushRayImageError(invalid_platform)
if self.architecture not in ARCHITECTURES_RAY_ML:
raise PushRayImageError(invalid_architecture)
elif self.ray_type in [RayType.RAY_LLM, RayType.RAY_LLM_EXTRA]:
if self.python_version not in PYTHON_VERSIONS_RAY_LLM:
raise PushRayImageError(invalid_python_version)
if self.platform not in PLATFORMS_RAY_LLM:
raise PushRayImageError(invalid_platform)
if self.architecture not in ARCHITECTURES_RAY_LLM:
raise PushRayImageError(invalid_architecture)
else:
# ray or ray-extra
if self.python_version not in PYTHON_VERSIONS_RAY:
raise PushRayImageError(invalid_python_version)
if self.platform not in PLATFORMS_RAY:
raise PushRayImageError(invalid_platform)
if self.architecture not in ARCHITECTURES_RAY:
raise PushRayImageError(invalid_architecture)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
Huh TIL. I don't believe we're running in optimized mode today, but I'll use this anyways to start building that practice
a0e2570 to
06d411f
Compare
| ctx = make_ctx(python_version="3.12", commit="abc123def456") | ||
| tags = ctx.destination_tags() | ||
| assert "abc123-py312-cpu" in tags | ||
| assert "build123-py312-cpu" in tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test does not actually test feature branch behavior
Low Severity
The test_feature_branch_non_pr test claims to test "non-PR feature branch" behavior per its name and docstring, but it uses make_ctx(python_version="3.12", commit="abc123def456") without overriding the branch parameter. The default branch in make_ctx is "master", so this test actually exercises master branch behavior instead of feature branch behavior. The test passes coincidentally because both code paths return [sha_tag, rayci_build_id], but feature branch handling is not actually being validated.
Adds new script
push_ray_image.pyfor publishing Wanda-cached Ray images to Docker Hub.Focus on this is replicating the tagging logic from ci/ray_ci/docker_container.py. Many test
cases were added to try to replicate the existing publishing cases, but it'll be good to hear
if any others would be helpful.
Topic: push-script
Signed-off-by: andrew [email protected]