Skip to content

MNT Update doc lock files to get Sphinx-Gallery 0.19 minigallery bugfix #30822

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
Feb 14, 2025

Conversation

lucyleeow
Copy link
Member

@lucyleeow lucyleeow commented Feb 13, 2025

Reference Issues/PRs

Bump min Update doc conda lock file for newest sphinx-gallery version which fixes minigallery duplication bug: sphinx-gallery/sphinx-gallery#1430 sphinx-gallery/sphinx-gallery#1435

What does this implement/fix? Explain your changes.

Any other comments?

cc @ogrisel @glemaitre

Copy link

github-actions bot commented Feb 13, 2025

✔️ Linting Passed

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

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

@lucyleeow lucyleeow changed the title MNT Bump Sphinx-Gallery version to fix minigallery duplication bug MNT Bump min Sphinx-Gallery version to fix minigallery duplication bug Feb 13, 2025
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Please push an empty commit with the [doc build] commit flag.

Otherwise LGTM. Thanks @lucyleeow!

@glemaitre
Copy link
Member

glemaitre commented Feb 13, 2025

There is not a lock file associated with the yaml one? In other words, we should need to regenerate the lock files isn't it?

@lesteve
Copy link
Member

lesteve commented Feb 13, 2025

There is not a lock file associated with the yaml one? In other words, we should need to regenerate the lock files isn't it?

Yes we would need to regenerate the lock-file and push a [doc build]. Something like this should do it (I thought there was some quick doc for it but I can't find it right now):

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

@lucyleeow
Copy link
Member Author

lucyleeow commented Feb 13, 2025

Yes but I was not 100% on the change so didn't update the lock files.

I checked and 0.17 had the mini gallery duplication bug so it's probably best to bump the min version.

I'll update the lock files

@lesteve
Copy link
Member

lesteve commented Feb 13, 2025

To be honest what we care about is that the doc build has sphinx-gallery 0.19 so that we don't have duplicated examples in the API page on the scikit-learn.org website. We don't really care about bumping the min dependencies of sphinx-gallery but we can also bump it I don't have a strong opinion on this. The doc-min-depencies is mostly used to make sure that the examples run fine with old numpy, scipy, matplotlib, etc ... versions.

@lucyleeow
Copy link
Member Author

The doc-min-depencies is mostly used to make sure that the examples run fine with old numpy, scipy, matplotlib, etc ... versions.

Yes this was why I was hesitant about updating the min dep. Happy to close ?

@lesteve
Copy link
Member

lesteve commented Feb 13, 2025

Yes this was why I was hesitant about updating the min dep. Happy to close ?

It would still be useful to update the doc build lock-file and make sure that the issue has been fixed so something like this + a [doc build] commit:

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

For further reference, PartialDependencyDisplay is one of the page that currently shows a duplication
image

@lesteve
Copy link
Member

lesteve commented Feb 13, 2025

No urgency at all, but since we install sphinx-gallery from conda-forge, we would also need that sphinx-gallery 0.19 is on conda-forge which doesn't seem to be the case yet.

Probably we need to wait a bit before the conda-forge automation creates a PR and then the PR needs to be merged, and then wait a bit more for it to be actually available.

@lucyleeow
Copy link
Member Author

No urgency at all, but since we install sphinx-gallery from conda-forge, we would also need that sphinx-gallery 0.19

oh yeah! Some other kind soul created that

Looking at it more, I think it makes sense to bump the version in pyproject.toml and _min_dependencies.py as these may used by people to install dependencies/determine what version is required to build docs. BUT if we update _min_dependencies.py we should probably update build_tools/circle/doc_min_dependencies_environment.yml to match? WDYT?

@lesteve
Copy link
Member

lesteve commented Feb 13, 2025

In general, no strong opinion on bumping the min dependency of sphinx-gallery. I would slightly lean towards keeping it to whatever we have since this is not a huge bug and it went unnoticed in scikit-learn for quite some time. You can still build fine with older versions and it's not like sphinx-gallery min dependency is forced on anyone, it's more which range of versions we decide to test in our CI.

BUT if we update _min_dependencies.py we should probably update build_tools/circle/doc_min_dependencies_environment.yml to match? WDYT?

This is done by the update_environment_and_lock_files.py script 😉. Basically you udapte _min_dependencies.py and update_environment_and_lock_files.py and then you update environment + lock-files by running update_environment_and_lock_files.py. The best doc is probably the docstring in the script itself actually.

IIRC pyproject.toml update is manual but there is a test that fails somewhere if it does not match _min_dependencies.py. Also I don't think many people actually rely on pip install scikit-learn[docs] or all the other variations for tests, examples, benchmarks but I could well be wrong about that ...

@lucyleeow
Copy link
Member Author

Okay I will update this PR to just update the lock files and not bump min deps.

This is done by the update_environment_and_lock_files.py script 😉.

🤦 ah I knew that but it just didn't click that was a .yml file. I will blame working too late at night, trying to catch you Europeans in your morning 😅

@lucyleeow lucyleeow changed the title MNT Bump min Sphinx-Gallery version to fix minigallery duplication bug MNT Update conda lock files for doc to update Sphinx-Gallery Feb 14, 2025
@lucyleeow
Copy link
Member Author

lucyleeow commented Feb 14, 2025

Doc build failing due to not being able to download dataset in example:

    ../examples/miscellaneous/plot_display_object_visualization.py failed leaving traceback:

    Traceback (most recent call last):
      File "/home/circleci/project/examples/miscellaneous/plot_display_object_visualization.py", line 35, in <module>
        X, y = fetch_openml(data_id=1464, return_X_y=True)
      File "/home/circleci/project/sklearn/utils/_param_validation.py", line 218, in wrapper
        return func(*args, **kwargs)
      File "/home/circleci/project/sklearn/datasets/_openml.py", line 1028, in fetch_openml
        data_description = _get_data_description_by_id(data_id, data_home)
      File "/home/circleci/project/sklearn/datasets/_openml.py", line 363, in _get_data_description_by_id
        json_data = _get_json_content_from_openml_api(
      File "/home/circleci/project/sklearn/datasets/_openml.py", line 254, in _get_json_content_from_openml_api
        raise OpenMLError(error_message)
    sklearn.datasets._openml.OpenMLError: Dataset with data_id 1464 not found.

trying again (in case this is transient) ...

@lesteve
Copy link
Member

lesteve commented Feb 14, 2025

The duplication seems indeed to have been fixed, see the PartialDependencyDisplay rendered page.

I will blame working too late at night, trying to catch you Europeans in your morning 😅

😴 😉

@lesteve lesteve changed the title MNT Update conda lock files for doc to update Sphinx-Gallery MNT Update doc lock files to get Sphinx-Gallery 0.19 minigallery bugfix Feb 14, 2025
@lesteve lesteve merged commit 5c95ebe into scikit-learn:main Feb 14, 2025
34 checks passed
@lucyleeow lucyleeow deleted the bump_sg branch February 14, 2025 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants