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

Fix documentation links always pointing to master. #9217

Merged
merged 9 commits into from
Jan 5, 2021

Conversation

sugeeth14
Copy link
Contributor

@sugeeth14 sugeeth14 commented Dec 19, 2020

What does this PR do?

This PR fixes documentation issue. The issue is that hyperlinks pointing to code file in the docs always point to the master.
Reference issue : #7988

I found extlinks more appropriate than rst_epilog
as mentioned in the original issue because hyperlink replacement is not possible with rst_epilog
Stackoverflow issue reference : https://stackoverflow.com/questions/1227037/substitutions-inside-links-in-rest-sphinx

Although this fixes the issues as discussed I am unable to fix the issue in markup files which are https://huggingface.co/transformers/master/contributing.html and https://huggingface.co/transformers/master/notebooks.html
Please suggest if it is possible to do that as well.

Fixes #7988

@sgugger @LysandreJik

Copy link
Collaborator

@sgugger sgugger 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 tackling this! The solution you propose does not solve all the issues around properly linking the tagged versions of the repo but I think we can change it slightly to make it work!

This is not urgent as @LysandreJik is on vacation (until Jan 4th) and I would very much like his review on this before merging.

@@ -27,7 +27,8 @@
version = u''
# The full version, including alpha/beta/rc tags
release = u'4.1.1'

# Prefix link to always point to
extlinks = {'prefix_link': ('https://github.com/huggingface/transformers/blob/v4.1.1/%s', '')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what @LysandreJik had in mind here was to use the release variable so each time we generate the docs on a given commit, the links are prefixed for that version.

But at the same time, it would also mean the master docs would always point to the latest release which is also something we don't want. So I think this line should be here but commented out, so that we uncomment it on the commit that is used to generate the docs of a specific version (with the proper tag using the release variable). This way when we regularly build the master doc at each PR, the links point to master, but when we make a release, we can just uncomment to build the links to the version released.

Does that make sense to you?

Copy link
Contributor Author

@sugeeth14 sugeeth14 Dec 21, 2020

Choose a reason for hiding this comment

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

Yes you're right it should point to master until release and duing release it must be changed, I have made changes accordingly and committed. Does this satisfy the requirements ?

https://github.com/huggingface/transformers/pull/9217/files#diff-008dcb3426febd767787b1521f1fe33086313b927ea37eaab86df5fa88a51698R31

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it's good! Thanks a lot! Let's wait for @LysandreJik to be back to merge this if you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @sgugger I don't mind. We can wait.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the meantime, there seems to be some styling problems so make sure to run make style on your branch, and one error in the doc building step (I've added a comment in marian.rst where I think it comes from).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah was checking those,thought of fixing them in the meantime. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does make style fix a lot of files which I haven't changed at all also when I try to use style_doc.py I get an error as here https://app.circleci.com/pipelines/github/huggingface/transformers/17685/workflows/f1f56681-8976-402a-95bd-387bd066ccb7/jobs/141134 that 10 files shoud be restyled ?! Am I missing something here ?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I just do black docs/source/conf.py also there are changes from single quote to double quote
image

Comment on lines 59 to 64
- :prefix_link:`Fine-tune on TPU
<examples/seq2seq/builtin_trainer/train_distil_marian_enro_tpu.sh>`
- :prefix_link:`Fine-tune on GPU
<examples/seq2seq/builtin_trainer/train_distil_marian_enro.sh>
- :prefix_link:`Fine-tune on GPU with pytorch-lightning
<examples/seq2seq/distil_marian_no_teacher.sh>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those are all missing __ at the end and can probably fit in one line now.

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.

Hi, thanks for your work on this! I fixed the style and added a missing backtick so that all the tests pass.

Great work, thanks @sugeeth14!

@LysandreJik LysandreJik merged commit 314cca2 into huggingface:master Jan 5, 2021
@sugeeth14
Copy link
Contributor Author

Thanks for merging @LysandreJik but I want to know why the errors occurred when I ran make style as I see in #9217 (comment)

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.

[Good first issue] Documentation links in older docs versions
3 participants