-
Notifications
You must be signed in to change notification settings - Fork 823
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
Fix and add test for build_image.sh: Make it invariant to arguments order
#2226
Merged
agunapal
merged 7 commits into
pytorch:master
from
fabridamicelli:fix_build_image_script
Apr 14, 2023
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0836720
make build_image.sh script pass shellcheck
fabridamicelli 74e5699
fix build_image.sh handling of custorm DOCKER_TAG
fabridamicelli 8d21f73
add test-image-build-tagging in CI
fabridamicelli 1291c0f
Merge branch 'master' into fix_build_image_script
agunapal c0df764
start consolidating docker CI
fabridamicelli 7c6b58f
explain test in docker ci
fabridamicelli 1db211a
Merge branch 'master' into fix_build_image_script
agunapal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -o errexit -o nounset -o pipefail | ||
|
|
||
| # This test checks the parsing and handling of arguments in `build_image.sh`, | ||
| # making sure that `build_image.sh` is invariant to the order of the passed | ||
| # arguments `-py` (python version), `-t` (image tag) and `-g` (use gpu flag) | ||
| # and that tagging works properly. | ||
| # That means, we have 3 args, so there are 6 possibilities to order them and | ||
| # we expect these script runs to produce the *very same output*: | ||
| # | ||
| # $ ./build_image.sh -py "${VERSION}" -t "${TAG}" -g | ||
| # $ ./build_image.sh -py "${VERSION}" -g -t "${TAG}" | ||
| # $ ./build_image.sh -t "${TAG}" -py "${VERSION}" -g | ||
| # $ ./build_image.sh -t "${TAG}" -g -py "${VERSION}" | ||
| # $ ./build_image.sh -g -py "${VERSION}" -t "${TAG}" | ||
| # $ ./build_image.sh -g -t "${TAG}" -py "${VERSION}" | ||
| # | ||
| # In order to assert the equivalence of all these variations, we take advantage | ||
| # of how docker builds images: If two images are exactly the same (ie, they are | ||
| # composed of the very same layers) they will have the same digest (ie, a hash | ||
| # value representing the content of the image), regardless of the tag assigned | ||
| # to the image. So, for example, if we run (with the same Dockerfile): | ||
| # | ||
| # $ docker build -f Dockerfile -t Org/Repo:TagOne . | ||
| # $ docker build -f Dockerfile -t Org/Repo:TagTwo . | ||
| # $ docker images --no-trunc | ||
| # | ||
| # we will see something like this: | ||
| # | ||
| # REPOSITORY TAG IMAGE ID CREATED SIZE | ||
| # Org/Repo TagOne sha256:e3824d794c0ccf10d2f61291f34e0d7e1e02e30b3d459465bc57d04dd3b65884 30 seconds ago 2.14GB | ||
| # Org/Repo TagTwo sha256:e3824d794c0ccf10d2f61291f34e0d7e1e02e30b3d459465bc57d04dd3b65884 30 seconds ago 2.14GB | ||
| # | ||
| # Notice that IMAGEID and CREATED are the same, since the first time it is | ||
| # actually created while the second time it just uses the cached layers. | ||
| # So the tag is "just a label" attached to the underlying image. | ||
| # | ||
| # Putting all together for our test: | ||
| # We run `build_image.sh` (on the same machine to allow docker cache) with each | ||
| # args order variation, tagging each variation with a different name (ensured | ||
| # by the random part of the string). | ||
| # We expect: | ||
| # - All the tags to exist (tagging works): len(images_to_test) == len(tags_to_test) | ||
| # - All tagged images to be actually one and the same under the hood: len(set(digests)) == 1 | ||
|
|
||
|
|
||
| PY_VERSION=$1 | ||
| TAG_1="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
| TAG_2="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
| TAG_3="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
| TAG_4="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
| TAG_5="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
| TAG_6="org/repo:image-${PY_VERSION}-${RANDOM}-${RANDOM}-${RANDOM}-${RANDOM}" | ||
|
|
||
| # Do builds alternating the flags order (-g, -t, -py) | ||
| # (which should build only one underlying image) | ||
| ./build_image.sh -py "${PY_VERSION}" -t "${TAG_1}" -g | ||
| ./build_image.sh -py "${PY_VERSION}" -g -t "${TAG_2}" | ||
|
|
||
| ./build_image.sh -g -py "${PY_VERSION}" -t "${TAG_3}" | ||
| ./build_image.sh -g -t "${TAG_4}" -py "${PY_VERSION}" | ||
|
|
||
| ./build_image.sh -t "${TAG_5}" -py "${PY_VERSION}" -g | ||
| ./build_image.sh -t "${TAG_6}" -g -py "${PY_VERSION}" | ||
|
|
||
| # Collect all the images with their tags and ids | ||
| IMGS_FILE="test_images.json" | ||
| docker images --no-trunc --format "{{json .}}" | jq '{"repo": .Repository, "tag": .Tag, "digest": .ID}' | jq -s > "${IMGS_FILE}" | ||
|
|
||
| python <<EOF | ||
| import json | ||
|
|
||
| tags_to_test = [ | ||
| "${TAG_1}", | ||
| "${TAG_2}", | ||
| "${TAG_3}", | ||
| "${TAG_4}", | ||
| "${TAG_5}", | ||
| "${TAG_6}", | ||
| ] | ||
|
|
||
| with open("${IMGS_FILE}") as file: | ||
| images_to_test = [ | ||
| img | ||
| for img in json.load(file) | ||
| if f'{img["repo"]}:{img["tag"]}' in tags_to_test | ||
| ] | ||
|
|
||
| if len(images_to_test) == 0: | ||
| raise ValueError("No images to test were detected") | ||
|
|
||
| if len(images_to_test) != len(tags_to_test): | ||
| raise ValueError(f"number of images_to_test {len(images_to_test)} does not match number of tags_to_test {len(tags_to_test)}") | ||
|
|
||
| digests = set(img["digest"] for img in images_to_test) | ||
|
|
||
| if len(digests) != 1: | ||
| raise ValueError(f"There should be only 1 digest, found these: {digests}") | ||
|
|
||
| print(f"Test successfull! All flags orders lead to the same image build with digest {digests} \n") | ||
| EOF | ||
|
|
||
| rm -f "${IMGS_FILE}" | ||
| docker system prune -f | ||
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.
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.
Nit: Would it be better to put this python code in a separate file and invoke it here or maybe implement the entire test in python?
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.
Thanks @namannandan . Hi @fabridamicelli this is perhaps another thing to consider for future PRs on adding more tests. It would be great if these can be implemented using pytest. You can take a look at the this directory to see examples of how this is being done for the non-docker examples. We don't have an example with docker.
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.
Thank you for reviewing and for the feedback !
@namannandan
I can understand the point about the combination of python/bash code in one script. My logic behind having everything in one script is to kind of have an atomic component with (1 test -> 1 file) – and the mixture will anyways exist if we call things from python with
subprocess.run. Still we could refactor it as you suggest and have it all in python.@agunapal
Regarding pytest: I take the point and I will check the other existing tests to follow the standards a bit closer, so we can step by step refactor things and iterate a bit on the design of the docker tests such that it is easy to extend with more tests and usage examples. I still have the feeling that it makes sense to keep docker tests under
docker/for docker things to be all in one place..