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

Remove modelcards dependency #2050

Merged

Conversation

Wauplin
Copy link
Collaborator

@Wauplin Wauplin commented Jan 20, 2023

While working on #2047, I realized that diffusers depends on modelcards package which is deprecated and not maintained anymore. This PR switches to the huggingface_hub implementation -a copy of modelcards with a few fixes-. It has been integrated to hfh in version 0.10 which is already a requirement for diffusers.

To make it work, one must have Jinja2 installed which was already a requirement. I've updated the create_model_card helper and wrote a test for it. Actually I'm not sure it was working at all as the template path seemed to be wrong.

(this PR also removed is_modelcards_available boolean method. Idk the deprecation policy in diffusers but I can put it back if needed. I have added a is_jinja_available (from huggingface_hub.utils) which is more appropriate now.

Note: this is not related to the 0.12 release of huggingface_hub. It can be merged before or after it doesn't matter (or never)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jan 20, 2023

The documentation is not available anymore as the PR was closed or merged.

@@ -87,10 +83,11 @@ def get_full_repo_name(model_id: str, organization: Optional[str] = None, token:


def create_model_card(args, model_name):
if not is_modelcards_available:
if not is_jinja_available():
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@@ -21,6 +21,7 @@
from collections import OrderedDict
from typing import Union

from huggingface_hub.utils import is_jinja_available # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure this is supported since huggingface_hub 0.10 ? That's currently our minimum required version for the Hub.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah you answered that above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Very clean! Good to merge for me. Let's wait until the tests are green and then we can merge :-)

@patrickvonplaten patrickvonplaten merged commit bcb4767 into huggingface:main Jan 20, 2023
@Wauplin
Copy link
Collaborator Author

Wauplin commented Jan 20, 2023

That was a quick review! 😄 Thanks

@Wauplin Wauplin deleted the remove-modelcards-dependency branch January 20, 2023 15:40
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* Switch to huggingface_hub.ModelCard

* Remove modelcards dependency in favor of Jinja2
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.

3 participants