Skip to content

Conversation

@sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Oct 14, 2023

Fixes: #5383.

@stevhliu cc for the doc change.

@Wauplin for visibility and awareness.

@sayakpaul sayakpaul requested a review from pcuenca October 14, 2023 06:15
Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

nice! cc @Wauplin

Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for dealing with it so fast @sayakpaul!

There's still a missing replacement in the korean translation of the docs:

>>> from huggingface_hub import HfFolder, Repository, whoami

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 16, 2023

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

@sayakpaul
Copy link
Member Author

Will merge when the CI tells me so!

@sayakpaul sayakpaul merged commit cc12f3e into main Oct 16, 2023
@sayakpaul sayakpaul deleted the update-with-hf-hub-repo branch October 16, 2023 14:04
Copy link
Collaborator

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Hey @sayakpaul thanks a lot for working on that! Love the fact that Repository is completely removed by this PR! 😍 I'm sorry I did not review it before (wanted to do it carefully).

I have spotted a few inconsistencies in the changes. For the run_as_future=True I am not sure if this is a bug or made on purpose. For the repo_id = create_repo(...) thingy it would have to be changed (otherwise it cannot work). Would you mind opening a followup PR for that? Thank you in advance!

Comment on lines +304 to +306
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... )
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... ).repo_id

Create_repo returns a RepoUrl object. To get the id, you need to do create_repo(...).repo_id

Comment on lines +304 to +306
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... )
... repo_id = create_repo(
... repo_id=config.hub_model_id or Path(config.output_dir).name, exist_ok=True
... ).repo_id

Create_repo returns a RepoUrl object. To get the id, you need to do create_repo(...).repo_id

Comment on lines +656 to +657
ignore_patterns=["step_*", "epoch_*"],
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ignore_patterns=["step_*", "epoch_*"],
)
ignore_patterns=["step_*", "epoch_*"],
run_as_future=True,
)

It looks like before blocking=False was used, meaning we would need to use run_as_future=True here to keep the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay as is because this is what we follow for the rest of the examples:

See here for additional reference:
#2934

Comment on lines +972 to +973
ignore_patterns=["step_*", "epoch_*"],
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ignore_patterns=["step_*", "epoch_*"],
)
ignore_patterns=["step_*", "epoch_*"],
run_as_future=True,
)

It looks like before blocking=False was used, meaning we would need to use run_as_future=True here to keep the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as before.

Comment on lines +679 to +680
ignore_patterns=["step_*", "epoch_*"],
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ignore_patterns=["step_*", "epoch_*"],
)
ignore_patterns=["step_*", "epoch_*"],
run_as_future=True,
)

It looks like before blocking=False was used, meaning we would need to use run_as_future=True here to keep the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as before.

Comment on lines +696 to +697
ignore_patterns=["step_*", "epoch_*"],
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ignore_patterns=["step_*", "epoch_*"],
)
ignore_patterns=["step_*", "epoch_*"],
run_as_future=True,
)

It looks like before blocking=False was used, meaning we would need to use run_as_future=True here to keep the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as before.

@sayakpaul
Copy link
Member Author

Since I just merged, I'd appreciate if you could directly PR :-)

sayakpaul added a commit that referenced this pull request Oct 17, 2023
* fix: create_repo()

* Empty-Commit
mhetrerajat pushed a commit to mhetrerajat/diffusers that referenced this pull request Oct 23, 2023
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* update training examples to use HFAPI.

* update training example.

* reflect the changes in the korean version too.

* Empty-Commit
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* fix: create_repo()

* Empty-Commit
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.

Use HfApi instead of Repository in training scripts

6 participants