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

Revisit documentation part 1: divio-like landing page, installation page, configuration page #1233

Merged
merged 18 commits into from
Dec 1, 2022

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Nov 29, 2022

First PR to revisit a bit the documentation. Goal is to invest some time in revisiting the documentation but to still make those changes incrementally. See #1226 for a more exhaustive list of what can be planned/what is planned to be improved "at some point".

PR includes:

  • update the landing page to adopt the divio framework
  • add guides / tutorials / concepts / references folders and section. Currently some are empty. Existing guides have been kept in the same place to avoid breaking urls
  • add a installation page to detail how to install huggingface_hub
  • add a configuration page to describe environment variables
  • (very) slightly revisit the quickstart page but no big changes

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 29, 2022

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

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Nice refactor! cc @stevhliu, who has done a similar work across our other libraries in the past.

I'm not convinced about the empty sections to be populated later; I'd rather this PR focused on an initial refactoring, and for these sections to appear once they're being populated

Comment on lines 1 to 7
# Concepts

In this section, you will find high-level explanations for building a better understanding
of important topics such as the philosophy behind `huggingface_hub`, the git-based vs http-based
paradigm or the cache system internals.

This section is currently empty but will get populated soon.
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about adding a new empty page to be populated later? I would add it when there is an actual need to reduce the noise

Copy link
Member

Choose a reason for hiding this comment

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

I agree it may be better to leave the "coming soon" docs out for now until they're ready. In addition to noise, it also creates expectations from users, and when the new doc isn't published soon some users may be disappointed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get your point. I removed them in dd95492.

@@ -0,0 +1,133 @@
# Configuration
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name it environment_variables as this page is focused solely on that?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise I would expect this page to also contain info about stuff such as lfs-enable-largefiles which is also part of the "Configuration" but out of scope for this specific reference

Copy link
Member

Choose a reason for hiding this comment

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

Great job documenting all these environment variables!

I'd consider moving this page to the Reference section, mention a few of the most important (or common) ones on the Installation page, and linking to the rest of them. While this is useful info, I don't think it is necessarily important for the majority of users who are getting started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the configuration page to package_references/environment_variables in 7c145cf. Thanks for the feedback !

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.

Super cool to see another library using this system! I mostly added some comments on polishing the new installation page :)

docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Dec 1, 2022

Thanks a lot @LysandreJik and @stevhliu for the feedback ! I integrated all of your comments. They helped a lot 👍 👍

@Wauplin Wauplin merged commit e6f7737 into main Dec 1, 2022
@Wauplin Wauplin deleted the revisit-documentation-part-1 branch December 1, 2022 07:47
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.

I had left some comments but didn't get to submit them before this was merged

docs/source/configuration.mdx Outdated Show resolved Hide resolved

### HF_ENDPOINT

To configure the 🤗 Hub base url. You might want to set this variable if your organization
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention what is the base url or what it is used for

docs/source/configuration.mdx Outdated Show resolved Hide resolved
a HTTP request to check that a new version is not available. Setting `HF_HUB_OFFLINE=1` will
skip this call which speeds up your loading time.

### HF_HUB_DISABLE_IMPLICIT_TOKEN
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to document publicly all environment variables? That might make it harder to disable env variables, specially for the ones that are more intended to be for internal use cases.

docs/source/configuration.mdx Outdated Show resolved Hide resolved
docs/source/configuration.mdx Outdated Show resolved Hide resolved
docs/source/index.mdx Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
docs/source/installation.mdx Show resolved Hide resolved
docs/source/installation.mdx Outdated Show resolved Hide resolved
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

5 participants