[ci] Add push_anyscale_image to push wanda anyscale images#60796
[ci] Add push_anyscale_image to push wanda anyscale images#60796andrew-anyscale wants to merge 3 commits intomasterfrom
Conversation
There is significant overlap with the next feature of push_anyscale_image, so this refactor reduces on a lot of duplicated work. Topic: image-tags-lib Signed-off-by: andrew <andrew@anyscale.com>
Add push_anyscale_image.py to copy Wanda-cached anyscale images to ECR, GCP, and Azure registries for release tests. This replaces the Docker-based build approach in AnyscaleDockerContainer with crane-based image copying. - AnyscaleImagePushContext class encapsulating tag generation logic - Uses shared image_tags_lib for tag formatting (python/platform suffixes) - Pushes to all three cloud registries (ECR, GCP, Azure) - Includes diff test verifying tag parity with original implementation Topic: push-anyscale-wanda Relative: image-tags-lib Signed-off-by: andrew <andrew@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a new script push_anyscale_image.py to handle pushing Anyscale images, replacing the old Docker-based approach. The new implementation is well-structured, using a dedicated context class AnyscaleImagePushContext to encapsulate the complex tag generation logic. The inclusion of comprehensive unit tests (test_push_anyscale_image.py) and a diff test (test_push_anyscale_image_diff.py) to ensure parity with the old implementation is excellent and provides high confidence in the correctness of the changes.
I have a few suggestions to simplify and improve the code's readability and maintainability by removing some redundant logic. Overall, this is a solid contribution.
| if self.platform == "cpu": | ||
| return f"{self.image_type}-anyscale-py{self.python_version}-cpu" | ||
| return f"{self.image_type}-anyscale-py{self.python_version}-{self.platform}" |
There was a problem hiding this comment.
The if block here is redundant. The expression in the return statement on line 121 correctly handles the case where self.platform is 'cpu'. You can simplify this method to a single return statement.
return f"{self.image_type}-anyscale-py{self.python_version}-{self.platform}"| is_master = self.branch == "master" | ||
| is_pull_request = self.pull_request != "false" | ||
| is_release = self.branch and self.branch.startswith("releases/") | ||
| sha_tag = self.commit[:6] | ||
|
|
||
| if is_master: | ||
| return [sha_tag, self.rayci_build_id] | ||
| elif is_release: | ||
| release_name = self.branch[len("releases/") :] | ||
| return [f"{release_name}.{sha_tag}", self.rayci_build_id] | ||
| elif is_pull_request: | ||
| return [f"pr-{self.pull_request}.{sha_tag}", self.rayci_build_id] | ||
| else: | ||
| return [sha_tag, self.rayci_build_id] |
There was a problem hiding this comment.
This method can be simplified. The logic for is_master and the final else block is identical, making the is_master check redundant. Additionally, this method can return an empty string for a version if rayci_build_id is not set, which requires filtering later in the main function and in tests.
The logic can be refactored to be more concise and to handle an empty rayci_build_id gracefully within this method. This would make the filtering in main and tests unnecessary.
is_pull_request = self.pull_request != "false"
is_release = self.branch and self.branch.startswith("releases/")
sha_tag = self.commit[:6]
if is_release:
primary_tag = f"{self.branch[len('releases/'):]}.{sha_tag}"
elif is_pull_request:
primary_tag = f"pr-{self.pull_request}.{sha_tag}"
else: # master or other branches
primary_tag = sha_tag
versions = [primary_tag]
if self.rayci_build_id:
versions.append(self.rayci_build_id)
return versions| raise PushAnyscaleImageError(str(e)) | ||
|
|
||
| # Filter out tags with empty rayci_build_id | ||
| tags = [t for t in tags if t and not t.startswith("-")] |
There was a problem hiding this comment.
If the _versions method is refactored as suggested in my other comment, it will no longer produce empty version strings. Therefore, this line of code to filter tags will become unnecessary and can be removed.
| # Filter empty build_id tags like main() does | ||
| tags = [t for t in tags if t and not t.startswith("-")] |
There was a problem hiding this comment.
Following the suggested refactoring in push_anyscale_image.py to handle empty rayci_build_id within the _versions method, this filtering logic in the test is no longer needed.
| # Filter empty build_id tags like main() does | ||
| tags = [t for t in tags if t and not t.startswith("-")] |
There was a problem hiding this comment.
Similar to the other test case, with the suggested refactoring in push_anyscale_image.py, this filtering logic is no longer needed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| for version in self._versions(): | ||
| for py in self._python_suffixes(): | ||
| for plat in self._platform_suffixes(): | ||
| tags.append(f"{version}{self._variation_suffix()}{py}{plat}") |
There was a problem hiding this comment.
Loop nesting order inconsistent with original and sibling implementation
Low Severity
destination_tags() iterates python → platform (inner loops), while both the original DockerContainer._get_image_tags() and sibling RayImagePushContext.destination_tags() in push_ray_image.py iterate platform → python. This produces the same set of tags but in a different order. The diff test masks this by using sorted() comparison. While the canonical tag (first element) is unaffected, this inconsistency between closely related implementations could cause confusion or issues if tag ordering becomes significant.
Additional Locations (1)
| @@ -0,0 +1,267 @@ | |||
| """ | |||
| Push Wanda-cached anyscale images to ECR, GCP, and Azure registries. | |||
There was a problem hiding this comment.
can we rename this to push_release_test_image instead?
| _DOCKER_ECR_REPO = os.environ.get( | ||
| "RAYCI_WORK_REPO", | ||
| "029272617770.dkr.ecr.us-west-2.amazonaws.com/rayproject", | ||
| ) |
There was a problem hiding this comment.
ECR work repo default missing /citemp path segment
Low Severity
The default value for _DOCKER_ECR_REPO is "029272617770.dkr.ecr.us-west-2.amazonaws.com/rayproject", but the canonical default in ci/ray_ci/container.py (which the original AnyscaleDockerContainer imports from) is "029272617770.dkr.ecr.us-west-2.amazonaws.com/rayproject/citemp" — note the missing /citemp path segment. When RAYCI_WORK_REPO is not set, the source Wanda image lookup would reference the wrong ECR repository. Additionally, sibling Wanda scripts (push_ray_image.py, copy_wanda_image.py) make RAYCI_WORK_REPO a required parameter with no fallback default, so this script's behavior diverges from the established pattern.
2 participants
Add push_anyscale_image.py to copy Wanda-cached anyscale images to ECR, GCP, and Azure registries for release tests. This replaces the Docker-based build approach in AnyscaleDockerContainer with crane-based image copying.
Topic: push-anyscale-wanda
Relative: image-tags-lib
Signed-off-by: andrew andrew@anyscale.com