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

Docs: build with Sphinx 5 #5121

Merged
merged 3 commits into from
Jun 6, 2022
Merged

Conversation

adamjstewart
Copy link
Contributor

Companion to pytorch/pytorch#70309. See pytorch/pytorch#60979, pytorch/pytorch#61045, and sphinx-doc/sphinx#9395 for discussion.

I believe the reason that we were previously pinning to Sphinx 3 was because of issues with pytorch_sphinx_theme and Sphinx 4 support, but these seem to have been resolved now. See https://torchgeo.readthedocs.io/ for an example of docs built with pytorch_sphinx_theme and Sphinx 4.

@facebook-github-bot
Copy link

facebook-github-bot commented Dec 22, 2021

💊 CI failures summary and remediations

As of commit d06535e (more details on the Dr. CI page):


  • 5/5 failures introduced in this PR

5 failures not recognized by patterns:

Job Step Action
CircleCI binary_linux_wheel_py3.9_rocm4.2 packaging/build_wheel.sh 🔁 rerun
CircleCI binary_linux_wheel_py3.8_rocm4.2 packaging/build_wheel.sh 🔁 rerun
CircleCI binary_linux_wheel_py3.7_rocm4.2 packaging/build_wheel.sh 🔁 rerun
CircleCI binary_linux_wheel_py3.7_rocm4.3.1 packaging/build_wheel.sh 🔁 rerun
CircleCI binary_linux_wheel_py3.8_rocm4.3.1 packaging/build_wheel.sh 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@oke-aditya
Copy link
Contributor

Hi @adamjstewart !
Here are the rendered docs with sphinx 4.
https://1090793-73328905-gh.circle-artifacts.com/0/docs/index.html

In case you didn't know You can click on build_docs action and view the artifacts over circleCI.

I might have a finer look this weekend comparing the two :)

@NicolasHug
Copy link
Member

NicolasHug commented Dec 31, 2021

Thanks for the PR @adamjstewart . I'll keep an eye on pytorch/pytorch#70309 and merge this one when pytorch/pytorch#70309 is accepted on the pytorch core side.

@adamjstewart
Copy link
Contributor Author

@NicolasHug pytorch/pytorch#70309 has been merged, so this should be good to go once the tests pass.

@adamjstewart
Copy link
Contributor Author

CI failures look unrelated, is there a way to rerun the failing test?

@oke-aditya
Copy link
Contributor

Can you merge main branch to this and do a push?. Or click on update branch button in github UI
That will re trigger the CI and make easier to validate docs against latest codebase.

@adamjstewart
Copy link
Contributor Author

Different error this time. Do I rebase again?

@oke-aditya
Copy link
Contributor

oke-aditya commented Jun 3, 2022

You actually don't need to rebase.
Can you try fast forwarding by simplying doing git pull upstream main.
If it can't fast forward you would need to rebase :( .

Btw the docs are visible over

https://output.circle-artifacts.com/output/job/9d7975ec-5efc-4b4e-bcfb-e44948bb938e/artifacts/0/docs/index.html

I'm having a look

@oke-aditya
Copy link
Contributor

oke-aditya commented Jun 3, 2022

OK so it looks great with Sphinx 5. Some bugs are fixed. Mostly the font size looks bold (and better)

Visible difference in font and layout

Old

https://pytorch.org/vision/main/models.html#initializing-pre-trained-models

image

New

https://output.circle-artifacts.com/output/job/9d7975ec-5efc-4b4e-bcfb-e44948bb938e/artifacts/0/docs/models.html#initializing-pre-trained-models

image

Old

image

New

image

While a good thing is that the improper graying has gone. On the flip side nice contrast of each thumbnail has vanished

Apart from that @adamjstewart do you know what are some features that we could use with sphinx 5?

@adamjstewart
Copy link
Contributor Author

From looking at the Changelog it seems most of the changes in Sphinx 4 and 5 are changes to defaults in autodoc. For me, the main reason to upgrade to modern Sphinx is bugfixes for issues like pytorch/pytorch#60979. Also see sphinx-doc/sphinx#9395.

@adamjstewart
Copy link
Contributor Author

You actually don't need to rebase. Can you try fast forwarding by simplying doing git pull upstream main. If it can't fast forward you would need to rebase :( .

Both have the same result, it's just a difference of whether there's a merge commit added or not.

@oke-aditya
Copy link
Contributor

oke-aditya commented Jun 3, 2022

I don't see any error with the branch. Just that it's out of date with main branch. Rest all looks great 😀.

Somehow I prefer merge commit over rebase. Maybe a personal choice.

@adamjstewart
Copy link
Contributor Author

Seems like the tests are also failing on main...

@datumbox datumbox requested a review from NicolasHug June 6, 2022 11:27
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

The docs look fine although slightly worse IMO. But it's probably best to align with torch core to avoid integration issues.
Thanks for the PR @adamjstewart , LGTM

@adamjstewart adamjstewart deleted the docs/sphinx branch June 6, 2022 15:55
facebook-github-bot pushed a commit that referenced this pull request Jun 9, 2022
Summary:
* Docs: build with Sphinx 5

* Sphinx 5 no longer accepts language = None

Reviewed By: YosuaMichael

Differential Revision: D37038117

fbshipit-source-id: 24ac8b0a67fafb2004aea581026822564a7ab87e

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants