DOC API docstring improvements #731

adrinjalali · 2022-03-02T13:59:28Z

This PR improves the docstrings for a few functions under huggingface_hub.

src/huggingface_hub/file_download.py

osanseviero

Thank you very much for this PR! I left a couple of suggestions 😄

Two quick questions

Do we want new lines between arguments? We don't do this anywhere else I think
Should we also add the argument types in the docstring?

osanseviero · 2022-03-03T13:43:23Z

src/huggingface_hub/file_download.py

-
-    Cloudfront is replicated over the globe so downloads are way faster for the end user (and it also lowers our
-    bandwidth costs).
+    """Resolve a url of a file from the given information.


Maybe people don't know what resolve a URL means. WDYT Of something along these lines?

Suggested change

"""Resolve a url of a file from the given information.

"""Creates a URL of a file based in the given information.

I think since there's an example, it should be clear. I'm not sure about using create here.

maybe "construct the URL"?

osanseviero · 2022-03-03T13:53:36Z

src/huggingface_hub/file_download.py

+    Args:
+        repo_id: A user or an organization name and a repo name seperated by a
+            ``/``.
+


nit: I don't think we add empty lines between parameters

I put them there in cases where there are many arguments. Without the lines it's not very readable to me, and they don't affect the rendered version anyway. Happy to remove them if you think they really should be removed.

I would go for consistency now and update later, WDYT?

src/huggingface_hub/file_download.py

adrinjalali

Should we also add the argument types in the docstring?

The docs suggest doing that only if type annotations are not present. I would be happier if we could put the default values in the doc though.

adrinjalali · 2022-03-03T14:23:11Z

src/huggingface_hub/file_download.py

-
-    Cloudfront is replicated over the globe so downloads are way faster for the end user (and it also lowers our
-    bandwidth costs).
+    """Resolve a url of a file from the given information.


I think since there's an example, it should be clear. I'm not sure about using create here.

adrinjalali · 2022-03-03T14:25:27Z

src/huggingface_hub/file_download.py

+    Args:
+        repo_id: A user or an organization name and a repo name seperated by a
+            ``/``.
+


I put them there in cases where there are many arguments. Without the lines it's not very readable to me, and they don't affect the rendered version anyway. Happy to remove them if you think they really should be removed.

src/huggingface_hub/file_download.py

osanseviero

Thank you for this! 🚀 it looks great

LysandreJik

This looks great, thanks for working on it! I've added comments, only related to the format.

LysandreJik · 2022-03-08T13:30:27Z

src/huggingface_hub/file_download.py

+    are more than a few MBs.
+
+    Args:
+        repo_id: A namespace (user or an organization) name and a repo name


I'd rather we add the types/default values directly in the docstring. That's what we do in the rest of the repository, and that's what's expected by the doc-builder tool which we'll use to build the docs for huggingface_hub. See documentation here.

We also add newlines between the argument name and type, and the description of that argument.

I'll add a few proposals. Right now the docs were setup with the Sphinx/RST format in mind (`` for markdown's `, ` for markdown's *, etc.), so for consistency's sake the proposals I'll add will re-use that format as well. If it isn't clear, feel free to use the format as defined in the document shared above, as the conversion should take place in 1-2 days anyway.

Other examples of this in huggingface_hub are in Repository:

huggingface_hub/src/huggingface_hub/repository.py

Lines 371 to 403 in 6dac5f4

"""

Instantiate a local clone of a git repo.

If specifying a `clone_from`:

will clone an existing remote repository, for instance one

that was previously created using ``HfApi().create_repo(name=repo_name)``.

``Repository`` uses the local git credentials by default, but if required, the ``huggingface_token``

as well as the git ``user`` and the ``email`` can be explicitly specified.

If `clone_from` is used, and the repository is being instantiated into a non-empty directory,

e.g. a directory with your trained model files, it will automatically merge them.

Args:

local_dir (``str``):

path (e.g. ``'my_trained_model/'``) to the local directory, where the ``Repository`` will be initalized.

clone_from (``str``, `optional`):

repository url (e.g. ``'https://huggingface.co/philschmid/playground-tests'``).

repo_type (``str``, `optional`):

To set when creating a repo: et to "dataset" or "space" if creating a dataset or space, default is model.

use_auth_token (``str`` or ``bool``, `optional`, defaults to ``True``):

huggingface_token can be extract from ``HfApi().login(username, password)`` and is used to authenticate against the hub

(useful from Google Colab for instance).

git_user (``str``, `optional`):

will override the ``git config user.name`` for committing and pushing files to the hub.

git_email (``str``, `optional`):

will override the ``git config user.email`` for committing and pushing files to the hub.

revision (``str``, `optional`):

Revision to checkout after initializing the repository. If the revision doesn't exist, a

branch will be created with that revision name from the default branch's current HEAD.

private (``bool``, `optional`, defaults to ``False``):

whether the repository is private or not.

skip_lfs_files (``bool``, `optional`, defaults to ``False``):

whether to skip git-LFS files or not.

"""

Suggested change

repo_id: A namespace (user or an organization) name and a repo name

repo_id (``str``):

A namespace (user or an organization) name and a repo name

LysandreJik · 2022-03-08T13:31:52Z

src/huggingface_hub/file_download.py

+        repo_id: A namespace (user or an organization) name and a repo name
+            seperated by a ``/``.
+
+        filename: The name of the file in the repo.


Suggested change

filename: The name of the file in the repo.

filename (``str``):

The name of the file in the repo.

LysandreJik · 2022-03-08T13:32:22Z

src/huggingface_hub/file_download.py

+
+        filename: The name of the file in the repo.
+
+        subfolder: An optional value corresponding to a folder inside the repo.


Suggested change

subfolder: An optional value corresponding to a folder inside the repo.

subfolder (``str``, `optional`): An optional value corresponding to a folder inside the repo.

LysandreJik · 2022-03-08T13:33:24Z

src/huggingface_hub/file_download.py

+        user_agent: The user-agent info in the form of a dictionary or a
+            string.
+
+        force_download: Whether the file should be downloaded even if it


When there is a default value other than None:

Suggested change

force_download: Whether the file should be downloaded even if it

force_download (``bool``, `optional`, defaults to ``False``):

Whether the file should be downloaded even if it

LysandreJik · 2022-03-08T13:44:34Z

src/huggingface_hub/file_download.py

    Raises:
-        In case of non-recoverable file (non-existent or inaccessible url + no cache on disk).
+        - ``EnvironmentError`` if ``use_auth_token=True`` and the token cannot
+            be found.
+
+        - ``OSError`` if ETag cannot be determined.
+
+        - ``ValueError`` if the file cannot be downloaded and cannot be found
+            locally.


LysandreJik · 2022-03-08T13:45:02Z

src/huggingface_hub/snapshot_download.py

+    # Note: at some point maybe this format of storage should actually replace
+    # the flat storage structure we've used so far (initially from allennlp
+    # if I remember correctly).
+


Better suited here, indeed!

adrinjalali · 2022-03-10T13:57:51Z

Hope this looks better. If there aren't anything I've missed @LysandreJik please feel free to merge :)

LysandreJik

This is great work, thanks for that @adrinjalali! Merging.

DOC API docstring improvements

82133a4

adrinjalali requested a review from LysandreJik March 2, 2022 13:59

improve snapshot_download's docstring

ceda062

adrinjalali commented Mar 2, 2022

View reviewed changes

src/huggingface_hub/file_download.py Outdated Show resolved Hide resolved

src/huggingface_hub/file_download.py Outdated Show resolved Hide resolved

osanseviero self-requested a review March 2, 2022 14:58

improve repo_type doc

bbab56f

osanseviero reviewed Mar 3, 2022

View reviewed changes

adrinjalali commented Mar 3, 2022

View reviewed changes

adrinjalali added 3 commits March 3, 2022 15:37

apply suggestions

9815ff7

add references section

7ec3e6a

use julien's wording

9d6893f

osanseviero approved these changes Mar 7, 2022

View reviewed changes

LysandreJik reviewed Mar 8, 2022

View reviewed changes

adrinjalali added 2 commits March 10, 2022 14:56

add types to docstrings

648e6ff

make sytle

f37c7e5

Merge branch 'main' into docs/api/1

f84dd96

LysandreJik approved these changes Mar 15, 2022

View reviewed changes

LysandreJik merged commit df4c6d9 into huggingface:main Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC API docstring improvements #731

DOC API docstring improvements #731

adrinjalali commented Mar 2, 2022

osanseviero left a comment

osanseviero Mar 3, 2022

adrinjalali Mar 3, 2022

julien-c Mar 7, 2022

osanseviero Mar 3, 2022

adrinjalali Mar 3, 2022

osanseviero Mar 7, 2022

adrinjalali left a comment

adrinjalali Mar 3, 2022

adrinjalali Mar 3, 2022

osanseviero left a comment

LysandreJik left a comment

LysandreJik Mar 8, 2022

LysandreJik Mar 8, 2022

LysandreJik Mar 8, 2022

LysandreJik Mar 8, 2022

LysandreJik Mar 8, 2022

LysandreJik Mar 8, 2022

adrinjalali commented Mar 10, 2022

LysandreJik left a comment

	"""Resolve a url of a file from the given information.
	"""Creates a URL of a file based in the given information.

	"""
	Instantiate a local clone of a git repo.

	If specifying a `clone_from`:
	will clone an existing remote repository, for instance one
	that was previously created using ``HfApi().create_repo(name=repo_name)``.
	``Repository`` uses the local git credentials by default, but if required, the ``huggingface_token``
	as well as the git ``user`` and the ``email`` can be explicitly specified.
	If `clone_from` is used, and the repository is being instantiated into a non-empty directory,
	e.g. a directory with your trained model files, it will automatically merge them.

	Args:
	local_dir (``str``):
	path (e.g. ``'my_trained_model/'``) to the local directory, where the ``Repository`` will be initalized.
	clone_from (``str``, `optional`):
	repository url (e.g. ``'https://huggingface.co/philschmid/playground-tests'``).
	repo_type (``str``, `optional`):
	To set when creating a repo: et to "dataset" or "space" if creating a dataset or space, default is model.
	use_auth_token (``str`` or ``bool``, `optional`, defaults to ``True``):
	huggingface_token can be extract from ``HfApi().login(username, password)`` and is used to authenticate against the hub
	(useful from Google Colab for instance).
	git_user (``str``, `optional`):
	will override the ``git config user.name`` for committing and pushing files to the hub.
	git_email (``str``, `optional`):
	will override the ``git config user.email`` for committing and pushing files to the hub.
	revision (``str``, `optional`):
	Revision to checkout after initializing the repository. If the revision doesn't exist, a
	branch will be created with that revision name from the default branch's current HEAD.
	private (``bool``, `optional`, defaults to ``False``):
	whether the repository is private or not.
	skip_lfs_files (``bool``, `optional`, defaults to ``False``):
	whether to skip git-LFS files or not.
	"""

	repo_id: A namespace (user or an organization) name and a repo name
	repo_id (``str``):
	A namespace (user or an organization) name and a repo name

	filename: The name of the file in the repo.
	filename (``str``):
	The name of the file in the repo.


		filename: The name of the file in the repo.

		subfolder: An optional value corresponding to a folder inside the repo.

	subfolder: An optional value corresponding to a folder inside the repo.
	subfolder (``str``, `optional`): An optional value corresponding to a folder inside the repo.

	force_download: Whether the file should be downloaded even if it
	force_download (``bool``, `optional`, defaults to ``False``):
	Whether the file should be downloaded even if it

DOC API docstring improvements #731

DOC API docstring improvements #731

Conversation

adrinjalali commented Mar 2, 2022

osanseviero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osanseviero left a comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Mar 10, 2022

LysandreJik left a comment

Choose a reason for hiding this comment