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

Use dataclasses for all objects returned by HfApi #1911 #1974

Merged
merged 5 commits into from
Jan 17, 2024

Conversation

Ahmedniz1
Copy link
Contributor

No description provided.

@Ahmedniz1
Copy link
Contributor Author

@Wauplin I've updated the code for bloblfsinfo class. What do I need to make sure to commit the PR.
Thanks

Copy link
Contributor

@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 @Ahmedniz1 thanks for starting on this PR. I've left 2 small comments on it. Apart from that, you'll need to:

  1. Make sure all instantiations of BlobLfsInfo are now passing a dictionary as input. For example, this line must be replaced by lfs = BlobLfsInfo(lfs)
  2. Format your code to comply with the codebase rules. To do so, install the dev dependencies (pip install -e ".[dev]") and run the formatter (make style). Then you can check everything's good (make quality) and finally commit the changes done to the code (most likely additional whitespace + newlines).
  3. Finally it is generally a good practice to add a short description to the PR (here) and especially mention that it's solving issue Use dataclasses for all objects returned by HfApi #1911. This way we have a connection between the issue and the related PR.

That should be it! Thanks for your contribution! :)

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@Ahmedniz1 Ahmedniz1 changed the title converted bloblfsinfo from typed_dict to dataclass Use dataclasses for all objects returned by HfApi #1911 Jan 17, 2024
@Ahmedniz1
Copy link
Contributor Author

@Wauplin I've made the required changes, I hope it resolves the conversion of bloblfsinfo class. If there is any issue in it, please do let me know as I'm new to open source.
Thanks

@HuggingFaceDocBuilderDev

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.

Copy link
Contributor

@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.

Hi @Ahmedniz1, thanks for making the changes! The CI failed tests made me realize that the current solution that I've suggested to make is not satisfying in term of backward compatibility. I'm suggesting you some new changes that should make it cleaner and more robust for downstream apps. I'm sorry about this change of instructions, the fault is on my end. Better to fix it now rather than when it's fixed though :)

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Thanks for making the changes @Ahmedniz1! Ending up with a small and clean PR in the end which is nice :) Let's wait for the CI to complete and then we should be fine to merge this! 🎉

@Wauplin
Copy link
Contributor

Wauplin commented Jan 17, 2024

It's green! Merging it now. Thanks again :)

@Wauplin Wauplin merged commit 0385794 into huggingface:main Jan 17, 2024
14 checks passed
@Ahmedniz1
Copy link
Contributor Author

Thanks for guiding me in detail @Wauplin. I really appreciate that.
Now, I just want to confirm this, should I update other classes to use data class which were mentioned in issue #1911.
Thanks

@Wauplin
Copy link
Contributor

Wauplin commented Jan 18, 2024

@Ahmedniz1 yes you can! Thanks for the help. I think NouamaneELGueddarii is working on the TransformerInfo class but you can work on the other if you'd like. You can make several classes in the same PR now that the process is clearer (both for you and for me). Just make sure to post on #1911 about it so that everyone knows on what you are working. Thanks in advance!

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.

None yet

3 participants