Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RFC] A proposed update to the Docker images ci_* tag pattern #66

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[RFC] A proposed update to the Docker images ci_* tag pattern #66

wants to merge 3 commits into from

Conversation

leandron
Copy link
Contributor

@leandron leandron commented Apr 8, 2022 •
edited
Loading

This RFC proposes a new format to keep our Docker images used in CI, moving away from our current incremental numbering to a more meaningful format that contains a timestamp and the latest hash from the repository used to generate such images.

cc @areusch @driazati @Mousius @konturn @junrushao1994 @masahi @gromero @mehrdadh @tqchen for visibility and reviews

@leandron
[RFC] A proposed update to the Docker images ci_* tag pattern
e88a2c1 
This RFC proposes a new format to keep our Docker images used in CI,
moving away from our current incremental numbering to a more meaningful
format that contains a timestamp and the latest hash from the repository
used to generate such images.
driazati
driazati approved these changes Apr 8, 2022
View reviewed changes
Copy link
Member

@driazati driazati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for sending this in, looks good and should go along nicely with the work in apache/tvm#10646

areusch
areusch approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @leandron this looks great!

rfcs/0000-docker-image-tags.md Outdated Show resolved Hide resolved
Mousius
Mousius requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a much better and more consistent format @leandron, could we also move to - or _ everywhere when we roll this out? (not tlcpackstaging/ci_cpu and tlcpack/ci-cpu)

Other than that, I spotted a few typos 😿

rfcs/0000-docker-image-tags.md Outdated Show resolved Hide resolved
rfcs/0000-docker-image-tags.md Outdated Show resolved Hide resolved
@areusch
Copy link
Contributor

areusch commented Apr 11, 2022

@Mousius i support - instead of _ (when running docker/bash.sh, you can then use _ as a sort of rule of thumb to determine if you're referencing a variable-driven revision or a specific one)

mehrdadh
mehrdadh approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks for working on this!

gromero
gromero requested changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leandron Just nitpicked what looks like a typo. Please see my comment inline.

Otherwise Rationale and alternatives makes sense to me and overall LGTM!

rfcs/0000-docker-image-tags.md Outdated Show resolved Hide resolved
@leandron @Mousius
Update rfcs/0000-docker-image-tags.md
f6904d6 
Co-authored-by: Christopher Sidebottom <[email protected]>
@leandron
Copy link
Contributor Author

Just friendly note here, that as I'm not hearing opposition, I'll move into creating a a PR to update the documentation and reflect the changes proposed here where appropriate.

@Mousius
Copy link
Member

Mousius commented Apr 20, 2022

@leandron, looking at Docker Hub (https://hub.docker.com/_/hello-world) it would appear the convention for image names is to use - there as well (i.e. tlcpack/ci-cpu rather than tlcpack/ci_cpu) can we go for that one?

@leandron
Copy link
Contributor Author

@leandron, looking at Docker Hub (https://hub.docker.com/_/hello-world) it would appear the convention for image names is to use - there as well (i.e. tlcpack/ci-cpu rather than tlcpack/ci_cpu) can we go for that one?

Sure. I’ll push an updated version with this and @gromero’s suggestion as well.

Mousius added a commit to apache/tvm that referenced this pull request May 5, 2022
@Mousius
[CI] Update all Docker Images to 20220505-060045-500703308
d6b43e1 
This gives us GoogleTest for #11202 and blocklint for #11200 but most importantly it makes use of the new and improved tags from @leandron in apache/tvm-rfcs#66

Closes #11202
Closes #11200
@Mousius Mousius mentioned this pull request May 5, 2022
Merged
Mousius added a commit to apache/tvm that referenced this pull request May 6, 2022
@Mousius
[CI] Update all Docker Images to 20220505-060045-500703308
a4d7b58 
This gives us GoogleTest for #11202 and blocklint for #11200 but most importantly it makes use of the new and improved tags from @leandron in apache/tvm-rfcs#66

Closes #11202
Closes #11200
leandron pushed a commit to apache/tvm that referenced this pull request May 6, 2022
@Mousius
[CI] Update all Docker Images to 20220505-060045-500703308 (#11219)
fb32997 
This gives us GoogleTest for #11202 and blocklint for #11200 but most importantly it makes use of the new and improved tags from @leandron in apache/tvm-rfcs#66

Closes #11202
Closes #11200
@areusch
Copy link
Contributor

areusch commented May 12, 2022

@leandron should we merge this? I think the change has landed in Jenkinsfile now

@areusch
Copy link
Contributor

areusch commented May 12, 2022

cc @Mousius @gromero can you approve?

@gromero
Copy link
Contributor

gromero commented May 13, 2022

cc @Mousius @gromero can you approve?

@leandron Hi. Are you still planing to change #66 (comment) ?

shtinsa pushed a commit to Deelvin/tvm that referenced this pull request May 17, 2022
@Mousius
[CI] Update all Docker Images to 20220505-060045-500703308 (apache#11219
635d0a2 
)

This gives us GoogleTest for apache#11202 and blocklint for apache#11200 but most importantly it makes use of the new and improved tags from @leandron in apache/tvm-rfcs#66

Closes apache#11202
Closes apache#11200
SebastianBoblest pushed a commit to SebastianBoblest/tvm that referenced this pull request May 27, 2022
@Mousius
[CI] Update all Docker Images to 20220505-060045-500703308 (apache#11219
b4edf47 
)

This gives us GoogleTest for apache#11202 and blocklint for apache#11200 but most importantly it makes use of the new and improved tags from @leandron in apache/tvm-rfcs#66

Closes apache#11202
Closes apache#11200
@leandron @Mousius
Update rfcs/0000-docker-image-tags.md
8170fd3 
Co-authored-by: Christopher Sidebottom <[email protected]>
@leandron leandron force-pushed the docker-image-tags-format branch from badd25c to 8170fd3 Compare June 1, 2022 16:27
@gromero
Copy link
Contributor

gromero commented Jun 7, 2022

@Mousius I don't have any further comments on it. Are you ok with the current state of this RFC? If so, could you please approve the changes so I can merge it? Thanks!

@Mousius
Copy link
Member

Mousius commented Jun 7, 2022

@leandron, looking at Docker Hub (https://hub.docker.com/_/hello-world) it would appear the convention for image names is to use - there as well (i.e. tlcpack/ci-cpu rather than tlcpack/ci_cpu) can we go for that one?

Sure. I’ll push an updated version with this and @gromero’s suggestion as well.

@gromero the above change is still pending as far as I can see?

@gromero
Copy link
Contributor

gromero commented Jun 9, 2022

@leandron, looking at Docker Hub (https://hub.docker.com/_/hello-world) it would appear the convention for image names is to use - there as well (i.e. tlcpack/ci-cpu rather than tlcpack/ci_cpu) can we go for that one?

Sure. I’ll push an updated version with this and @gromero’s suggestion as well.

@gromero the above change is still pending as far as I can see?

@Mousius hrm right. So we'll need @leandron to make the next move here.

@driazati driazati mentioned this pull request Jul 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manupak manupak manupak left review comments

@driazati driazati driazati approved these changes

@Lunderberg Lunderberg Lunderberg left review comments

@Mousius Mousius Mousius requested changes

@mehrdadh mehrdadh mehrdadh approved these changes

@gromero gromero gromero approved these changes

@areusch areusch areusch approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@leandron @areusch @Mousius @gromero @Lunderberg @mehrdadh @driazati @manupak