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

Sphinx version information in "Building the documentation" section needs reevaluation #27643

Closed
TamaraAtanasoska opened this issue Oct 23, 2023 · 10 comments

Comments

@TamaraAtanasoska
Copy link
Contributor

Describe the issue linked to the documentation

At the bottom of the "Building the documentation" section there is a warning about the best performing sphinx version, which leads to this file, supposedly mirroring the configuration on CircleCI.

In the file, the suggested version is sphinx=6.0.0. However, sphinx=7.0.0 is the minimum necessary to make the current package combination work. If the version is reverted back to sphinx=6.0.0, the following error will appear: sphinx-prompt 1.8.0 requires Sphinx<8.0.0,>=7.0.0, but you have sphinx 6.0.0 which is incompatible.

Suggest a potential alternative/fix

Replace sphinx=6.0.0 with sphinx=7.0.0.

@betatim
Copy link
Member

betatim commented Oct 23, 2023

Thanks for reporting this.

We also have a "minimum versions build" where sphinx 6.0.0 and sphinx-prompt 13 are used together. As there is no version specified for sphinx-prompt I would assume that while solving the various constraints the package manager would select sphinx=6 (because its fixed/selected by the user) and then find the compatible sphinx-prompt version (1.3.0).

Could you explain a bit more how you ended up with sphinx-prompt 1.8.0? Did you select that version? Which tools did you use to set up things (conda, pip, something else)?

@betatim betatim removed the Needs Triage Issue requires triage label Oct 23, 2023
@lesteve
Copy link
Member

lesteve commented Oct 23, 2023

If you are looking to contribute to scikit-learn, the pragmatic solution is to downgrade sphinx-prompt: pip install sphinx=6.0 sphinx-prompt.

I am not sure this is that easy to document because this kind of thing tend to change a lot with time ... we could link to the lock file we use in the CI since it has all the versions which we use for the CI but this is quite verbose.

This is easy to get into such a situation with pip:

# gets sphinx 7.2.6 sphinx-prompt 1.8 (latest releases) 
pip install sphinx sphinx-prompt

Oh wait the doc says I need sphinx 6.0:

pip install sphinx==6.0

At the end of the output you get some kind of warnings that you have incompatible packages but pip still installs them:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
sphinx-prompt 1.8.0 requires Sphinx<8.0.0,>=7.0.0, but you have sphinx 6.0.0 which is incompatible.
Successfully installed sphinx-6.0.0

@lesteve
Copy link
Member

lesteve commented Oct 23, 2023

And by the way we pin sphinx to 6.0 because there was an issue with sphinx search with more recent versions at one point #25504, not sure whether the issue is still here.

Also some issues have been reported with sphinx 7, some of them have been fixed e.g. #26627 but there may be others left ...

@betatim
Copy link
Member

betatim commented Oct 23, 2023

Thanks for the idea Loic about how a user can end up in this situation. Maybe we can improve our documentation guide? It sounds like this might be easier than upgrading to sphinx 7?

@ogrisel
Copy link
Member

ogrisel commented Oct 24, 2023

I think it's worth testing if we can remove the pinning to sphinx 6. That would be more sustainable in the long term.

@TamaraAtanasoska
Copy link
Contributor Author

If you are looking to contribute to scikit-learn, the pragmatic solution is to downgrade sphinx-prompt: pip install sphinx=6.0 sphinx-prompt.

I am not sure this is that easy to document because this kind of thing tend to change a lot with time ... we could link to the lock file we use in the CI since it has all the versions which we use for the CI but this is quite verbose.

This is easy to get into such a situation with pip:

# gets sphinx 7.2.6 sphinx-prompt 1.8 (latest releases) 
pip install sphinx sphinx-prompt

Oh wait the doc says I need sphinx 6.0:

pip install sphinx==6.0

At the end of the output you get some kind of warnings that you have incompatible packages but pip still installs them:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
sphinx-prompt 1.8.0 requires Sphinx<8.0.0,>=7.0.0, but you have sphinx 6.0.0 which is incompatible.
Successfully installed sphinx-6.0.0

I can confirm that it was a similar process. I am working on adding a new example in the documentation, I was reading the building docs and saw the note, changed the version I had. I have since then reverted it to the newer version and I am working without issues on adding the example, but I thought maybe this is worthwhile looking into as it might be confusing for others. Maybe it would be enough just to remove/reprahse that part of the building documentation(the "warning") while this is being worked on, or in the process of eventually updating?

@lesteve
Copy link
Member

lesteve commented Oct 24, 2023

I think it's worth testing if we can remove the pinning to sphinx 6. That would be more sustainable in the long term.

Locally it seems to work fine with the latest Sphinx version. I have opened #27656 to see if there are issues.

@betatim
Copy link
Member

betatim commented Oct 24, 2023

Now that #27656 has been merged the sphinx version isn't pinned anymore. Do you think this is good enough to close this issue or should we make some additional changes to the instructions (especially from the viewpoint of a new contributor)?

@TamaraAtanasoska
Copy link
Contributor Author

Now that #27656 has been merged the sphinx version isn't pinned anymore. Do you think this is good enough to close this issue or should we make some additional changes to the instructions (especially from the viewpoint of a new contributor)?

This looks great to me, and not confusing any more. The issue that I had yesterday has no way happening like this.

@lesteve
Copy link
Member

lesteve commented Oct 24, 2023

OK let's close this one then, thanks for your feed-back @TamaraAtanasoska!

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

No branches or pull requests

4 participants