Skip to content

[MRG] Patches sphinx.ext.autosummary for case insensitive file systems#13022

Merged
adrinjalali merged 6 commits intoscikit-learn:masterfrom
thomasjpfan:sphinx_case_insensitive_v2
Jun 25, 2020
Merged

[MRG] Patches sphinx.ext.autosummary for case insensitive file systems#13022
adrinjalali merged 6 commits intoscikit-learn:masterfrom
thomasjpfan:sphinx_case_insensitive_v2

Conversation

@thomasjpfan
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan commented Jan 21, 2019 •
edited
Loading

Reference Issues/PRs

This is an alternative solution to issue #12712. This PR or PR #12968 can address the issue.

What does this implement/fix? Explain your changes.

This PR adds configuration options to change the suffix:

 custom_autosummary_names_with_new_suffix = {
    'sklearn.cluster.dbscan',
    'sklearn.cluster.optics',
    'sklearn.covariance.oas',
    'sklearn.decomposition.fastica'
}
custom_autosummary_new_suffix = '-lowercase.rst'
custom_autosummary_generated_dirname = os.path.join('modules', 'generated')

This PR monkeypatches os.path.join used by sphinx.ext.autosummary.generate to return a filename with a new suffix.

Any other comments?

This PR contains less logic and gives the control to the user, thus a little simpler when compared to #12968.

@thomasjpfan thomasjpfan mentioned this pull request Jan 21, 2019
Closed
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jan 21, 2019

The difference being that the other pr automatically identifies collisions? Yes, this is simpler to read

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Jan 21, 2019

Yes the other PR is automatic, while this one requires a user to add a function to custom_autosummary_names_with_new_suffix if there is a conflict. Since both solutions monkey patches, I think a simpler to read version is better.

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Jul 18, 2019

There is an issue in sphinx regarding this issue: sphinx-doc/sphinx#1495

rth
rth reviewed Oct 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

It's a nice fix, but honestly I do wonder if we want to maintain this. The only affected users are maintainers on Mac OS as far as I can tell, and the solution is not that trivial. Proposing it in sphinx would be ideal.

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Oct 4, 2019

Yea, I wish this was less hacky. The logic of "check if file exist by not using the file system and renaming the file to something" is kind of weird to configure (from a sphinx user point of view)

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Apr 27, 2020

This is ancient, but if it's not been fixed upstream in Sphinx (we should check), we should consider merging it before trying to create a zipped-HTML release of the docs.

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Apr 27, 2020

We are still using this for update the docs for dash-docs.

Last time I looked at this, autosummary was using the filenames as the "source of truth" and this only works for case sensitive file systems.

From the original issue: sphinx-doc/sphinx#1495 it looks like this issue has been pushed up to "Some future version" milestone. (Sounds familiar eh?)

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Apr 27, 2020 •
edited
Loading

I have a feeling this would introduce new sphinx warnings and fail the CI (now that it catches sphinx warnings).

Edit - Oh wow it worked

@thomasjpfan thomasjpfan mentioned this pull request Jun 10, 2020
Closed
@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented Jun 25, 2020

Let's merge this one so that we can proceed on #17051?

@adrinjalali adrinjalali merged commit 8a695d7 into scikit-learn:master Jun 25, 2020
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jun 25, 2020

This has broken the doc build:

Traceback (most recent call last):
  File "/home/circleci/miniconda/envs/testenv/lib/python3.8/site-packages/sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File "/home/circleci/miniconda/envs/testenv/lib/python3.8/site-packages/sphinx/application.py", line 244, in __init__
    self.setup_extension(extension)
  File "/home/circleci/miniconda/envs/testenv/lib/python3.8/site-packages/sphinx/application.py", line 398, in setup_extension
    self.registry.load_extension(self, extname)
  File "/home/circleci/miniconda/envs/testenv/lib/python3.8/site-packages/sphinx/registry.py", line 414, in load_extension
    metadata = setup(app)
  File "/home/circleci/project/doc/sphinxext/custom_autosummary_new_suffix.py", line 76, in setup
    for listener_id, obj in builder_inited_listeners.items():
AttributeError: 'list' object has no attribute 'items'

@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented Jun 25, 2020

should I revert it or are you gonna do it?

This was referenced Jun 25, 2020
Merged
Merged
@joshuacwnewton joshuacwnewton mentioned this pull request Jun 26, 2020
Closed
@thomasjpfan thomasjpfan mentioned this pull request Jun 29, 2020
Closed
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Jun 29, 2020
@thomasjpfan
Revert "ENH Patches sphinx.ext.autosummary for case insensitive file …
149b22b 
…systems (scikit-learn#13022)"

This reverts commit 8a695d7.
@jnothman jnothman mentioned this pull request Jun 30, 2020
Closed
rth pushed a commit that referenced this pull request Jul 1, 2020
@thomasjpfan
Revert "ENH Patches sphinx.ext.autosummary for case insensitive file …
903c82e 
…systems (#13022)" (#17780)

This reverts commit 8a695d7.
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
@thomasjpfan @glemaitre
ENH Patches sphinx.ext.autosummary for case insensitive file systems (s…
0c57a83 
…cikit-learn#13022)

* ENH: Patches autosummary for case insensitive file systems

* DOC: More details

* REV
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
@thomasjpfan @glemaitre
Revert "ENH Patches sphinx.ext.autosummary for case insensitive file …
7a53c76 
…systems (scikit-learn#13022)" (scikit-learn#17780)

This reverts commit 8a695d7.
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@thomasjpfan @jayzed82
ENH Patches sphinx.ext.autosummary for case insensitive file systems (s…
6808bb3 
…cikit-learn#13022)

* ENH: Patches autosummary for case insensitive file systems

* DOC: More details

* REV
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@thomasjpfan @jayzed82
Revert "ENH Patches sphinx.ext.autosummary for case insensitive file …
12da2c2 
…systems (scikit-learn#13022)" (scikit-learn#17780)

This reverts commit 8a695d7.
@bsipocz bsipocz mentioned this pull request Jan 27, 2023
Open
@bsipocz
Copy link
Copy Markdown
Contributor

bsipocz commented Jan 27, 2023

@thomasjpfan - I was so happy to see you have a solution for this problem here, and sad to see it needed to be reverted.
Did you come up with something else instead?

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Jan 29, 2023

@bsipocz The fix was added to sphinx directly in sphinx-doc/sphinx#7927 with the autosummary_filename_map feature. Scikit-learn makes uses the feature as follows:

scikit-learn/doc/conf.py

Lines 583 to 589 in 4db0492

# maps functions with a class name that is indistinguishable when case is
# ignore to another filename
autosummary_filename_map = {
"sklearn.cluster.dbscan": "dbscan-function",
"sklearn.covariance.oas": "oas-function",
"sklearn.decomposition.fastica": "fastica-function",
}

@HealthyPear HealthyPear mentioned this pull request Mar 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

+1 more reviewer

@rth rth rth left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thomasjpfan @jnothman @adrinjalali @bsipocz @rth