Enable specifying whether or not to pull the docker image from ECR #6836
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
facebook-github-bot
added
the
CLA Signed
ZainRizvi
commented
Jun 25, 2025
| # gracefully return the docker image name as it is. Pulling docker image in Linux | ||
| # job could then download the pre-built image as usual | ||
| if [[ ! -d "${DOCKER_BUILD_DIR}" ]] || [[ ! -f "${DOCKER_BUILD_DIR}/${DOCKER_BUILD_SCRIPT}" ]]; then | ||
| if [[ -d "${DOCKER_BUILD_DIR}" ]] && [[ -f "${DOCKER_BUILD_DIR}/${DOCKER_BUILD_SCRIPT}" ]] && [[ "${USE_CUSTOM_DOCKER_REGISTRY}" == "true" ]]; then |
Contributor
Author
There was a problem hiding this comment.
note: Inverted the if statement for better readability
zxiiro
reviewed
Jun 25, 2025
| echo "docker-image=${DOCKER_IMAGE_NAME}" >> "${GITHUB_OUTPUT}" | ||
| echo "There is no Docker build script in ${REPO_NAME} repo, skipping..." | ||
| echo "There is no Docker build script in ${REPO_NAME} repo, or not using custom registry, skipping..." |
Collaborator
There was a problem hiding this comment.
This line was tripping me up. I tried to make it more clear and concise.
Suggested change
| echo "There is no Docker build script in ${REPO_NAME} repo, or not using custom registry, skipping..." | |
| echo "No Docker build script in ${REPO_NAME} repo or custom registry not used, skipping..." |
Contributor
Author
There was a problem hiding this comment.
Good call. I changed the wording even more.
zxiiro
reviewed
Jun 25, 2025
.github/workflows/linux_job_v2.yml
Outdated
| default: "" | ||
| type: string | ||
| use-custom-docker-registry: | ||
| description: "Use the custom ECR registry hosted on AWS. Only takes effect if there's a build.sh script in the docker-build-dir." |
Collaborator
There was a problem hiding this comment.
Suggested change
| description: "Use the custom ECR registry hosted on AWS. Only takes effect if there's a build.sh script in the docker-build-dir." | |
| description: "Use the custom ECR registry. Applies only if build.sh exists in docker-build-dir." |
ZainRizvi
commented
Jun 25, 2025
.github/workflows/linux_job_v2.yml
Outdated
| - name: Calculate docker image | ||
| id: calculate-docker-image | ||
| uses: ./test-infra/.github/actions/calculate-docker-image | ||
| uses: pytorch/test-infra/.github/actions/calculate-docker-image@zainr/nova-gpu |
Contributor
Author
There was a problem hiding this comment.
todo: revert this before merging
seemethere
reviewed
Jun 25, 2025
.github/workflows/linux_job_v2.yml
Outdated
| - name: Calculate docker image | ||
| id: calculate-docker-image | ||
| uses: ./test-infra/.github/actions/calculate-docker-image | ||
| uses: pytorch/test-infra/.github/actions/calculate-docker-image@zainr/nova-gpu |
Contributor
Author
There was a problem hiding this comment.
yup, I left a "todo: revert this" comment on it just a couple mins before you posted this :)
Removed now
git pus
seemethere
approved these changes
Jun 25, 2025
zxiiro
approved these changes
Jun 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We originally tried to simplify the number of parameters users had to pass in to linux_job_v2 by using the existence of the docker build script as a heuristic.
That worked for a while, but that heuristic is now starting to break. If a repo has an unrelated docker build script located in
.ci/docker/build.sh, we assume it wants to pull from our custom ECR path instead of docker hub.Our ideal end state would now look like:
That migration requires many repos to be edited with a fix however, so for now we're having the following non-breaking change:
Have an explicit parameter on linux_job_v2 that needs to be set if a repo wants to use our ECR registries. Set it to TRUE by default, with the calculate-docker-image logic being "If param is set to True AND special docker file exists" then we use the ECR registry. If either is false, use docker hub
No migrations needed in the short term. We can later migrate domain repos to explicitly set this setting and then change the default to FALSE, but that's P2
Testing: Ran this job against it, and it only failed due to an error in the script inputted to the workflow, which is ignorable