[ci][docker] Automatically update docker images#11673
Closed
driazati wants to merge 1 commit into
Closed
Conversation
Member
Author
|
@tvm-bot rerun |
This activates the code from #11329 behind a couple Jenkins flags: * `USE_AUTOUPDATED_DOCKER_IMAGES` - whether or not to use the dynamically determined docker images in the build * `PUSH_DOCKER_IMAGES` - whether or not to push built images to `tlcpack` on `main` These are there so we can quickly toggle this behavior off it breaks anything. Once this is on though, any docker changes in a PR should be reflected entirely in that build, so there would be no need for a separate PR to update the Docker images. We will have to see if this works in practice, as the docker image builds do download quite a bit of data (which can flake) and add some runtime overhead (about 30m), so when an update lands all PRs will end up needing to rebuild until the merged commit finishes.
areusch
reviewed
Jun 16, 2022
areusch
left a comment
areusch
left a comment
Contributor
There was a problem hiding this comment.
we discussed this today a bit in high-bandwidth and:
- it's possible to write Dockerfiles that build idempotent images, but it's pretty hard. my concern here is that they won't always rebuild correctly and we'll get hard-to-trace CI bugs
- @driazati proposed an alternate solution where, when changes have been made in tvm to
docker/since the last nightly-rebuilt image, the nightly CI validation pipeline opens a PR to update the images with newly-built ones. the PR CI test then can serve to validate them. - as part of this change, we would push the tested images to tlcpackstaging and modify Jenkinsfile such that, when it doesn't find a tlcpack/ image, it also tries to pull the image from tlcpackstaging/. on
main, when it has done this and the build succeeds, it retags the image to tlcpack/. this allows anyone to submit an image to tlcpack without requiring a committer to manually retag.
| // Pull image names from the results of should_rebuild_docker.py | ||
| if (env.USE_AUTOUPDATED_DOCKER_IMAGES == 'yes') { | ||
| ci_arm = sh( | ||
| script: "cat .docker-image.names/ci_arm", |
Contributor
There was a problem hiding this comment.
need to be consistent with .gitignore (docker-image.names vs docker-image-names)
This was referenced Jun 17, 2022
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This activates the code from #11329 behind a couple Jenkins flags:
USE_AUTOUPDATED_DOCKER_IMAGES- whether or not to use the dynamically determined docker images in the buildPUSH_DOCKER_IMAGES- whether or not to push built images totlcpackonmainThese are there so we can quickly toggle this behavior off it breaks anything. Once this is on though, any docker changes in a PR should be reflected entirely in that build, so there would be no need for a separate PR to update the Docker images.
We will have to see if this works in practice, as the docker image builds do download quite a bit of data (which can flake) and add some runtime overhead (about 30m), so when an update lands all PRs will end up needing to rebuild until the merged commit finishes.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.
cc @Mousius @areusch