Change matplotlib (+ numpy) to optional extra

[OverkillGuy]

Remove need for matplotlib by default (and NumPy, transitively), per #222.

To enable matplotlib usage, install via

pip install sphinxcontrib-needs[matplotlib]

Add a note about needbar also needing matplotlib in pyproject.toml (not visible till now)

Needbar and needpie directives both need matplotlib, but could fail imports if the optional dependency is not installed. Accomodate failure by raising a specific and useful error message via ImportError

Directive-specific docs updated with versionchanged warning about that dependency now being optional.

---

To Do list from contributing guidelines

This isn't 100% ready to go, I just did the easy part, the code changes, needs work to integrate PR without breaking, hence Draft PR.

High impact, high priority

• [ ] Evaluate impact of feature-gating on test suite (disable/run tests differently? tests for optional import?)
• [ ] Check with maintainers what level of documentation would be good enough to announce this big (breaking) change
• [ ] Set up local dev environment to run tests in (I did all this "blind" without running it, naughty)

Cosmetic, minor

• [ ] Confirm the optional/extra lib name "matplotlib" is OK (I considered needpie for feature that needed it, but then discovered needbar also uses it, and more may use it in future)
• [ ] Confirm with maintainers the importerror message is good enough
• [ ] Confirm with maintainers the directive docs entry is good enough
• [ ] Check with maintainers what to do about implicit (transitive) dependency on NumPy (see comment in needbar.py)
• [ ] Update the nextversion message in directives for versionchanged block
• [ ] Add docs/changelog.rst entry
• [ ] Add myself to Authors

[OverkillGuy]

Revisited this: Rebased the whole thing (without plantuml side), and tried to use it as a future user would.

Created https://github.com/OverkillGuy/sphinx-needs-test, a sample repo from my personal cookiecutter (soon to be published), which first adds just sphinx-needs WITHOUT matplotlib, from my rebased PR. I demonstrate in README how installing sphinx-needs as-is doesn't pull matplotlib/numpy but still builds a doc with requirements. That's tagged v0.2.0 on main branch.

Then, in the use-matplotlib branch of the repo, I first (bd1acb8) add a needpie, which fails building (no matplotlib but we're attempting to use it), and in the next commit (95d103f) I use the matplotlib optional, and buildign docs works again.

Having not done this usage test before, I spotted that merely importing needbar.py and needpie.py would crash the whole app, as the process_needbar function etc are used in any case. Adapted the checks accordingly: At import time, failure to import now sets a global MATPLOTLIB_AVAILABLE=False, and a new function error_on_missing_matplotlib(), injected at key matplotlib usage locations, raises as expected when MATPLOTLIB_AVAILABLE is False.

Overall I feel like this (finally) demonstrates that this PR is mature enough on the code side to at least be usable. I'd like to bring the PR into more thorough review.

[danwos]

Thanks for the changes. I agree that this PR should be merged soon.

We discussed also internally, if some functions should be moved to an external package, like sphinx-needs-analyzis or so. But no decision here.

So I think this feature can be merged. Will do a review next week, as I have no possibility to do it right now.

[sigma67]

Anything that's blocking this? Willing to help if needed.

[OverkillGuy]

Making a pass at rebasing right now, sending soon.

[OverkillGuy]

Rebased locally, tests all red for now: Looks like a couple core places in the codebase are importing matplotlib since I made the PR.

Investigating, hopefully this can be cordonned off as before, nothing major...

[OverkillGuy]

Right, that's me rebased, errors fixed (had to shuffle about the error message a bit), and tested, see procedure below.

Ready for review!

Test procedure

Using https://github.com/OverkillGuy/sphinx-needs-test. on branch= main (version 0.2.0) DOES NOT use matplotlib, observe the make install docs works Then check out commit bd1acb8 which ADDS needbar use, causing crash like so:

And check out 95d103f aka use-matplotlib branch, which adds the extras= ["matplotlib"], restoring the make docs feature.

[OverkillGuy]

Applied all comments, ready for rereview. Took the assumption that this is a breaking change = 2.0 is the "versionchanged" when the directives were optional.

[OverkillGuy]

Good catch on last mention of sphinxcontrib-needs in PR.

However grepping for the old name reveals a LOT of links pointing to previous github repo, previous docs URLs etc. Do we care?

[sigma67]

Hi @OverkillGuy, are you planning to tackle this or do you need help?

[OverkillGuy]

Hey, I tried to add tests but I'm bumping against the existing layout that doesn't make much sense to me. Definitely need help.

Points I notice:

• noxfile's run_tests target tries to make test but still does pip install -r docs/requirements.txt, which just doesn't look like a necessary step (still fails many tests if I don't).
• same run_tests target also brings in manually different versions of sphinx + docutils, which again sound like should be wrapped in a func to abtract the docs/requirements.txt + these.
• The docs/requirements.txt import matplotlib explicitly, despite these being required by no obvious docs target, instead needed in the main app's main deps (and now optional)

So, I need input on what to do:

• It feels right to remove matplotlib from docs/requirements.txt (but it breaks EVERYTHING)
• It feels right to remove all this docs/requirements.txt from the run_tests bit (but it breaks even more things)
• I would like to add a test on the normal run_test target that tries to import needbar/needpie, get an importerror and call that a PASS
• I would like to add a new noxfile target that says "install main deps + matplotlib extra" and run another kind of test that tries to import needbar/needpie and SUCCEED.
• What do I do about the existing needpie tests?

Any advice on that?

[chrisjsewell]

Heya @OverkillGuy as you can see I'm making quite a few changes at the moment 😅 but I'll circle back round and have a look at this PR soon cheers

But yeh I agree the poetry/nox setup etc can be a bit of a pain lol

[sigma67]

Hi @OverkillGuy , I did you a favor and merged master, see over at: https://github.com/OverkillGuy/sphinxcontrib-needs/pull/1

Regarding your questions I have some suggestions:

Conditional test execution with matplotlib installed: use a pytest marker pytest.mark.matplotlib on the relevant tests. The following should be marked from my tests (fail with an ImportError):

• test_complex_builders.py::test_doc_complex_latex
• test_complex_builders.py::test_doc_complex_singlehtml
• test_needpie.py::test_doc_build_html
• test_needpie.py::test_sphinx_api_needpie
• test_needs_filter_data.py::test_doc_needs_filter_code
• test_needpie_with_zero_needs.py::test_needpie_with_zero_needs
• test_needs_filter_data.py::test_doc_needs_filter_data_html

Configure nox to do two separate testruns (with nox.parametrize):

• install matplotlib, run pytest
• don't install matplotlib, run pytest -m "not matplotlib"

I am not sure about the need for doc/requirements.txt for the tests, perhaps @ubmarco or @chrisjsewell can shed some light. This step should probably be skipped for the no-matplotlib testrun. Alternatively you can replace the call with

pip install $(grep -ivE "matplotlib" docs/requirements.txt)

to install everything in that file except matplotlib.

Don't hesitate to ask in case of more questions.

[sigma67]

Here's another idea if you want to be more specific. Instead of wrapping the entire function function with a mark and skipping it you can do this to ensure that an ImportError is raised on a specific call.

import importlib
from contextlib import nullcontext

import pytest


@pytest.fixture(scope="session", name="raises_on_missing_matplotlib")
def fixture_raises_on_missing_matplotlib():
    try:
        importlib.import_module("matplotlib")
        return nullcontext()
    except ImportError as err:
        return pytest.raises(ImportError)


def test_something(raises_on_missing_matplotlib):
  with raises_on_missing_matplotlib:
     print()

You'd still have to do the parametrize part to run with different environments, but you no longer need to modify the pytest call.

[sigma67]

It seems like we're not making any progress here. Shall I submit a draft with the proposed fixture?

[OverkillGuy]

It seems like we're not making any progress here. Shall I submit a draft with the proposed fixture?

Sorry, things have come up, compounded with lack of vision on my side for the nox/testing, I'd appreciate a submission to get me going in the right direction yes!

Apologies to make this a burden, I hoped to deliver this turnkey

[chrisjsewell]

may be superceded by #1061

[OverkillGuy]

#1061 was merged, doing same. Thanks for all of your patience!