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.

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

DOC Update removed sphinx global var style in layout.html #26627

Merged
merged 9 commits into from Jun 23, 2023
Merged

DOC Update removed sphinx global var style in layout.html #26627

merged 9 commits into from Jun 23, 2023

Conversation

lucyleeow
Copy link
Member

Reference Issues/PRs

closes #26599

What does this implement/fix? Explain your changes.

Updates Sphinx style global variable, which was deprecated in sphinx 5.1 and removed in sphinx 7.0 to styles. Ref: https://github.com/sphinx-doc/sphinx/pull/11381/files

I know we have pinned Sphinx to 6.0.0 due to #25504, but this global var is deprecated even for 6.0.0, and we may want to update our Sphinx version in future.

Any other comments?

@github-actions
Copy link

github-actions bot commented Jun 20, 2023 •
edited

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: fc52183. Link to the linter CI: here

@lucyleeow
Copy link
Member Author

Ah styles was introduced in 5.1, after the min dependency 4.0.1. I think I will just close this for now and leave the issue for reference.

@lucyleeow lucyleeow closed this Jun 20, 2023
@lucyleeow lucyleeow deleted the doc_style branch June 20, 2023 06:11
@adrinjalali
Copy link
Member

I think we should just move the min dependency to 6.0

@adrinjalali
Copy link
Member

Cc @thomasjpfan

@thomasjpfan
Copy link
Member

I'm okay with updating the minimum Sphinx dependency to 6.0.

@lucyleeow lucyleeow restored the doc_style branch June 20, 2023 10:08
@lucyleeow
Copy link
Member Author

Will amend this PR and update min dep tomorrow if no objections.

@lucyleeow lucyleeow reopened this Jun 20, 2023
@lucyleeow lucyleeow changed the title DOC Update deprecated sphinx global var style in layout.html DOC Update removed sphinx global var style in layout.html Jun 20, 2023
@lucyleeow
Copy link
Member Author

Not sure why circle ci doc-min-dependencies is still using the old sphinx version...?

@lucyleeow
Copy link
Member Author

lucyleeow commented Jun 21, 2023 •
edited

Running update_environments_and_lock_files.py is taking >1h (and still not finished) to solve the environment for doc_min_dependencies, maybe the older versions of sphinx-gallery and sphinx-prompt are making the env difficult to solve?

Will try again at end of the day and leave it running...

@lucyleeow
Copy link
Member Author

Also for this PR, should update_environments_and_lock_files.py be run on all builds or just 'doc_min_dependencies' ?

@adrinjalali
Copy link
Member

I think it's enough to update the circleCI lock files. Also cc @lesteve

@lucyleeow
Copy link
Member Author

Hmm it ran for >3h, and then I got a ConnectionError. Anyone else want to try running it....?

@lesteve
Copy link
Member

lesteve commented Jun 23, 2023

I pushed a commit updating the doc_min_dependencies lock file. This took 20s on my machine (only to generate the doc_min_dependencies one with --select-build doc_min)

Wild guess: maybe you don't have mamba installed?

@lucyleeow
Copy link
Member Author

lucyleeow commented Jun 23, 2023 •
edited

🤦 yes you are right, I didn't have mamba. I thought since the command had --mamba it would tell me if I didn't have it/it was definitely using it.

@lucyleeow
Copy link
Member Author

Anyway, thanks. I think this is ready for review now.

thomasjpfan
thomasjpfan approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the fix. LGTM

@adrinjalali adrinjalali enabled auto-merge (squash) June 23, 2023 11:02
auto-merge was automatically disabled June 23, 2023 11:08

Head branch was pushed to by a user without write access

@lucyleeow
Copy link
Member Author

Forgot to update the doc lock files as well, as suggested above. Done now, sorry.

@thomasjpfan thomasjpfan enabled auto-merge (squash) June 23, 2023 11:09
@thomasjpfan thomasjpfan merged commit 9cbcc1f into scikit-learn:main Jun 23, 2023
26 checks passed
@lucyleeow lucyleeow deleted the doc_style branch June 24, 2023 04:57
@glemaitre glemaitre mentioned this pull request Jun 26, 2023
Merged
@lucyleeow lucyleeow mentioned this pull request Jun 27, 2023
Closed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jun 29, 2023
@lucyleeow @lesteve @jeremiedbb
DOC Update removed sphinx global var style in layout.html (scikit…
ce10f37 
…-learn#26627)

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
jeremiedbb pushed a commit that referenced this pull request Jun 29, 2023
@lucyleeow @lesteve @jeremiedbb
DOC Update removed sphinx global var style in layout.html (#26627)
cf98b65 
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
@lesteve lesteve mentioned this pull request Oct 23, 2023
Closed
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@lucyleeow @lesteve @REDVM
DOC Update removed sphinx global var style in layout.html (scikit…
5170e8b 
…-learn#26627)

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DOC Sphinx global var style removed in Sphinx 7.0.0
4 participants
@lucyleeow @adrinjalali @thomasjpfan @lesteve