Refactor to make sphinx a soft dependency

[rossbar]

See #648 for motivation.

This is an attempt at refactoring numpydoc so that the bits that don't depend on sphinx, primarily NumpyDocString, can be used by downstream libraries without pulling in a transitive sphinx dependency.

Fortunately numpydoc was already organized in such a way that this was fairly straightforward: docscrape.py is the important module (where NumpyDocString is defined) and already doesn't depend on sphinx. Therefore I went about trying to remove any remaining transitive dependencies and reorganizing the test suite.

A quick commit-by-commit summary:

• 8a51c0a : moves sphinx from a hard dependency to a separate dependency group that I arbitrarily called default (matching the pattern used by NetworkX). Happy to reorganize this according to whomever's preference!
• fb070f9 : Only attempt to construct the setup function (used in registering the extension with the sphinx-build pipeline) on import if sphinx is installed.
• effcf01 : Split the docscrape tests into two test modules: test_docscrape.py and test_docscrape_sphinx.py. The former primarily captures tests of the NumpyDocString class, whereas the latter covers SphinxDocString and anything else that depends on sphinx. The diff is nasty, but no tests were added/removed - this commit only changes where tests live (and adds pytest.importorskip("sphinx") to the latter).
• a380d08 : some minor cleanup related to effcf01 - import text for test cases rather than re-defining strings
• 4d3b301 : All tests in test_full.py require sphinx, so pytest.importorskip that test module
• 530d580 : AFAICT there was only one private function _clean_text_signature defined in numpydoc.py which didn't depend on sphinx. All other functionality in that module is related to the sphinx extension (docstring mangling, etc.). Therefore, I chose to split this one function out into it's own private module with separate testing. That way I could safely pytest.importorskip test_numpydoc.py without losing any coverage.
• 4bebfdd : The numpydoc test suite runs doctests by default, which causes problems at test collection time when some modules which depend on sphinx are imported. This commit adds a dynamic config for pytest that skips doctest collection for these modules when sphinx is not present. Again, pattern borrowed from NetworkX.
• 8283bd1 : One idea for breaking the single dependence on sphinx that I noticed for the cli/hooks workflows. If we instead use NumpyDocString in render_object instead of SphinxDocString, then the cli is (I think) completely sphinx-free. I'm not sure what implications this has for the cli-based validation workflows - I'm hoping @stefmolin or someone more familiar with this piece can weigh in!

I'm opening as a draft PR to get initial feedback on the idea and review from the cli/hooks folks. I have the following list of todo's before I move from draft->reviwable status:

• Try building scipy with numpydoc's NumpyDocString instead of scipy's vendored version to ensure everything works and scipy's build recipe remains unaffected
• Try with napari: see Numpydoc dependency napari/napari#8322 for details
• Verify scikit-learn 's test suite works as expected with this PR
• Add another CI workflow to run the tests in an environment where sphinx is not installed
• Build the docs of major scientific Python libraries that depend on numpydoc (numpy, scipy, matplotlib, the scikits, networkx) to verify that all doc build recipes are unaffected by this change.

[larsoner]

Thanks for tackling this! Just one quick idea comes to mind for:

effcf01 : Split the docscrape tests into two test modules...

Have you considered splitting while keeping blame? I did this recently in mne-tools/mne-qt-browser#350 (comment) for example and it seems to have worked for preserving blame. But I understand if this doesn't seem worth it here!

Your TODO list looks good to me as well.

[rossbar]

Have you considered splitting while keeping blame?

Thanks for the pointer @larsoner , this is definitely worthwhile ! I will give it a shot

[rossbar]

AFAICT the test failures are not obviously related: the failures seem to be related to path formatting on windows for the validator. @stefmolin @mattgebert any idea what might be going on here?

[stefmolin]

AFAICT the test failures are not obviously related: the failures seem to be related to path formatting on windows for the validator. @stefmolin @mattgebert any idea what might be going on here?

This is definitely related to Windows paths, but it is failing here and not in main, which makes it seem like it is related to these changes (or a dependency change in the action runner).

---

As far as whether this will affect the AST logic and the hook, I haven't tested, but I don't think it will have any effect and making the sphinx dependency is definitely a good idea.

[stefmolin]

I wonder if this is necessary. What about including numpydoc[default] in our test dependencies and then we can test everything?

Alternatively, what about marking the tests that require sphinx as expected fails when sphinx isn't available?

[rossbar]

What about including numpydoc[default] in our test dependencies and then we can test everything?

This would work for CI but wouldn't work out-of-the-box for a development environment. I personally think there's value in being able to run only the tests (including doctests) for the non-sphinxy bits.

[stefmolin]

It might be worth using nox if we want that because now we are talking about needing to use two different virtual environments to run the tests like this.

[rossbar]

I'm -1 on more tooling for testing/environment management. All this change does is introduce importorskip-like behavior for doctests as well to ensure that the "full" test suite (i.e. including doctests) runs the correct tests whether or not sphinx is installed.

[rossbar]

(or a dependency change in the action runner).

Good observation - I opened #804 to test whether it was something in the runner environment - everything there runs as expected so it's likely not that.

This is definitely related to Windows paths, but it is failing here and not in main, which makes it seem like it is related to these changes

Indeed that seems to be the case, but I still can't tell how. After looking at this a bit, there are two things I'm confused by:

1. How did these changes affect these test results (having re-looked at everything there doesn't seem to be any direct connection to the changes and the hooks/test_ tests).
2. How did these tests ever pass on windows in the first place? The problems that were tripping it up do indeed seem to be entirely related to windows path rendering... why wasn't this a problem before? FWIW 7708ca0 "fixes" the issue by robust-ifying the tests themselves to different platforms. That doesn't answer either of these two questions though.

Normally I'd address these questions by bisect-ing to ID which specific commit caused the issue, but I don't have ready access to a windows machine.

[stefmolin]

I'm also at a loss for why these changes would cause the tests to suddenly fail on Windows. The only thing I can think of is that the proposed changes don't touch our GitHub Actions, meaning that we are no longer testing any of the sphinx behavior – it's not installed. Maybe sphinx comes with something that changes how the paths behave on Windows? I don't have access to a Windows machine either.

[rossbar]

I'm also at a loss for why these changes would cause the tests to suddenly fail on Windows. The only thing I can think of is that the proposed changes don't touch our GitHub Actions, meaning that we are no longer testing any of the sphinx behavior – it's not installed. Maybe sphinx comes with something that changes how the paths behave on Windows? I don't have access to a Windows machine either.

I believe I've figured it out: the issue is the test/hooks test were never being run in CI. See #653 , specifically e728f61 and the associated action logs, for proof. Basically the tests are run with the --pyargs flag which runs tests from the installed package, but the tests/hooks tests were not included in the package, so they were never run.

I suspect the reason they started being run with this PR was the introduction of conftest.py, which IIRC can subtly influence test collection¹.

Anyways - I will port the test fixes in 7708ca0 over to #653 to fix all of that separately so we can remain focused on the sphinx-dep changes in this PR.

Footnotes

1. I'm not 100% sure here, but that's where I'd start investigating if I really wanted to chase down exactly what's happening. ↩

[stefmolin]

What about including the reason for all of these calls?

[mattgebert]

@rossbar Sorry I haven't had time recently to get on top of this, and am just having a quick look at this now!

I noticed the hook tests never being called (see #622 (comment) ) a while ago when I was writing #622

I did a few things to rectify this, not sure if this is the same as what you've now done in #653. You can check the file changes.

• I had to add an explicit call in the CI for the numpydoc/tests/hooks directory to ensure it runs.
• The other thing in the hook tests is the use of delimeters that don't match the Windows filepath. Added a os.replace("/", os.sep)

#622 is also now ready to integrate btw since we move to Python 3.10+.