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

DOC Update website to pydata-sphinx-theme #28084

Closed
24 of 26 tasks
Charlie-XIAO opened this issue Jan 9, 2024 · 24 comments
Closed
24 of 26 tasks

DOC Update website to pydata-sphinx-theme #28084

Charlie-XIAO opened this issue Jan 9, 2024 · 24 comments

Comments

@Charlie-XIAO
Copy link
Contributor

Charlie-XIAO commented Jan 9, 2024

This issue is a continuation of #26809 and aims to migrate the scikit-learn website towards pydata-sphinx-theme. As this is an ambitious goal, this issue is to track the steps of the migration. cc @lucyleeow who guided me to open this issue.

Quick links

TODO before merging into main

  • doc-min-dependencies is bypassed in CI for now and we need to reactivate it before merging into main. Also, we need to make sure all dependencies are documented, in particular sphinx-design which was not added in the very first setup PR. See also #28379.
  • Remove the new_web_theme part in .circleci/config.yml.
  • In conf.py, change the version switcher link to https://scikit-learn.org/dev/_static/versions.json.
  • Remove themes/ and update exclude_patterns in conf.py: these are only useful for the old theme.
  • Pin higher versions of sphinx, pydata-sphinx-theme, and sphinx-gallery. See also tracker for upstream issues.

Tracking work towards the new_web_theme branch

Tracking work related by towards the main branch

@Charlie-XIAO Charlie-XIAO added Documentation Needs Triage Issue requires triage labels Jan 9, 2024
@lucyleeow lucyleeow removed the Needs Triage Issue requires triage label Jan 9, 2024
@Charlie-XIAO

This comment was marked as outdated.

@adrinjalali
Copy link
Member

@Charlie-XIAO I rebased the new_web_theme to the latest main.

This is awesome work, thanks.

@glemaitre
Copy link
Member

This is awesome work, thanks.

Indeed, this is.

Please feel free to ping me whenever you need reviews.

@Charlie-XIAO
Copy link
Contributor Author

@adrinjalali @glemaitre Thanks for your confirmation, and thanks for updating new_web_theme! I've opened #28132 for the very first step.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Jan 16, 2024

Tracking the list of upstream issues in pydata-sphinx-theme, bootstrap, sphinx, etc.

Latest stable release of pydata-sphinx-theme: 0.15.2
Latest stable release of sphinx-design: 0.15.0
Latest stable release of sphinx: 7.2.6


@adrinjalali
Copy link
Member

Now that #28132 (comment) is merged, I think it might not be a bad idea to have that branch pushed somewhere so that we can have a live preview.

@Charlie-XIAO

This comment was marked as outdated.

@Charlie-XIAO

This comment was marked as outdated.

@adrinjalali
Copy link
Member

I was thinking more something alone the lines of https://scikit-learn.org/new_web_theme though. to really test things, and have it updated automatically with every merged PR.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 2, 2024

Ah I see, so essentially I can modify the deploy step of CircleCI to do something when CIRCLE_BRANCH is new_web_theme right? I can open a PR to try. Update: #28347.

@betatim
Copy link
Member

betatim commented Feb 2, 2024

Should we add a label for PRs related to this effort? I'm generally not the biggest fan of labels, but it seems like it could be useful here to filter PRs to just those related to this issue. WDYT?

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 2, 2024

Yea a label may be useful. Maybe it can be automatically added with some workflow since I currently give related PRs titles with a shared prefix?

Or alternatively I'm keeping track of all PRs towards the branch, related PRs towards main, and upstream issues (which you can find or redirect to from the very first comment). So essentially you can easily find everything related from the first comment in this issue, and maintainers can edit if more shortcuts are wanted.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 7, 2024

A question: is it possible to directly add the dependencies (pydata-sphinx-theme, sphinx-remove-toctrees, etc.) in main? I think git is not able to merge the lock files from main correctly into new_web_theme.

@lesteve
Copy link
Member

lesteve commented Feb 7, 2024

A question: is it possible to directly add the dependencies
I think git is not able to merge the lock files from main correctly into new_web_theme.

I think you can keep working in the preview branch. Lock-files are quite likely to have conflicts but you can sort them out like any other conflicts in git, see #28336 (comment)

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 7, 2024

Thanks for the reply @lesteve. If we do not worry about the conflicts then merging main into new_web_theme would indeed solve my problem.

@Charlie-XIAO
Copy link
Contributor Author

The only thing is that every time after merging main into new_web_theme I think the lock files will no longer have the dependencies for pydata-sphinx-theme so we need to manually update the lock files again with a PR, otherwise things will stop working.

@lesteve
Copy link
Member

lesteve commented Feb 7, 2024

Yes there is an additional manual process involved to solve the conflicts, I think this is unavoidable. This would be a bit weird to have additional dependencies in main that we don't use ...

Unless I am missing something, if you want to merge main into new_web_theme this is already a manual process right and you need to open a PR? The additional step is to fix conflicts before opening the PR. There is probably a way to make it less painful by running something like this (not tested so probably need to be adapted):

# I never know --theirs vs --ours and it depends whether you do rebase or merge I think
git checkout --theirs -- build_tools
python build_tools/update_environments_and_lock_files.py --select-build 'doc$'
git a build_tools
# or use rebase if you do rebase
git merge --continue

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 7, 2024

I do not have the right to merge, so I need to leave it to maintainers. So, either every time maintainers do this when merging new_web_theme into main, or I make PRs for this every time maintainers do a merge.

Initially I asked about adding to main because that would be "once and for all" (if I am not mistaken). But yes having things we do not use in main is strange... especially since the doc will get modified as well (though very minor places, by which I mean the table on this page that is written based on _min_dependencies.py).

I am okay will both options, and I think this might not be something I can decide. @adrinjalali who have helped me do the merging job recently.

@lesteve
Copy link
Member

lesteve commented Feb 7, 2024

I think it is fine if new_web_theme branch lags behind main and is updated only from time to time, for example say every week or even less frequently.

I guess the first thing is to get feed-back on how it looks and for this this does not matter if you don't have the doc updates from the last two weeks.

The mechanism to update new_web_theme preview is to do a PR (once it's merged you have to wait a while, ~1 hour, before the doc is built on the branch and the content pushed to .github.io). If you merge main into your PR branch and you have lock-files conflicts, you need to fix the conflicts before you open a PR.

@Charlie-XIAO
Copy link
Contributor Author

Yes you are right and in fact I need the theme branch to be updated only when I directly push some related changes to main.

The mechanism to update new_web_theme preview is to do a PR (once it's merged you have to wait a while, ~1 hour, before the doc is built on the branch and the content pushed to .github.io). If you merge main into your PR branch and you have lock-files conflicts, you need to fix the conflicts before you open a PR.

Yes true. And I believe I shouldn't need to merge main into my PR branch if I'm targeting the theme branch.

So the decision is not to disturb main. I do need to make a PR to fix what's already broken though. Thanks for your careful explanations @lesteve

@glemaitre
Copy link
Member

glemaitre commented May 16, 2024

I merged the latest PR and we should not have any pending PR. Now, I think that we need:

  • Merge main into the new website branch and solve conflict
  • Have a look if we can alleviate some of the remaining issue through the configuration file.

Then, I think that we should be merging the PR in main.

@glemaitre
Copy link
Member

glemaitre commented May 16, 2024

FYI, I create a specific board that could be useful to track any PR issues in other repositories: https://github.com/orgs/scikit-learn/projects/6

@Charlie-XIAO
Copy link
Contributor Author

Nice! I will take a look at the merging tomorrow; I think there is going to be some conflicts in particular regarding the drop-down which git often fails resolve elegantly. AFAIC with the current changes the new website is definitely "usable". We may navigate around once again when I open the PR targeting main. There are indeed styling issues left e.g. admonitions, misused block quotes in APIs, etc. but those can be tackled afterwards.

@glemaitre
Copy link
Member

I'm closing this issue since we did most of the items.
I aggregated the remaining issues (that needs upstream work) in the following board: https://github.com/orgs/scikit-learn/projects/6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

6 participants