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.
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678
[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678
Changes from 7 commits
d5f2cf6
dc7f55d
91b26ff
156586f
03a704a
d33ed6c
2baf0d2
5d9e664
62ddbb8
0d73555
5297ad4
99ce47c
19d26da
2b93dcc
0f31032
987178b
5bd864c
33e2d91
322c0e1
73ea51d
e32e5e1
6b36050
ffc3845
183fd65
2d5c555
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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) I would add an explicit test both for when the model card doesn't exist yet and for when the model card already exists. Maybe not needed to test the full
push_to_hub
method but simply thecreate_and_tag_model_card
helper (or whatever its name :) )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.
Does 99ce47c work for you?
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.
Oh, I didn't notice that there is a
generate_model_card
and acreate_model_card
in the hub utils. Should we merge them since they seem to do closely related things? (the difference is generating for a training or generating from anywhere, right?). Naming is misleading in that case (sorry, didn't notice before when I suggestgenerate_model_card
).Regarding the test, yes it looks good to me :)
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.
Sorry, but it won't work since
tmpdir
is local. What's the best way to test here?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.
@Wauplin I merged them. However, I am not sure about the test since
tmpdir
is local andModelCard.load()
would fail there.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.
I think tests are good. I've added a comment below to test from existing file with existing library_name that is not diffusers.
But what I meant above is that with this PR the hub utils feels clunky. We now have:
create_model_card
that creates a model card for a training. The method looks outdated and not used anywhere. However it introduces a template that is nice.generate_model_card
that either loads an existing model card or create a new one (from a different template) and addlibrary_name: diffusers
to it. This method is used in the codebase.Maybe what I would do to solve this (and sorry if it's a revamp of the PR):
create_model_card
(or even remove it completely) if it's not usedhub_utils.py
aload_or_create_model_card
helper that returns aModelCard
object without modifying anything. It's similar to thetry: (ModelCard.load) except EntryNotFound: (ModelCard.from_template)
parthub_utils.py
apopulate_model_card
that takes as input aModelCard
object and addlibrary_name: diffusers
if doesn't exist yet.pipeline_utils.py
), you would doWDTY?
(to take with a grain of salt, I'm not expert in
diffusers
codebase so I might be missing some parts)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.
(sorry @sayakpaul I didn't see your comment while posting this message)
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.
Makes sense. The latest commit should be have reflected these. Let me know if that makes sense.
I have opted to remove
create_model_card()
and the related test as it's not really used.