KEMBAR78
[docs] Update sphinx to 3.5.4 by garymm · Pull Request #61601 · pytorch/pytorch · GitHub
Skip to content

[docs] Update sphinx to 3.5.4 #61601

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[docs] Update sphinx to 3.5.4 #61601

wants to merge 1 commit into from

Conversation

@garymm
Copy link
Collaborator

@garymm garymm commented Jul 13, 2021 •
edited
Loading

Sphinx 4.x is out, but it seems that requires many more changes to
adopt. So instead use the latest version of 3.x, which includes
several nice features.

  • Add some noindex directives to deal with warnings that would otherwise
    be triggered by this change due to conflicts between the docstrings
    declaring a function and the autodoc extension declaring the
    same function.
  • Update distributions.utils.lazy_property to make it look like a
    regular property when sphinx autodoc inspects classes.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 13, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)

ci.pytorch.org: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

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

Click here to manually regenerate this comment.

@H-Huang H-Huang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 13, 2021
albanD
albanD reviewed Jul 19, 2021
View reviewed changes
@garymm
Copy link
Collaborator Author

garymm commented Jul 20, 2021

@albanD I can haz approval + import plz?

albanD
albanD approved these changes Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jbschlosser jbschlosser removed their request for review July 22, 2021 15:59
@garymm
Copy link
Collaborator Author

garymm commented Jul 26, 2021

@albanD can you please merge this?

@albanD
Copy link
Collaborator

albanD commented Jul 26, 2021

Hey!
I have been trying but this is breaking quite a lot of internal tests consistently (not sure why yet).
Could you actually do a rebase on top of master here to try and see if that helps?

@garymm
Copy link
Collaborator Author

garymm commented Jul 27, 2021

Rebased

@albanD
Copy link
Collaborator

albanD commented Jul 27, 2021

Ho we see the same failures here now :D So that was a valid race of something that landed in the meantime?

@garymm
[docs] Update sphinx to 3.5.4
ea56322 
Sphinx 4.x is out, but it seems that requires many more changes to
adopt. So instead use the latest version of 3.x, which includes
several nice features.

* Add some noindex directives to deal with warnings that would otherwise
  be triggered by this change due to conflicts between the docstrings
  declaring a function and the autodoc extension declaring the
  same function.
* Update distributions.utils.lazy_property and related code to make it
  look like a regular property when sphinx autodoc inspects classes.
@garymm
Copy link
Collaborator Author

garymm commented Jul 28, 2021

Reproduced the failures locally, fixed them, and pushed a new version.

@mattip mattip mentioned this pull request Jul 29, 2021
Closed
@facebook-github-bot
Copy link
Contributor

@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@albanD
Copy link
Collaborator

albanD commented Jul 29, 2021

Awesome!
Thanks for the fix.
I'll try to land that now!

@facebook-github-bot
Copy link
Contributor

@albanD merged this pull request in 9fdf7ec.

@albanD
Copy link
Collaborator

albanD commented Jul 30, 2021

@garymm while looking closer at the final doc, there are some un-expected spacing in the function definitions.
For example,
image
Becomes
image

With a lot of extra spacing and in particular between the nn and the Conv2d.

Do you think we can fix that?

@garymm
Copy link
Collaborator Author

garymm commented Jul 30, 2021

I see what you mean. I guess there's something that can be changed in the theme CSS, but I really have no experience with CSS.
@holly1238 @Bgreen23 any suggestions?

In the mean time I'll take a look to see if I can figure it out on my own.

If we need to revert, let me know and I will revert just the requirements.txt change and leave the other parts of this PR.

@garymm garymm deleted the sphinx branch July 30, 2021 18:10
@NicolasHug
Copy link
Member

For ref, I opened an issue about this a while ago: pytorch/pytorch_sphinx_theme#115
pytorch/pytorch_sphinx_theme#118 is also tangentially related

@albanD
Copy link
Collaborator

albanD commented Jul 30, 2021

@garymm I would love to be able to keep this new version in. I think it is ok for the master doc to not look optimal for a few days while we investigate this.

@garymm
Copy link
Collaborator Author

garymm commented Jul 30, 2021

Trying to fix this in pytorch/pytorch_sphinx_theme#134

@awgu awgu mentioned this pull request Aug 3, 2021
Closed
@mthrok mthrok mentioned this pull request Aug 6, 2021
Closed
@nateanl nateanl mentioned this pull request Aug 6, 2021
Merged
@trsvchn trsvchn mentioned this pull request Oct 19, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albanD albanD albanD approved these changes

@mattip mattip Awaiting requested review from mattip

@zasdfgbnm zasdfgbnm Awaiting requested review from zasdfgbnm

@VitalyFedyunin VitalyFedyunin Awaiting requested review from VitalyFedyunin

Assignees

No one assigned

Labels

cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@garymm @facebook-github-bot @albanD @NicolasHug @H-Huang @pytorchbot