Skip to content

DOC Fixes search on webpage #24128

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

Merged
merged 5 commits into from
Aug 9, 2022
Merged

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Aug 5, 2022

Reference Issues/PRs

Fixes #24127

What does this implement/fix? Explain your changes.

There are many changes to Sphinx's javascript and updating our own searchtools.js feel like too much maintenance on us. This PR leverages Sphins'x searchtools.js directly to fix search.

I'll say we backport this into 1.1.X too so that the live site gets fixed.

@thomasjpfan thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Aug 5, 2022
@thomasjpfan
Copy link
Member Author

Using Sphinx's searchtools.js leads to more HTMP calls because it grabs the context for each search result.

The custom searchtools.js on main adjust the behavior by not loading the summary when searching.

@thomasjpfan
Copy link
Member Author

With d4f7363 (#24128), the search summary is disabled, which is consistent with the behavior with the search behavior in the 1.0 docs.

Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM
Timing is a bit unfortunate as 1.1.2 was just shipped. Do we find another release manager for 1.1.3 or shall we defer to 1.2?

@thomasjpfan
Copy link
Member Author

We can cherry-pick this PR into the 1.1.X branch and that should update the docs for https://scikit-learn.org/stable/ without needing to release a new version.

@lesteve
Copy link
Member

lesteve commented Aug 8, 2022

Trying the search in the CircleCI artifact e.g. https://output.circle-artifacts.com/output/job/41a9e69d-f629-4a96-9d64-ccd07f94afaf/artifacts/0/doc/search.html?q=neighbors there seems to be some weird matches that contains some inline HTML e.g.:

image

If that's more than 15 minutes to fix, I am fine with merging as is, as this is a lot better than a completely broken search.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

If that's more than 15 minutes to fix, I am fine with merging as is, as this is a lot better than a completely broken search.

+1. I think we can inspect the reason of this faulty text generation then.

@thomasjpfan
Copy link
Member Author

The incorrect render was a Sphinx bug. I ran the following to update just the doc dependencies:

python build_tools/update_environments_and_lock_files.py --select-build doc

The search page now renders correctly.

@lesteve
Copy link
Member

lesteve commented Aug 9, 2022

Thanks a lot, let's merge this one!

@lesteve lesteve merged commit 87d182d into scikit-learn:main Aug 9, 2022
lesteve pushed a commit to lesteve/scikit-learn that referenced this pull request Aug 9, 2022
lesteve pushed a commit to lesteve/scikit-learn that referenced this pull request Aug 9, 2022
@lesteve
Copy link
Member

lesteve commented Aug 9, 2022

The search now works for the stable version:
https://scikit-learn.org/stable/search.html?q=neighbors

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The documentation search is broken.
4 participants