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.

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

build_image.py: tag->tag_suffix #865

Merged
merged 1 commit into from
Jan 27, 2025
Merged

build_image.py: tag->tag_suffix #865

merged 1 commit into from
Jan 27, 2025

Conversation

supersergiy
Copy link
Member

It's unintuitive that the actual docker tag is not identical to --tag argument given to the script. Too much know-how

@supersergiy supersergiy requested a review from nkemnitz January 24, 2025 19:10
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (68e5fd1) to head (6757e1f).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #865   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          142       142           
  Lines         6066      6066           
=========================================
  Hits          6066      6066           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nkemnitz
Copy link
Collaborator

The python tag was supposed to make it easier catching issues from scheduler and image versions being out of sync. I was even thinking I should add more tags for Pytorch and CUDA version, similar to how their official images are tagged.

Doesn't it show the full image path with name and tag at the end of the build_image.py script? I usually copy it from there, because I can never remember the Artifact Registry repo path.

@nkemnitz
Copy link
Collaborator

I guess we could use labels, that would at least allow finding and filtering the image with the correct version.
Maybe the builder could check if scheduler environment and docker image are compatible

@supersergiy
Copy link
Member Author

I see your argument, but I feel like "tag" has very specific meaning in docker context, and overloading it to mean just a part of the tag is confusing. How about we rename that argument to "tag_suffix"?

I've never looked into labels, but it might be a good solution too. Lmk what you prefer

@supersergiy supersergiy changed the title remove implicit py tag from docker build_image.py: tag->tag_suffix Jan 26, 2025
@supersergiy
Copy link
Member Author

@nkemnitz made the change. Couldn't verify it works because I'm having an access issue, but hope that will resolve independently

Copy link
Collaborator

@nkemnitz nkemnitz left a comment

Choose a reason for hiding this comment

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

Sounds good, no objection.

@nkemnitz nkemnitz merged commit 320413a into main Jan 27, 2025
10 checks passed
@nkemnitz nkemnitz deleted the no-implicit-py-tag branch January 27, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants