Change matplotlib (+ numpy) to optional extra #493

OverkillGuy · 2022-02-10T00:33:02Z

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

To enable matplotlib usage, install via

pip install sphinx-needs[matplotlib]
# equivalent via [all]
pip install sphinx-needs[all]

Add a note about needbar/needpie 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.

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 (see comments)

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

Related to useblocks#222 for matplotlib + depending on PR useblocks#493. Fixes useblocks#482 by adding plantuml as (optional) dependency. Open questions: - What to do about no_plantuml func, see diff - What is minimum plantuml version to allow? (currently latest = 0.22)

OverkillGuy · 2022-10-12T01:16:47Z

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 · 2022-10-12T05:09:17Z

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 · 2023-08-08T19:05:11Z

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

ubmarco

I like the PR, just had one comment. And a rebase would be needed to resolve the conflicts.

docs/installation.rst

Remove need for matplotlib by default (and NumPy, transitively), per useblocks#222. To enable matplotlib usage, install via pip install sphinxcontrib-needs[matplotlib] Add the note about needbar also needing matplotlib So far no change done in docs or import to handle missing features.

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. Fix formatting Appease the pre-commit gods.

As discused in useblocks#222, the potential loss of feature that disabling matplotlib by default is not nice. To avoid that, add a new install extra meta-feature called "all", which will aggregate all extras one might want (currently just matplotlib). This becomes the new recommended way to install, so no loss of feature for those who use it, but allow the lighter option by disabling the all. Update docs accordingly (may be updating it a little strong). New docs entry for matplotlib/numpy support to align with plantuml.

OverkillGuy · 2023-08-08T21:10:46Z

Making a pass at rebasing right now, sending soon.

Previous iteration raised on ANY import, which was the case when importing the module without really using it. Change the warning to only warn in case of USAGE of matplotlib, making it run OK in test project

OverkillGuy · 2023-08-08T21:19:14Z

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...

Still using the on-use error via global var

Function now defined in single location "utils", taking in boolean, avoiding cross-file global issues. Use the newly relocated function in utils too

OverkillGuy · 2023-08-08T22:11:04Z

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

Need guidance, see inline

docs/directives/needbar.rst

docs/directives/needpie.rst

docs/installation.rst

pyproject.toml

Oversight: since moving the function to explicitly take parameter, forgot to pass the arg ALWAYS, some calls weren't updated, whoops

sphinx_needs/utils.py

OverkillGuy · 2023-08-11T21:56:53Z

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.

sigma67 · 2023-08-12T08:35:15Z

README.rst

-    poetry add sphinx-needs
+    poetry add sphinx-needs[all]
+    # Lighter install without matplotlib = no pie charts
+    poetry add sphinxcontrib-needs


Why does it still say sphinxcontrib-needs here? Perhaps a merge error? The comment can be removed as well I think.

Fixed the sphinxcontrib-needs name, but if I remove the comment we have these 2 without explanation?

poetry add sphinx-needs[all] poetry add sphinx-needs

That felt weird so I added that comment. Happy to remove, mostly felt the unexplained [all] would add confusion.

docs/directives/needbar.rst

docs/directives/needpie.rst

sphinx_needs/utils.py

sigma67

Just some minor suggestions. Overall I can confirm that it works as intended.

I ran pip install git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib with the following output:

Output

  Running command git clone --filter=blob:none --quiet https://github.com/OverkillGuy/sphinxcontrib-needs 'C:\Users\Benedikt\AppData\Local\Temp\pip-req-build-9lpgn4iz'
  Running command git checkout -b optional-matplotlib --track origin/optional-matplotlib
  branch 'optional-matplotlib' set up to track 'origin/optional-matplotlib'.
  Switched to a new branch 'optional-matplotlib'
  Resolved https://github.com/OverkillGuy/sphinxcontrib-needs to commit 61a3124e58521ed8201b4991ab93f61344d2982f
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Collecting docutils>=0.15
  Using cached docutils-0.20.1-py3-none-any.whl (572 kB)
Collecting jsonschema>=3.2.0
  Downloading jsonschema-4.19.0-py3-none-any.whl (83 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 83.4/83.4 kB 4.9 MB/s eta 0:00:00
Collecting requests<3.0.0,>=2.25.1
  Using cached requests-2.31.0-py3-none-any.whl (62 kB)
Collecting requests-file<2.0.0,>=1.5.1
  Downloading requests_file-1.5.1-py2.py3-none-any.whl (3.7 kB)
Collecting sphinx>=5.0
  Downloading sphinx-7.1.2-py3-none-any.whl (3.2 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.2/3.2 MB 10.1 MB/s eta 0:00:00
Collecting sphinx-data-viewer<0.2.0,>=0.1.1
  Downloading sphinx-data-viewer-0.1.2.tar.gz (6.6 kB)
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Collecting attrs>=22.2.0
  Using cached attrs-23.1.0-py3-none-any.whl (61 kB)
Collecting jsonschema-specifications>=2023.03.6
  Using cached jsonschema_specifications-2023.7.1-py3-none-any.whl (17 kB)
Collecting referencing>=0.28.4
  Using cached referencing-0.30.2-py3-none-any.whl (25 kB)
Collecting rpds-py>=0.7.1
  Using cached rpds_py-0.9.2-cp311-none-win_amd64.whl (180 kB)
Collecting charset-normalizer<4,>=2
  Using cached charset_normalizer-3.2.0-cp311-cp311-win_amd64.whl (96 kB)
Collecting idna<4,>=2.5
  Using cached idna-3.4-py3-none-any.whl (61 kB)
Collecting urllib3<3,>=1.21.1
  Using cached urllib3-2.0.4-py3-none-any.whl (123 kB)
Collecting certifi>=2017.4.17
  Using cached certifi-2023.7.22-py3-none-any.whl (158 kB)
Collecting six
  Using cached six-1.16.0-py2.py3-none-any.whl (11 kB)
Collecting sphinxcontrib-applehelp
  Downloading sphinxcontrib_applehelp-1.0.6-py3-none-any.whl (120 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 120.0/120.0 kB 7.3 MB/s eta 0:00:00
Collecting sphinxcontrib-devhelp
  Downloading sphinxcontrib_devhelp-1.0.4-py3-none-any.whl (83 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 83.5/83.5 kB 4.6 MB/s eta 0:00:00
Collecting sphinxcontrib-jsmath
  Using cached sphinxcontrib_jsmath-1.0.1-py2.py3-none-any.whl (5.1 kB)
Collecting sphinxcontrib-htmlhelp>=2.0.0
  Downloading sphinxcontrib_htmlhelp-2.0.3-py3-none-any.whl (99 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 99.2/99.2 kB 5.6 MB/s eta 0:00:00
Collecting sphinxcontrib-serializinghtml>=1.1.5
  Downloading sphinxcontrib_serializinghtml-1.1.7-py3-none-any.whl (92 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 92.6/92.6 kB 5.1 MB/s eta 0:00:00
Collecting sphinxcontrib-qthelp
  Downloading sphinxcontrib_qthelp-1.0.5-py3-none-any.whl (89 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 89.4/89.4 kB 4.9 MB/s eta 0:00:00
Collecting Jinja2>=3.0
  Using cached Jinja2-3.1.2-py3-none-any.whl (133 kB)
Collecting Pygments>=2.13
  Downloading Pygments-2.16.1-py3-none-any.whl (1.2 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.2/1.2 MB 8.2 MB/s eta 0:00:00
Collecting snowballstemmer>=2.0
  Using cached snowballstemmer-2.2.0-py2.py3-none-any.whl (93 kB)
Collecting babel>=2.9
  Downloading Babel-2.12.1-py3-none-any.whl (10.1 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 10.1/10.1 MB 11.9 MB/s eta 0:00:00
Collecting alabaster<0.8,>=0.7
  Using cached alabaster-0.7.13-py3-none-any.whl (13 kB)
Collecting imagesize>=1.3
  Using cached imagesize-1.4.1-py2.py3-none-any.whl (8.8 kB)
Collecting packaging>=21.0
  Using cached packaging-23.1-py3-none-any.whl (48 kB)
Collecting colorama>=0.4.5
  Using cached colorama-0.4.6-py2.py3-none-any.whl (25 kB)
Collecting MarkupSafe>=2.0
  Using cached MarkupSafe-2.1.3-cp311-cp311-win_amd64.whl (17 kB)
Building wheels for collected packages: sphinx-needs, sphinx-data-viewer
  Building wheel for sphinx-needs (pyproject.toml) ... done
  Created wheel for sphinx-needs: filename=sphinx_needs-1.2.2-py3-none-any.whl size=2608156 sha256=eeaaac6cf6ded86fd028f16d114e4182dddac99c8c4f5da0d6775a7cf5348e6a
  Stored in directory: C:\Users\Benedikt\AppData\Local\Temp\pip-ephem-wheel-cache-xoehr0xh\wheels\f9\ed\aa\f0427f904af27765cd09bb067776fc0b5f82c01def41effb62
  Building wheel for sphinx-data-viewer (pyproject.toml) ... done
  Created wheel for sphinx-data-viewer: filename=sphinx_data_viewer-0.1.2-py3-none-any.whl size=7990 sha256=b81e006580dc6b314200118d65fa3ea3f8ffffeede098cfc94dff4e9e7dbba04
  Stored in directory: c:\users\benedikt\appdata\local\pip\cache\wheels\34\d6\02\a485dd010dd8c3123cfa0a8bdae75aea24f31d3cc6993bb0e9
Successfully built sphinx-needs sphinx-data-viewer
Installing collected packages: snowballstemmer, urllib3, sphinxcontrib-jsmath, sphinx-data-viewer, six, rpds-py, Pygments, packaging, MarkupSafe, imagesize, idna, docutils, colorama, charset-normalizer, certifi, babel, attrs, alabaster, requests, referencing, Jinja2, requests-file, jsonschema-specifications, jsonschema, sphinxcontrib-serializinghtml, sphinxcontrib-qthelp, sphinxcontrib-htmlhelp, sphinxcontrib-devhelp, sphinxcontrib-applehelp, sphinx, sphinx-needs
Successfully installed Jinja2-3.1.2 MarkupSafe-2.1.3 Pygments-2.16.1 alabaster-0.7.13 attrs-23.1.0 babel-2.12.1 certifi-2023.7.22 charset-normalizer-3.2.0 colorama-0.4.6 docutils-0.20.1 idna-3.4 imagesize-1.4.1 jsonschema-4.19.0 jsonschema-specifications-2023.7.1 packaging-23.1 referencing-0.30.2 requests-2.31.0 requests-file-1.5.1 rpds-py-0.9.2 six-1.16.0 snowballstemmer-2.2.0 sphinx-7.1.2 sphinx-data-viewer-0.1.2 sphinx-needs-1.2.2 sphinxcontrib-applehelp-1.0.6 sphinxcontrib-devhelp-1.0.4 sphinxcontrib-htmlhelp-2.0.3 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-1.0.5 sphinxcontrib-serializinghtml-1.1.7 urllib3-2.0.4

Running pip install "sphinx-needs[all] @ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib" then installs the now-optional dependency matplotlib and its dependencies:

Output

$ pip install "sphinx-needs[all] @ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib"
Collecting sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib
  Cloning https://github.com/OverkillGuy/sphinxcontrib-needs (to revision optional-matplotlib) to c:\users\benedikt\appdata\local\temp\pip-install-bb312k1g\sphinx-needs_4104b84e5c534710845f459d9086a4c9
  Running command git clone --filter=blob:none --quiet https://github.com/OverkillGuy/sphinxcontrib-needs 'C:\Users\Benedikt\AppData\Local\Temp\pip-install-bb312k1g\sphinx-needs_4104b84e5c534710845f459d9086a4c9'
  Running command git checkout -b optional-matplotlib --track origin/optional-matplotlib
  branch 'optional-matplotlib' set up to track 'origin/optional-matplotlib'.
  Switched to a new branch 'optional-matplotlib'
  Resolved https://github.com/OverkillGuy/sphinxcontrib-needs to commit 61a3124e58521ed8201b4991ab93f61344d2982f
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Requirement already satisfied: docutils>=0.15 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.20.1)
Requirement already satisfied: jsonschema>=3.2.0 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (4.19.0)
Collecting matplotlib>=3.3.0
  Downloading matplotlib-3.7.2-cp311-cp311-win_amd64.whl (7.5 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 7.5/7.5 MB 11.4 MB/s eta 0:00:00
Requirement already satisfied: requests<3.0.0,>=2.25.1 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.31.0)
Requirement already satisfied: requests-file<2.0.0,>=1.5.1 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.5.1)
Requirement already satisfied: sphinx>=5.0 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (7.1.2)
Requirement already satisfied: sphinx-data-viewer<0.2.0,>=0.1.1 in c:\users\benedikt\.venv\lib\site-packages (from sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.1.2)
Requirement already satisfied: attrs>=22.2.0 in c:\users\benedikt\.venv\lib\site-packages (from jsonschema>=3.2.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (23.1.0)
Requirement already satisfied: jsonschema-specifications>=2023.03.6 in c:\users\benedikt\.venv\lib\site-packages (from jsonschema>=3.2.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2023.7.1)
Requirement already satisfied: referencing>=0.28.4 in c:\users\benedikt\.venv\lib\site-packages (from jsonschema>=3.2.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.30.2)
Requirement already satisfied: rpds-py>=0.7.1 in c:\users\benedikt\.venv\lib\site-packages (from jsonschema>=3.2.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.9.2)
Collecting contourpy>=1.0.1
  Downloading contourpy-1.1.0-cp311-cp311-win_amd64.whl (470 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 470.9/470.9 kB 9.8 MB/s eta 0:00:00
Collecting cycler>=0.10
  Using cached cycler-0.11.0-py3-none-any.whl (6.4 kB)
Collecting fonttools>=4.22.0
  Downloading fonttools-4.42.0-cp311-cp311-win_amd64.whl (2.1 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 2.1/2.1 MB 10.4 MB/s eta 0:00:00
Collecting kiwisolver>=1.0.1
  Using cached kiwisolver-1.4.4-cp311-cp311-win_amd64.whl (55 kB)
Collecting numpy>=1.20
  Downloading numpy-1.25.2-cp311-cp311-win_amd64.whl (15.5 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 15.5/15.5 MB 13.3 MB/s eta 0:00:00
Requirement already satisfied: packaging>=20.0 in c:\users\benedikt\.venv\lib\site-packages (from matplotlib>=3.3.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (23.1)
Collecting pillow>=6.2.0
  Downloading Pillow-10.0.0-cp311-cp311-win_amd64.whl (2.5 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 2.5/2.5 MB 11.4 MB/s eta 0:00:00
Collecting pyparsing<3.1,>=2.3.1
  Using cached pyparsing-3.0.9-py3-none-any.whl (98 kB)
Collecting python-dateutil>=2.7
  Using cached python_dateutil-2.8.2-py2.py3-none-any.whl (247 kB)
Requirement already satisfied: charset-normalizer<4,>=2 in c:\users\benedikt\.venv\lib\site-packages (from requests<3.0.0,>=2.25.1->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (3.2.0)
Requirement already satisfied: idna<4,>=2.5 in c:\users\benedikt\.venv\lib\site-packages (from requests<3.0.0,>=2.25.1->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (3.4)
Requirement already satisfied: urllib3<3,>=1.21.1 in c:\users\benedikt\.venv\lib\site-packages (from requests<3.0.0,>=2.25.1->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.0.4)
Requirement already satisfied: certifi>=2017.4.17 in c:\users\benedikt\.venv\lib\site-packages (from requests<3.0.0,>=2.25.1->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2023.7.22)
Requirement already satisfied: six in c:\users\benedikt\.venv\lib\site-packages (from requests-file<2.0.0,>=1.5.1->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.16.0)
Requirement already satisfied: sphinxcontrib-applehelp in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.0.6)
Requirement already satisfied: sphinxcontrib-devhelp in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.0.4)
Requirement already satisfied: sphinxcontrib-jsmath in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.0.1)
Requirement already satisfied: sphinxcontrib-htmlhelp>=2.0.0 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.0.3)
Requirement already satisfied: sphinxcontrib-serializinghtml>=1.1.5 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.1.7)
Requirement already satisfied: sphinxcontrib-qthelp in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.0.5)
Requirement already satisfied: Jinja2>=3.0 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (3.1.2)
Requirement already satisfied: Pygments>=2.13 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.16.1)
Requirement already satisfied: snowballstemmer>=2.0 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.2.0)
Requirement already satisfied: babel>=2.9 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.12.1)
Requirement already satisfied: alabaster<0.8,>=0.7 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.7.13)
Requirement already satisfied: imagesize>=1.3 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (1.4.1)
Requirement already satisfied: colorama>=0.4.5 in c:\users\benedikt\.venv\lib\site-packages (from sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (0.4.6)
Requirement already satisfied: MarkupSafe>=2.0 in c:\users\benedikt\.venv\lib\site-packages (from Jinja2>=3.0->sphinx>=5.0->sphinx-needs[all]@ git+https://github.com/OverkillGuy/sphinxcontrib-needs@optional-matplotlib) (2.1.3)
Installing collected packages: python-dateutil, pyparsing, pillow, numpy, kiwisolver, fonttools, cycler, contourpy, matplotlib
Successfully installed contourpy-1.1.0 cycler-0.11.0 fonttools-4.42.0 kiwisolver-1.4.4 matplotlib-3.7.2 numpy-1.25.2 pillow-10.0.0 pyparsing-3.0.9 python-dateutil-2.8.2

Perhaps also revisit the checkboxes in the original PR comment at the top.

OverkillGuy · 2023-08-12T13:16:00Z

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

Looks good from my perspective. Ideally you'd have some test sas well that patch the matplotlib/numpy imports to ImportError, so you can be sure that everything except needpie/needbar works without those packages. Although not strictly necessary I guess. Edit: I see you already have a separate testing repo for that

time for @ubmarco to take a look

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?

I only noticed because it was a changed line, it looks fine now. It's not in scope here to fix all mentions

ubmarco

Hi @OverkillGuy & @sigma67,
thanks for this PR. From technical perspective I have no objection. Works as intended.

I also see this PR as a breaking change, so the version 2.0.0 is a good choice.
Wrt to the sphinx-needs release cycle, I'd like to merge it after the release of 1.3.0.
We want to release some pending smaller features into a release that is non-breaking. 1.3.0 will be released in the next days.
Let's then rebase this PR once more and finally merge it.

I'd sleep better if this PR would include a test case.
The current test cases should fail with this PR as nox does not install extras by default (it just includes them in the pip constraints file). They just don't as in the current noxfile, the docs dependencies are installed as well. And those include matplotlib.
This should be improved so nox separates the environments of docs build and test cases.
Then we can have test cases for needbar/needpie that use a dedicated nox env without the extras. For all other test cases we need to install those extras.
I'm afraid this refactoring burden falls onto this PR as it first introduces dependency splitting.

sigma67 · 2023-08-18T08:38:37Z

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

OverkillGuy · 2023-08-19T14:09:47Z

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 · 2023-08-28T06:47:10Z

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 · 2023-09-16T18:04:57Z

Hi @OverkillGuy , I did you a favor and merged master, see over at: OverkillGuy#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.

Optional matplotlib

sigma67 · 2023-09-17T18:17:17Z

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 · 2023-09-28T08:09:50Z

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

OverkillGuy · 2023-09-28T09:52:55Z

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 · 2023-11-07T07:49:36Z

may be superceded by #1061

OverkillGuy · 2023-11-10T13:06:34Z

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

OverkillGuy mentioned this pull request Feb 13, 2022

Add explicit plantuml dependency as optional #495

Closed

3 tasks

OverkillGuy marked this pull request as ready for review February 22, 2022 01:12

OverkillGuy force-pushed the optional-matplotlib branch from 2a33f0e to 60ed08d Compare October 12, 2022 01:07

ubmarco requested changes Aug 8, 2023

View reviewed changes

docs/installation.rst Outdated Show resolved Hide resolved

Jb DOYON added 4 commits August 8, 2023 22:05

Add checks for optional matplotlib dependency

4a356a1

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. Fix formatting Appease the pre-commit gods.

Update docs for new optional deps for needpie/needbar

1eb4ce2

Jb DOYON added 2 commits August 8, 2023 22:12

Update poetry.lock with new ALL option

4c56291

Refactor matplotlib checks to only raise on USE

3a34611

Previous iteration raised on ANY import, which was the case when importing the module without really using it. Change the warning to only warn in case of USAGE of matplotlib, making it run OK in test project

Jb DOYON added 4 commits August 8, 2023 22:30

Catch new use of Matplotlib in utils

f6f7b06

Still using the on-use error via global var

Move error_on_missing_matplotlib to utils

878dcfc

Function now defined in single location "utils", taking in boolean, avoiding cross-file global issues. Use the newly relocated function in utils too

Fix linter errors

596fdc9

Fix docs header matplotlib/numpy support

e0450fe

OverkillGuy force-pushed the optional-matplotlib branch from 60ed08d to e0450fe Compare August 8, 2023 22:07

OverkillGuy commented Aug 8, 2023

View reviewed changes

docs/directives/needbar.rst Outdated Show resolved Hide resolved

docs/directives/needpie.rst Show resolved Hide resolved

docs/installation.rst Outdated Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

Fix required arg for error_on_missing_matplotlib

bcd661c

Oversight: since moving the function to explicitly take parameter, forgot to pass the arg ALWAYS, some calls weren't updated, whoops

OverkillGuy requested a review from ubmarco August 8, 2023 22:30

OverkillGuy commented Aug 9, 2023

View reviewed changes

sphinx_needs/utils.py Outdated Show resolved Hide resolved

Clarify documentation of matplotlib extra

61a3124

OverkillGuy requested a review from sigma67 August 11, 2023 22:35

sigma67 reviewed Aug 12, 2023

View reviewed changes

docs/directives/needbar.rst Outdated Show resolved Hide resolved

sigma67 reviewed Aug 12, 2023

View reviewed changes

docs/directives/needpie.rst Outdated Show resolved Hide resolved

sigma67 reviewed Aug 12, 2023

View reviewed changes

sphinx_needs/utils.py Outdated Show resolved Hide resolved

sigma67 reviewed Aug 12, 2023

View reviewed changes

Fix review comments

add246a

OverkillGuy requested a review from sigma67 August 12, 2023 13:16

Jb DOYON added 2 commits August 12, 2023 14:48

Update Changelog

c3f750d

Update authors

a671ccb

sigma67 approved these changes Aug 12, 2023

View reviewed changes

Update comment with bar chart

99432fd

ubmarco requested changes Aug 16, 2023

View reviewed changes

sigma67 added 3 commits September 16, 2023 18:57

Merge branch 'master' into optional-matplotlib

7b9205f

update lockfile

6a01674

fix errors after merge

6de5076

Merge pull request #1 from sigma67/optional-matplotlib

6254ecc

Optional matplotlib

chrisjsewell mentioned this pull request Nov 6, 2023

♻️ Make matplotlib dependency optional #1061

Merged

OverkillGuy closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change matplotlib (+ numpy) to optional extra #493

Change matplotlib (+ numpy) to optional extra #493

OverkillGuy commented Feb 10, 2022 •

edited

Loading

OverkillGuy commented Oct 12, 2022

danwos commented Oct 12, 2022

sigma67 commented Aug 8, 2023

ubmarco left a comment

OverkillGuy commented Aug 8, 2023

OverkillGuy commented Aug 8, 2023

OverkillGuy commented Aug 8, 2023

OverkillGuy left a comment

OverkillGuy commented Aug 11, 2023

sigma67 Aug 12, 2023 •

edited

Loading

OverkillGuy Aug 12, 2023

sigma67 left a comment •

edited

Loading

OverkillGuy commented Aug 12, 2023

sigma67 left a comment •

edited

Loading

ubmarco left a comment

sigma67 commented Aug 18, 2023

OverkillGuy commented Aug 19, 2023 •

edited

Loading

chrisjsewell commented Aug 28, 2023

sigma67 commented Sep 16, 2023

sigma67 commented Sep 17, 2023 •

edited

Loading

sigma67 commented Sep 28, 2023

OverkillGuy commented Sep 28, 2023 •

edited

Loading

chrisjsewell commented Nov 7, 2023

OverkillGuy commented Nov 10, 2023

Change matplotlib (+ numpy) to optional extra #493

Change matplotlib (+ numpy) to optional extra #493

Conversation

OverkillGuy commented Feb 10, 2022 • edited Loading

High impact, high priority

Cosmetic, minor

OverkillGuy commented Oct 12, 2022

danwos commented Oct 12, 2022

sigma67 commented Aug 8, 2023

ubmarco left a comment

Choose a reason for hiding this comment

OverkillGuy commented Aug 8, 2023

OverkillGuy commented Aug 8, 2023

OverkillGuy commented Aug 8, 2023

Test procedure

OverkillGuy left a comment

Choose a reason for hiding this comment

OverkillGuy commented Aug 11, 2023

sigma67 Aug 12, 2023 • edited Loading

Choose a reason for hiding this comment

OverkillGuy Aug 12, 2023

Choose a reason for hiding this comment

sigma67 left a comment • edited Loading

Choose a reason for hiding this comment

OverkillGuy commented Aug 12, 2023

sigma67 left a comment • edited Loading

Choose a reason for hiding this comment

ubmarco left a comment

Choose a reason for hiding this comment

sigma67 commented Aug 18, 2023

OverkillGuy commented Aug 19, 2023 • edited Loading

chrisjsewell commented Aug 28, 2023

sigma67 commented Sep 16, 2023

sigma67 commented Sep 17, 2023 • edited Loading

sigma67 commented Sep 28, 2023

OverkillGuy commented Sep 28, 2023 • edited Loading

chrisjsewell commented Nov 7, 2023

OverkillGuy commented Nov 10, 2023

OverkillGuy commented Feb 10, 2022 •

edited

Loading

sigma67 Aug 12, 2023 •

edited

Loading

sigma67 left a comment •

edited

Loading

sigma67 left a comment •

edited

Loading

OverkillGuy commented Aug 19, 2023 •

edited

Loading

sigma67 commented Sep 17, 2023 •

edited

Loading

OverkillGuy commented Sep 28, 2023 •

edited

Loading