-
Notifications
You must be signed in to change notification settings - Fork 6.5k
[Hub] feat: explicitly tag to diffusers when using push_to_hub #6678
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
Changes from 15 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
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,8 @@ | |
| import numpy as np | ||
| import requests_mock | ||
| import torch | ||
| from huggingface_hub import delete_repo | ||
| from huggingface_hub import ModelCard, delete_repo | ||
| from huggingface_hub.utils import is_jinja_available | ||
| from requests.exceptions import HTTPError | ||
|
|
||
| from diffusers.models import UNet2DConditionModel | ||
|
|
@@ -732,3 +733,26 @@ def test_push_to_hub_in_organization(self): | |
|
|
||
| # Reset repo | ||
| delete_repo(self.org_repo_id, token=TOKEN) | ||
|
|
||
| @unittest.skipIf( | ||
| not is_jinja_available(), | ||
| reason="Model card tests cannot be performed without Jinja installed.", | ||
| ) | ||
| def test_push_to_hub_library_name(self): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't notice that there is a Regarding the test, yes it looks good to me :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, but it won't work since There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Maybe what I would do to solve this (and sorry if it's a revamp of the PR):
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
| model = UNet2DConditionModel( | ||
| block_out_channels=(32, 64), | ||
| layers_per_block=2, | ||
| sample_size=32, | ||
| in_channels=4, | ||
| out_channels=4, | ||
| down_block_types=("DownBlock2D", "CrossAttnDownBlock2D"), | ||
| up_block_types=("CrossAttnUpBlock2D", "UpBlock2D"), | ||
| cross_attention_dim=32, | ||
| ) | ||
| model.push_to_hub(self.repo_id, token=TOKEN) | ||
|
|
||
| model_card = ModelCard.load(f"{USER}/{self.repo_id}", token=TOKEN).data | ||
| assert model_card.library_name == "diffusers" | ||
|
|
||
| # Reset repo | ||
| delete_repo(self.repo_id, token=TOKEN) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,40 +12,61 @@ | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import os | ||
| import unittest | ||
| from pathlib import Path | ||
| from tempfile import TemporaryDirectory | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| import diffusers.utils.hub_utils | ||
| from diffusers.utils.hub_utils import create_model_card, generate_model_card | ||
|
|
||
|
|
||
| class CreateModelCardTest(unittest.TestCase): | ||
| def create_dummy_args(self, output_dir): | ||
| # Dummy args values | ||
| args = Mock() | ||
| args.output_dir = output_dir | ||
| args.local_rank = 0 | ||
| args.hub_token = "hub_token" | ||
| args.dataset_name = "dataset_name" | ||
| args.learning_rate = 0.01 | ||
| args.train_batch_size = 100000 | ||
| args.eval_batch_size = 10000 | ||
| args.gradient_accumulation_steps = 0.01 | ||
| args.adam_beta1 = 0.02 | ||
| args.adam_beta2 = 0.03 | ||
| args.adam_weight_decay = 0.0005 | ||
| args.adam_epsilon = 0.000001 | ||
| args.lr_scheduler = 1 | ||
| args.lr_warmup_steps = 10 | ||
| args.ema_inv_gamma = 0.001 | ||
| args.ema_power = 0.1 | ||
| args.ema_max_decay = 0.2 | ||
| args.mixed_precision = True | ||
| return args | ||
|
|
||
| @patch("diffusers.utils.hub_utils.get_full_repo_name") | ||
| def test_create_model_card(self, repo_name_mock: Mock) -> None: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow the discussion here: #6678 (comment). |
||
| repo_name_mock.return_value = "full_repo_name" | ||
| with TemporaryDirectory() as tmpdir: | ||
| # Dummy args values | ||
| args = Mock() | ||
| args.output_dir = tmpdir | ||
| args.local_rank = 0 | ||
| args.hub_token = "hub_token" | ||
| args.dataset_name = "dataset_name" | ||
| args.learning_rate = 0.01 | ||
| args.train_batch_size = 100000 | ||
| args.eval_batch_size = 10000 | ||
| args.gradient_accumulation_steps = 0.01 | ||
| args.adam_beta1 = 0.02 | ||
| args.adam_beta2 = 0.03 | ||
| args.adam_weight_decay = 0.0005 | ||
| args.adam_epsilon = 0.000001 | ||
| args.lr_scheduler = 1 | ||
| args.lr_warmup_steps = 10 | ||
| args.ema_inv_gamma = 0.001 | ||
| args.ema_power = 0.1 | ||
| args.ema_max_decay = 0.2 | ||
| args.mixed_precision = True | ||
| args = self.create_dummy_args(output_dir=tmpdir) | ||
|
|
||
| # Model card mush be rendered and saved | ||
| diffusers.utils.hub_utils.create_model_card(args, model_name="model_name") | ||
| create_model_card(args, model_name="model_name") | ||
| self.assertTrue((Path(tmpdir) / "README.md").is_file()) | ||
|
|
||
| def test_generate_existing_model_card_with_library_name(self): | ||
| with TemporaryDirectory() as tmpdir: | ||
| content = "hello" | ||
| with open(os.path.join(tmpdir, "README.md"), "w") as f: | ||
| f.write(content) | ||
|
|
||
| model_card = generate_model_card(tmpdir) | ||
| assert model_card.data.library_name == "diffusers" | ||
|
|
||
| def test_generate_model_card_with_library_name(self): | ||
| with TemporaryDirectory() as tmpdir: | ||
| model_card = generate_model_card(tmpdir) | ||
|
|
||
| model_card = generate_model_card(tmpdir) | ||
| assert model_card.data.library_name == "diffusers" | ||
sayakpaul marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.