[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678

sayakpaul · 2024-01-23T02:05:04Z

What does this PR do?

Explicitly adds the library name to be diffusers when using push_to_hub. In a follow-up PR, will add this to the officially maintained training scripts, too.

Internal discussion: https://huggingface.slack.com/archives/C04EX6W3QSY/p1704797847482439?thread_ts=1704785760.863359&cid=C04EX6W3QSY. Cc: @osanseviero as originally initiated by him.

Note to the reviewers:

We have never supported "tags". So, to me, it makes sense to start by just explicitly adding library_name to the model cards. If there's a need, we can always revisit this. Adding tags support should be easy now.
We have never relied on "ignore_metadata_errors". So, I have taken the liberty to set it to False where applicable.

src/diffusers/utils/hub_utils.py

patrickvonplaten

Can we maybe also use this PR as an opportunity to refactor all the "create model card" functions of the examples, e.g. here:

diffusers/examples/dreambooth/train_dreambooth.py

Line 70 in 318556b

def save_model_card(

?

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

sayakpaul · 2024-01-23T07:53:44Z

Can we maybe also use this PR as an opportunity to refactor all the "create model card" functions of the examples, e.g. here:

That should be in a separate PR, @patrickvonplaten and I feel relatively strongly about it. This is because we prepare the README.mds from the training scripts manually and don't rely on the ModelCard class from huggingface_hub. So, that will naturally cause changes unrelated to this PR.

HuggingFaceDocBuilderDev · 2024-01-23T07:55:15Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Wauplin

Thanks for making this PR @sayakpaul! Left a few comments. Main points are:

if a model card already exists and library_name is not set, let's set it to diffusers (not the case at the moment)
maybe have a default template more specific to diffusers, like SetFit is doing

src/diffusers/utils/hub_utils.py

tests/models/test_modeling_common.py

tests/pipelines/test_pipelines_common.py

Wauplin · 2024-01-23T09:02:20Z

tests/models/test_modeling_common.py

+        not is_jinja_available(),
+        reason="Model card tests cannot be performed with Jinja installed.",
+    )
+    def test_push_to_hub_library_name(self):


(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 the create_and_tag_model_card helper (or whatever its name :) )

Does 99ce47c work for you?

Oh, I didn't notice that there is a generate_model_card and a create_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 suggest generate_model_card).

Regarding the test, yes it looks good to me :)

Sorry, but it won't work since tmpdir is local. What's the best way to test here?

@Wauplin I merged them. However, I am not sure about the test since tmpdir is local and ModelCard.load() would fail there.

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 add library_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):

deprecate create_model_card (or even remove it completely) if it's not used

add in hub_utils.py a load_or_create_model_card helper that returns a ModelCard object without modifying anything. It's similar to the try: (ModelCard.load) except EntryNotFound: (ModelCard.from_template) part

add in hub_utils.py a populate_model_card that takes as input a ModelCard object and add library_name: diffusers if doesn't exist yet.

then in the codebase (for example in pipeline_utils.py), you would do

model_card = load_or_create_model_card(repo_id, token=token, is_pipeline=True) populate_model_card(model_card) model_card.save(os.path.join(save_directory, "README.md"))

WDTY?

(to take with a grain of salt, I'm not expert in diffusers codebase so I might be missing some parts)

(sorry @sayakpaul I didn't see your comment while posting this message)

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.

Co-authored-by: Lucain <lucainp@gmail.com>

tests/others/test_hub_utils.py

Wauplin · 2024-01-23T10:30:38Z

tests/models/test_modeling_common.py

+        not is_jinja_available(),
+        reason="Model card tests cannot be performed with Jinja installed.",
+    )
+    def test_push_to_hub_library_name(self):


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 add library_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):

deprecate create_model_card (or even remove it completely) if it's not used

add in hub_utils.py a load_or_create_model_card helper that returns a ModelCard object without modifying anything. It's similar to the try: (ModelCard.load) except EntryNotFound: (ModelCard.from_template) part

add in hub_utils.py a populate_model_card that takes as input a ModelCard object and add library_name: diffusers if doesn't exist yet.

then in the codebase (for example in pipeline_utils.py), you would do

model_card = load_or_create_model_card(repo_id, token=token, is_pipeline=True) populate_model_card(model_card) model_card.save(os.path.join(save_directory, "README.md"))

WDTY?

(to take with a grain of salt, I'm not expert in diffusers codebase so I might be missing some parts)

sayakpaul · 2024-01-23T12:10:24Z

tests/others/test_hub_utils.py



 class CreateModelCardTest(unittest.TestCase):
-    @patch("diffusers.utils.hub_utils.get_full_repo_name")
-    def test_create_model_card(self, repo_name_mock: Mock) -> None:


Follow the discussion here: #6678 (comment).

sayakpaul · 2024-01-23T12:10:59Z

src/diffusers/utils/hub_utils.py

    if not is_jinja_available():
        raise ValueError(
            "Modelcard rendering is based on Jinja templates."
            " Please make sure to have `jinja` installed before using `create_model_card`."
            " To install it, please run `pip install Jinja2`."
        )

-    if hasattr(args, "local_rank") and args.local_rank not in [-1, 0]:


Follow the discussion here: #6678 (comment).

Wauplin

Looks great! Thanks for all the iterations @sayakpaul! 🔥
Left some minor comments but overall approved it :)

EDIT: MODEL_CARD_TEMPLATE_PATH is not used anymore in diffusers. Might be worth removing the constants and deleting model_card_template.md file from the repo (except if this is considered as a breaking change). So that no one thinks the model card will be created from this template.

src/diffusers/utils/hub_utils.py

tests/others/test_hub_utils.py

src/diffusers/utils/hub_utils.py

src/diffusers/models/modeling_utils.py

Co-authored-by: Lucain <lucainp@gmail.com>

sayakpaul · 2024-01-23T14:10:42Z

EDIT: MODEL_CARD_TEMPLATE_PATH is not used anymore in diffusers. Might be worth removing the constants and deleting model_card_template.md file from the repo (except if this is considered as a breaking change). So that no one thinks the model card will be created from this template.

I have addressed this too. If you want to take another look, feel free to.

Will merge once @patrickvonplaten takes another look. In an immediate follow-up PR, will standard implementing a model card template.

Wauplin

Yay! 🎉

src/diffusers/pipelines/pipeline_utils.py

Co-authored-by: Lucain <lucainp@gmail.com>

sayakpaul · 2024-01-23T14:19:53Z

Will fix the failing test

sayakpaul · 2024-01-23T20:02:10Z

Alright. Things seem to be pristine now.

Let's see what Patrick has to say.

sayakpaul · 2024-01-26T01:56:59Z

@patrickvonplaten a gentle ping.

patrickvonplaten · 2024-01-26T12:22:23Z

Can we maybe also use this PR as an opportunity to refactor all the "create model card" functions of the examples, e.g. here:

That should be in a separate PR, @patrickvonplaten and I feel relatively strongly about it. This is because we prepare the README.mds from the training scripts manually and don't rely on the ModelCard class from huggingface_hub. So, that will naturally cause changes unrelated to this PR.

TBH, I think this PR is exactly abuot refactoring the examples because this is the no.1 use case when people use push_to_hub. It would also be a great opportunity to actually test the functionality here:

diffusers/examples/textual_inversion/textual_inversion.py

Line 87 in dc85b57

def save_model_card(repo_id: str, images=None, base_model=str, repo_folder=None):
diffusers/examples/dreambooth/train_dreambooth.py

Line 70 in dc85b57

def save_model_card(

If you feel very strongy about it, I'm ok with doing it in a new PR right after, but overall I think the main purpose of the changes here are exactly for the example scripts and therefore they should be updated accordingly

patrickvonplaten · 2024-01-26T12:22:34Z

@yiyixuxu can you also take a look?

sayakpaul · 2024-01-26T12:46:54Z

TBH, I think this PR is exactly abuot refactoring the examples because this is the no.1 use case when people use push_to_hub.

I don't think so and the description makes it quite clear. See the Slack thread too. The purpose of this PR is to provide the diffusers model name when we do push to hub. I anticipate the training scripts would require a lot more changes because none of those share a unified approach when it comes to prepping the model cards.

Therefore, I gently request you to reconsider what you suggested.

Also how do you propose testing?

patrickvonplaten · 2024-01-26T13:16:13Z

If you feel strongly, ok to merge for me, but let's please not forgot to update the example scripts.

Regarding testing, I would first test whether everything works as expected by using it in the example scripts (which is why I propose to change it here) 😅 Right now we know what the model cards looks like when training dreambooth - it would be good to see how the model card looks like when changing this here:

diffusers/examples/dreambooth/train_dreambooth.py

Line 93 in dc85b57

- diffusers

sayakpaul · 2024-01-26T17:31:36Z

That is alright. I am going to merge and my next PR will be about updating the scripts. Thanks a mile for beating it with me!

yiyixuxu · 2024-01-26T18:07:19Z

thanks for adding this @sayakpaul
We can ask the community to help update the scripts too, no?

sayakpaul · 2024-01-27T03:59:00Z

We can ask the community to help update the scripts too, no?

Sure, but the examples differ in how the model cards are created. So, we need to have a solid reference PR first. This is what I am gonna work on next :)

…ngface#6678) * feat: explicitly tag to diffusers when using push_to_hub * remove tags. * reset repo. * Apply suggestions from code review Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> * fix: tests * fix: push_to_hub behaviour for tagging from save_pretrained * Apply suggestions from code review Co-authored-by: Lucain <lucainp@gmail.com> * Apply suggestions from code review Co-authored-by: Lucain <lucainp@gmail.com> * import fixes. * add library name to existing model card. * add: standalone test for generate_model_card * fix tests for standalone method * moved library_name to a better place. * merge create_model_card and generate_model_card. * fix test * address lucain's comments * fix return identation * Apply suggestions from code review Co-authored-by: Lucain <lucainp@gmail.com> * address further comments. * Update src/diffusers/pipelines/pipeline_utils.py Co-authored-by: Lucain <lucainp@gmail.com> --------- Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com> Co-authored-by: Lucain <lucainp@gmail.com>

feat: explicitly tag to diffusers when using push_to_hub

d5f2cf6

sayakpaul requested review from Wauplin and patrickvonplaten January 23, 2024 02:05

sayakpaul added 2 commits January 23, 2024 07:37

remove tags.

dc7f55d

reset repo.

91b26ff

patrickvonplaten reviewed Jan 23, 2024

View reviewed changes

src/diffusers/utils/hub_utils.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Jan 23, 2024

View reviewed changes

sayakpaul and others added 3 commits January 23, 2024 13:12

Merge branch 'main' into add-diffusers-tag

156586f

Apply suggestions from code review

03a704a

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

fix: tests

d33ed6c

fix: push_to_hub behaviour for tagging from save_pretrained

2baf0d2

Wauplin reviewed Jan 23, 2024

View reviewed changes

sayakpaul and others added 5 commits January 23, 2024 14:42

Apply suggestions from code review

5d9e664

Co-authored-by: Lucain <lucainp@gmail.com>

Apply suggestions from code review

62ddbb8

Co-authored-by: Lucain <lucainp@gmail.com>

import fixes.

0d73555

add library name to existing model card.

5297ad4

add: standalone test for generate_model_card

99ce47c

sayakpaul requested review from Wauplin and patrickvonplaten January 23, 2024 09:29

sayakpaul added 5 commits January 23, 2024 15:00

Merge branch 'main' into add-diffusers-tag

19d26da

fix tests for standalone method

2b93dcc

moved library_name to a better place.

0f31032

merge create_model_card and generate_model_card.

987178b

fix test

5bd864c

Wauplin reviewed Jan 23, 2024

View reviewed changes

sayakpaul added 2 commits January 23, 2024 16:23

address lucain's comments

33e2d91

fix return identation

322c0e1

sayakpaul requested a review from Wauplin January 23, 2024 12:01

sayakpaul commented Jan 23, 2024

View reviewed changes

Wauplin approved these changes Jan 23, 2024

View reviewed changes

sayakpaul and others added 2 commits January 23, 2024 19:36

Apply suggestions from code review

73ea51d

Co-authored-by: Lucain <lucainp@gmail.com>

address further comments.

e32e5e1

Merge branch 'main' into add-diffusers-tag

6b36050

Wauplin approved these changes Jan 23, 2024

View reviewed changes

src/diffusers/pipelines/pipeline_utils.py Outdated Show resolved Hide resolved

Update src/diffusers/pipelines/pipeline_utils.py

ffc3845

Co-authored-by: Lucain <lucainp@gmail.com>

Merge branch 'main' into add-diffusers-tag

183fd65

Merge branch 'main' into add-diffusers-tag

2d5c555

Wauplin mentioned this pull request Jan 26, 2024

[Hub + Examples] Standardize model cards in the training scripts #5667

Closed

sayakpaul merged commit d4c7ab7 into main Jan 26, 2024

sayakpaul deleted the add-diffusers-tag branch January 27, 2024 03:59

sayakpaul mentioned this pull request Jan 27, 2024

[Model Card] standardize dreambooth model card #6729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678

[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678

sayakpaul commented Jan 23, 2024

patrickvonplaten left a comment

sayakpaul commented Jan 23, 2024

HuggingFaceDocBuilderDev commented Jan 23, 2024

Wauplin left a comment

Wauplin Jan 23, 2024

sayakpaul Jan 23, 2024

Wauplin Jan 23, 2024

sayakpaul Jan 23, 2024

sayakpaul Jan 23, 2024

Wauplin Jan 23, 2024

Wauplin Jan 23, 2024

sayakpaul Jan 23, 2024

Wauplin Jan 23, 2024

sayakpaul Jan 23, 2024

sayakpaul Jan 23, 2024

Wauplin left a comment •

edited

Loading

sayakpaul commented Jan 23, 2024

Wauplin left a comment

sayakpaul commented Jan 23, 2024

sayakpaul commented Jan 23, 2024

sayakpaul commented Jan 26, 2024

patrickvonplaten commented Jan 26, 2024

patrickvonplaten commented Jan 26, 2024

sayakpaul commented Jan 26, 2024 •

edited

Loading

patrickvonplaten commented Jan 26, 2024

sayakpaul commented Jan 26, 2024

yiyixuxu commented Jan 26, 2024

sayakpaul commented Jan 27, 2024

[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678

[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678

Conversation

sayakpaul commented Jan 23, 2024

What does this PR do?

patrickvonplaten left a comment

Choose a reason for hiding this comment

sayakpaul commented Jan 23, 2024

HuggingFaceDocBuilderDev commented Jan 23, 2024

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin left a comment • edited Loading

Choose a reason for hiding this comment

sayakpaul commented Jan 23, 2024

Wauplin left a comment

Choose a reason for hiding this comment

sayakpaul commented Jan 23, 2024

sayakpaul commented Jan 23, 2024

sayakpaul commented Jan 26, 2024

patrickvonplaten commented Jan 26, 2024

patrickvonplaten commented Jan 26, 2024

sayakpaul commented Jan 26, 2024 • edited Loading

patrickvonplaten commented Jan 26, 2024

sayakpaul commented Jan 26, 2024

yiyixuxu commented Jan 26, 2024

sayakpaul commented Jan 27, 2024

Wauplin left a comment •

edited

Loading

sayakpaul commented Jan 26, 2024 •

edited

Loading