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

API reference documentation #782

Merged
merged 8 commits into from
Mar 31, 2022
Merged

API reference documentation #782

merged 8 commits into from
Mar 31, 2022

Conversation

LysandreJik
Copy link
Member

Work in progress aiming to supply an API reference for the huggingface_hub python library.

Done from a feature branch as many may collaborate on the branch and it will be easier to setup the GitHub Actions CI in a first step.

@adrinjalali
Copy link
Contributor

I would appreciate if we setup the CI such that it doesn't require a branch from the same repo, and having the initial PR from a separate repo would make sure the CI runs fine.

If we expect this to be something that multiple people work one, we could have a branch in the repo for it, and have PRs to that branch instead of main, and once we're comfortable with the branch, we can have a single PR to merge it with main. This way multiple people could also send separate PRs to the same branch and there wouldn't be conflicts between their work for working on the same branch.

@LysandreJik
Copy link
Member Author

Regarding the CI: yes, that is the goal, but it is not ready right now and needs iteration, which I hope to do right here.

Regarding the second point, I'm not sure I understand; that is absolutely the goal as well. I am creating an initial commit on a feature branch, which takes care of most of the work. As you can see, it is not yet ready as the CI did not run, a very important step in order to collaborate on this specific branch.

We can then work on that branch through PRs as you mention, once I set this branch as ready for contributions.

How would you have managed this differently?

@adrinjalali
Copy link
Contributor

Fair enough. I though you meant for people to directly push to this branch. I'm happy if people push to this branch through PRs from their forks.

I would create a branch with no changes, and then create a PR from my fork to that branch with the initial changes you're proposing here. But that's a minor difference. I'm happy with what you propose :)

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@LysandreJik
Copy link
Member Author

LysandreJik commented Mar 23, 2022

We should be good to go to work on this feature branch now. As mentioned by the bot above, the development can be seen directly on the following URL: https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_782/en/index

From what I've seen, here are the remaining steps:

The following high-level objectives need to be handled

  • Not all images are rendered correctly
  • Document everything missing from doc-builder's README & alongside all that was needed to get up to this point.

The following methods have not yet been checked for their documentation:

  • hf_api.model_info
  • hf_api.list_metrics
  • hf_api.list_repo_files
  • hf_api.dataset_info
  • hf_api.update_repo_visibility
  • hf_api.move_repo
  • hf_api.upload_file
  • hf_api.delete_file
  • hf_folder.save_token
  • hf_folder.get_token
  • hf_folder.delete_token
  • get_logger
  • get_verbosity
  • set_verbosity
  • set_verbosity_info
  • set_verbosity_debug
  • set_verbosity_warning
  • set_verbosity_error
  • disable_propagation
  • enable_propagation
  • snapshot_download
  • InferenceAPI
  • ModelHubMixin
  • PyTorchModelHubMixin
  • KerasModelHubMixin
  • from_pretrained_keras
  • push_to_hub_keras
  • save_pretrained_keras

@adrinjalali
Copy link
Contributor

We should be good to go to work on this feature branch now.

Not sure if I understand. So you mean we can now send PRs to this branch?

@LysandreJik
Copy link
Member Author

Yes, that's what I mean!

@LysandreJik
Copy link
Member Author

Opened a first PR here which takes care of all the remaining methods mentioned above: #799

Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Same with long lines and hyperlink issues here.

docs/source/package_reference/hf_api.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/hf_api.mdx Outdated Show resolved Hide resolved
LysandreJik and others added 2 commits March 29, 2022 09:34
* Finalize docstrings

* Endpoint helpers

* Run on doc-frontend target

* It was just another GitHub Actions issue

* Address review comments

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

* Address review comments

* Add deprecation warnings

* Precise deprecation warnings

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@LysandreJik
Copy link
Member Author

PR was merged, but GitHub Actions didn't launch the workflow (it seems to be down) to update the documentation yet, please be aware of it when reviewing if checking the documentation.

Since the delete documentation didn't launch either, the documentation of this PR with the commit above can be seen here

@LysandreJik
Copy link
Member Author

LysandreJik commented Mar 29, 2022

@stevhliu, @osanseviero, could you take a look at this PR? Would love to hear your feedback about the content.

This will likely be put at https://huggingface.co/docs/python-wrapper

Copy link
Member

@osanseviero osanseviero 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, thank you for this PR! I did a pass through most files (missing repository and endpoint_helpers to make sure they are consistent.

docs/source/package_reference/mixins.mdx 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
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/logging.py Outdated Show resolved Hide resolved
Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

This is great, I learned a lot from reading this! 🎉

What do you think about adding a brief code example for each function like you already have for the DatasetFilter section? This could help cover functions/methods we don't explicitly cover in the tutorials/guides (maybe because they aren't commonly used but they're still useful).

It'd be quite a bit to add though so we can always tackle it in a later PR. ❤️

docs/source/package_reference/hf_api.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/logging.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/logging.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/logging.mdx Outdated Show resolved Hide resolved
docs/source/package_reference/logging.mdx Outdated Show resolved Hide resolved
src/huggingface_hub/utils/endpoint_helpers.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/endpoint_helpers.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/endpoint_helpers.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/endpoint_helpers.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/endpoint_helpers.py Outdated Show resolved Hide resolved
Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>
Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>
@julien-c
Copy link
Member

This will likely be put at https://huggingface.co/docs/python-wrapper

For consistency I would rather serve the doc at https://huggingface.co/docs/huggingface_hub, but add "Client library" or "Python wrapper" to the titles etc so it's not confusing. Thoughts?

@LysandreJik LysandreJik marked this pull request as ready for review March 30, 2022 09:31
@LysandreJik
Copy link
Member Author

LysandreJik commented Mar 30, 2022

This PR should be ready to be merged.

When merged, I would like to open issues for the following:

  • On doc-builder, open issues to ask for support of a few items:
    • Different colors for <Tip>, which should have different severity levels
    • Mention that in-parameter-docstrings doesn't support code samples
    • Raises and Yields are not rendered correctly
    • versionadded and other directives that would have been useful
  • On huggingface_hub, open issues to track the following:
    • Lack of images right now, and definition of a way to provide images to the hub

If this looks good to you, @osanseviero @stevhliu, I'll merge this PR.

@@ -0,0 +1,24 @@
- sections:
Copy link
Member

@coyotte508 coyotte508 Mar 31, 2022

Choose a reason for hiding this comment

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

I think a title should also be added here (like below with title: "Reference").

image

https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_782/en/index

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, good catch! Will add this.

Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Happy to merge and improve on smaller PRs. This is already too large.

Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@LysandreJik
Copy link
Member Author

Thanks all for the reviews, all green, merging this PR! Will open a few relevant issues as discussed above.

@LysandreJik LysandreJik merged commit 3e72bc8 into main Mar 31, 2022
@LysandreJik LysandreJik deleted the doc-frontend branch March 31, 2022 10:04
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

7 participants