MAINT: refine installation of core and test dependencies #131

Sheila-nk · 2024-02-28T07:50:21Z

Closes #76

Sheila-nk · 2024-02-28T08:34:57Z

After removing the installation of optional dependencies: scipy and matplotlib, self-tests are failing since we require both libraries to execute some of the doctests.
They are technically optional, but we do need them for our testing purposes.

ev-br

Looks slick!
Not sure I follow though.
The goal of gh-76 is that self-tests pass in an environment without scipy or MPL. Tests which depend on either of them just need to be conditionally skipped, there is some usage of HAVE_SCIPY variable in test_testmod IIRC.

.github/workflows/pip.yml

pyproject.toml

Sheila-nk · 2024-02-28T17:28:29Z

The goal of #76 is that self-tests pass in an environment without scipy or MPL. Tests which depend on either of them just need to be conditionally skipped, there is some usage of HAVE_SCIPY variable in test_testmod IIRC.

Oh! Okay, I was not sure how exactly to go about skipping them without using the --ignore cli option. Let me investigate why they're failing and try this approach instead.

ev-br

Could you also add a second workflow (a single pytest/python version would be enough) , with no scipy and no MPL, too?

A minor inline question is optional, best acted upon in a separate PR if at all.

ev-br · 2024-03-05T07:57:47Z

scpdt/tests/test_pytest_configuration.py

@@ -1,15 +1,29 @@
 import pytest
-from pathlib import PosixPath, Path
+from pathlib import PosixPath


Minor, probably unrelated: any specific reason to use PosixPath not Path? Have to admit I don't know what the consequences are

Thanks for bringing this up! You're right - using Path instead of PosixPath makes the code more platform-independent, which is definitely a better approach.
I did a local test with Path and didn't notice any issues that may have informed my decision, so I'll go ahead and make that change while adding the second workflow.

Sheila-nk · 2024-03-05T10:13:50Z

@ev-br might need a bit of clarification
The second flow is for self-tests only?

A minor inline question is optional, best acted upon in a separate PR if at all.

What do you mean by this?

ev-br · 2024-03-05T10:26:56Z

The second flow is for self-tests only?

Yup. Looking at https://github.com/ev-br/scpdt/blob/main/.github/workflows/pip.yml, two "Run testmod a scipy submodule" explicitly need scipy, the others do not?

The PosixPath vs Path change --- meant to mean this can be left out of this PR or added in, whichever is simpler.

Sheila-nk · 2024-03-06T09:23:47Z

Hey @ev-br, maybe you could review the new workflow before I make edits to the initial one? I've included the two tests from the initial workflow that do not require Scipy and MPL.

I also noticed that we're using older versions of the setup actions. If there's no specific reason for this, I can update them. Additionally, we could set up Dependabot to automatically create PRs when updates are available. Let me know what you think!

ev-br · 2024-03-06T10:31:21Z

TBH, I'm not sure what's going on with the GH workflows. On main, we have one which adds everything, including optionals. What we need is a second one which does not install MPL/SciPy and finishes with clear signal that some tests are skipped and the rest are OK. Why does the first one need tweaking and is the second one have optionals?

Side note: you might want to remove -s from pytest invocation in the new one so that it's less noisy.

Re dependabot: I'd rather not touch it with anything shorter than a bargepole.

Sheila-nk · 2024-03-06T13:19:59Z

On main, we have one which adds everything, including optionals. What we need is a second one which does not install MPL/SciPy and finishes with clear signal that some tests are skipped.

I honestly don't think we need a second workflow.
In the initial workflow, pip install -e . here installs only the core dependencies i.e pytest and numpy.
And the first test jobs are run without Scipy and MPL. The tests which require either of the optional dependencies are skipped.
Then there is pip install -e '.[test]' here which installs scipy and matplotlib. And the rest of the tests carry on without skipping.

Why does the first one need tweaking and is the second one have optional?

No, the second one does not install optional dependencies. If we go ahead with the two-workflows approach, the two self-tests will need to be removed from the first workflow since they are being run in the same way as in the second workflow, except with a single pytest/python version

Sheila-nk · 2024-03-06T13:23:30Z

I have noticed that the 3rd test run is not running tests: Test testfile CLI after installation of the test dependencies.
Will need to look into that.

ev-br · 2024-03-06T16:45:31Z

Ah, I did not realize how CI organized here! Please add some comments in the yml then.
What we also want to make sure though, we want full self-tests pytest --pyargs scpdt run both with optionals and without them. Whether it's a single workflow or two, up to you.

Sheila-nk · 2024-03-07T12:13:16Z

There's this error locally and on GH: https://github.com/ev-br/scpdt/actions/runs/8186212193/job/22384153598?pr=131#step:8:149
I'm unfamiliar with the matplotlib library, so maybe you can help? @ev-br
The internet isn't much help and debugging hasn't provided any enlightenment.

ev-br · 2024-03-07T15:28:50Z

Hm. I do see something similar locally. However I don't think it's related to matplotlib:

$ pytest --pyargs scpdt

reports

FAILED scpdt/tests/test_skipmarkers.py::TestSyntaxErrors::test_invalid_python - AssertionError: assert <class 'AttributeError'> is TypeError
FAILED scpdt/tests/test_skipmarkers.py::TestStopwords::test_bogus_output - doctest.UnexpectedException: <DocTest stopwords_bogus_output from none:0 (1 example)>
FAILED scpdt/tests/test_testmod.py::test_stopwords - assert 1 == 0

But rerunning individual files does not:

$ pytest scpdt/tests/test_skipmarkers.py::TestStopwords::test_bogus_output
===================================================== test session starts =====================================================
platform linux -- Python 3.10.0, pytest-8.0.2, pluggy-1.4.0 -- /home/br/mambaforge/envs/scpdt2/bin/python3.10
cachedir: .pytest_cache
rootdir: /home/br/sweethome/proj/scipy/refguide_check_2/scpdt
configfile: pyproject.toml
plugins: scpdt-0.1
collected 1 item                                                                                                              

scpdt/tests/test_skipmarkers.py::TestStopwords::test_bogus_output PASSED                                                [100%]

====================================================== 1 passed in 0.44s ======================================================

So it's not matplotlib, it's us unfortunately. Let me try bisecting it.

ev-br · 2024-03-07T18:02:10Z

$ git bisect bad
aa5e4878cf099d436c1b5895f79da0d66eb17f30 is the first bad commit
commit aa5e4878cf099d436c1b5895f79da0d66eb17f30
Author: Sheila-nk <[email protected]>
Date:   Tue Mar 5 10:43:21 2024 +0300

    skip self tests that require scipy or matplotlib

 .github/workflows/pip.yml                |  5 ++++-
 scpdt/tests/test_pytest_configuration.py | 20 ++++++++++++++++++--
 scpdt/tests/test_skipmarkers.py          |  8 ++++++++
 scpdt/tests/test_testmod.py              | 12 +++++++++++-
 4 files changed, 41 insertions(+), 4 deletions(-)

The good/bad criterion is $ pytest --pyargs scpdt -x fails or passes.

Sheila-nk · 2024-03-08T14:56:21Z

Decided to start with a new branch from main and gradually add changes from this branch.
Everything works fine until I try to import matplotlib:

try:
    import matplotlib    # noqa
    HAVE_MATPLOTLIB = True
except Exception:
    HAVE_MATPLOTLIB = False

I can skip tests when matplotlib is not installed. But once I install it, the tests fail.
And when I remove the import, they pass. 😆
Trying to see what that is about. Any ideas?

Sheila-nk · 2024-03-08T15:10:48Z

scpdt/tests/test_pytest_configuration.py::test_stopword_cases SKIPPED (need matplotlib)       [ 40%]
scpdt/tests/test_pytest_configuration.py::test_local_file_cases PASSED                        [ 42%]
scpdt/tests/test_runner.py::test_single_failure PASSED                                        [ 44%]
scpdt/tests/test_runner.py::test_exception PASSED                                             [ 46%]
scpdt/tests/test_runner.py::test_get_history PASSED                                           [ 48%]
scpdt/tests/test_runner.py::TestDebugDTRunner::test_debug_runner_failure PASSED               [ 51%]
scpdt/tests/test_runner.py::TestDebugDTRunner::test_debug_runner_exception PASSED             [ 53%]
scpdt/tests/test_skipmarkers.py::TestSyntaxErrors::test_invalid_python FAILED                 [ 55%]
scpdt/tests/test_skipmarkers.py::TestSyntaxErrors::test_invalid_python_plus_skip PASSED       [ 57%]
scpdt/tests/test_skipmarkers.py::TestSyntaxErrors::test_invalid_python_pseudocode PASSED      [ 59%]
scpdt/tests/test_skipmarkers.py::TestPseudocodeMarkers::test_pseudocode_markers PASSED        [ 61%]
scpdt/tests/test_skipmarkers.py::TestStopwords::test_bogus_output FAILED                      [ 63%]
scpdt/tests/test_skipmarkers.py::TestMayVary::test_may_vary PASSED                            [ 65%]
scpdt/tests/test_skipmarkers.py::TestMayVary::test_may_vary_source PASSED                     [ 67%]
scpdt/tests/test_skipmarkers.py::test_SKIPBLOCK PASSED                                        [ 69%]
scpdt/tests/test_testfile.py::test_one_scipy_tutorial XFAIL (needs the scipy repo at a fi...) [ 71%]
scpdt/tests/test_testfile.py::test_linalg_clone PASSED                                        [ 73%]
scpdt/tests/test_testmod.py::test_module PASSED                                               [ 75%]
scpdt/tests/test_testmod.py::test_module_vanilla_dtfinder PASSED                              [ 77%]
scpdt/tests/test_testmod.py::test_stopwords FAILED

In the above run, I skipped test_stopword_cases using the HAVE_MATPLOTLIB variable. It was skipped and the rest of the tests that require matplotlib and have no skip marker failed as expected.

@pytest.mark.skipif(not HAVE_MATPLOTLIB, reason='need matplotlib')
    def test_stopword_cases(pytester):
        """Test that pytest uses the DTParser for doctests."""
        path_str = stopwords_cases.__file__
        python_file = PosixPath(path_str)
        result = pytester.inline_run(python_file, "--doctest-modules")
>       assert result.ret == pytest.ExitCode.OK

But when I installed matplotlib, all tests that require matplotlib failed.
And this is the error:

Failed example:
    import matplotlib.pyplot as plt; plt.xlim([1, 2])
Exception raised:
    Traceback (most recent call last):
      File "/Users/sheilakahwai/mambaforge/lib/python3.10/doctest.py", line 1350, in __run
        exec(compile(example.source, filename, "single",
      File "<doctest stopwords_bogus_output[0]>", line 1, in <module>
        import matplotlib.pyplot as plt; plt.xlim([1, 2])
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/pyplot.py", line 1958, in xlim
        ax = gca()
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/pyplot.py", line 2540, in gca
        return gcf().gca()
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/figure.py", line 1658, in gca
        return ax if ax is not None else self.add_subplot()
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/figure.py", line 782, in add_subplot
        ax = projection_class(self, *args, **pkw)
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/axes/_base.py", line 670, in __init__
        self._init_axis()
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/axes/_base.py", line 812, in _init_axis
        self.xaxis = maxis.XAxis(self, clear=False)
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/axis.py", line 2295, in __init__
        super().__init__(*args, **kwargs)
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/axis.py", line 676, in __init__
        self.label = mtext.Text(
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/text.py", line 155, in __init__
        self.update(kwargs)
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/text.py", line 197, in update
        kwargs = cbook.normalize_kwargs(kwargs, Text)
      File "/Users/sheilakahwai/Documents/Work/scpdt/venv/lib/python3.10/site-packages/matplotlib/cbook.py", line 1783, in normalize_kwargs
        for canonical, alias_list in alias_mapping.items()
    AttributeError: type object 'Text' has no attribute 'items'

When I remove the matplotlib import, all the tests pass.

ev-br · 2024-03-08T15:31:37Z

on main with MPL, do tests pass?
on main + a single conditional skip, do tests pass?

Sheila-nk · 2024-03-08T15:54:20Z

on main with MPL, do tests pass?

Yes, all tests pass.

on main + a single conditional skip, do tests pass?

No. Interestingly, attempting to import MPL causes ALL tests requiring MPL to fail, even before I apply skip markers to any of the tests.
It's quite strange.

The current workaround I have found is conditionally importing MPL at the test level.

ev-br · 2024-03-08T16:34:06Z

What is the MPL version and what happens if you downgrade it?

ev-br · 2024-03-08T17:43:11Z

OK, it does not look dependent on the MPL version in the end. In two envs with matplotlib 3.7.1 and 3.8.1, using the following patch seems to work:

$ git diff
diff --git a/scpdt/tests/test_skipmarkers.py b/scpdt/tests/test_skipmarkers.py
index d68e2b8..64c82f0 100644
--- a/scpdt/tests/test_skipmarkers.py
+++ b/scpdt/tests/test_skipmarkers.py
@@ -4,11 +4,20 @@ import pytest
 from ..impl import DTConfig, DTParser, DebugDTRunner
 
 
+try:
+    import matplotlib.pyplot as plt    # noqa
+    HAVE_MATPLOTLIB = True
+except Exception:
+    HAVE_MATPLOTLIB  = False
+
+HAVE_MATPLOTLIB = False
+
 class TestSyntaxErrors:
     """Syntax errors trigger a doctest failure *unless* marked.
 
     Either mark it with +SKIP or as pseudocode.
     """
+    @pytest.mark.skipif(not HAVE_MATPLOTLIB, reason="MPL optional")
     def test_invalid_python(self):
         # This string raises
         # TypeError("unsupported operand type(s) for +: 'int' and 'tuple'")

Note that here I'm importing matplotlib.pyplot instead of matplotlib itself.

Why?

matplotlib/matplotlib#26827 looks related. I have no clue of what's going on there, apart from "something is weird in the way the top-level matplotlib package is organized".
Furthermore, note that if we import matplotlib itself,

$ pytest --pyargs scpdt   # breaks
$ pytest scpdt/tests         # breaks
$ pytest .                            # breaks
$ pytest scpdt/tests/test_skipmarkers.py     # does not break!

So something is weird in how the matplotlib namespace imports but it's usually OK. Something is weird in the way pytest imports things but it's usually OK. When these two weirdnesses collide, they conspire.

Can you check if this works on your system Sheila?

Sheila-nk · 2024-03-11T11:54:54Z

Can you check if this works on your system Sheila?

Yes, works both locally and on GH! 👍

ev-br · 2024-03-11T12:09:35Z

Great! So can finish up this PR then?

Sheila-nk · 2024-03-11T18:17:35Z

Yes, I believe it's done. Maybe you can review it before I proceed to merge?

ev-br · 2024-03-12T10:15:38Z

LGTM now, thank you Sheila!

Split off optional dependencies (scipy & matplotlib), skip tests when these are not available. Reviewed at scipy/scipy_doctest#131

Sheila-nk changed the title ~~MAINT: separate install of plugin and optional dependencies~~ MAINT: refine installation of core and test dependencies Feb 28, 2024

Sheila-nk added the maintenance label Feb 28, 2024

ev-br requested changes Feb 28, 2024

View reviewed changes

.github/workflows/pip.yml Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

Sheila-nk added 4 commits March 5, 2024 10:26

remove install of optional dependencies

b38cee2

install optional dependencies separately

2fc0d1b

install core and test dependencies

2f30c99

skip self tests that require scipy or matplotlib

6eb2002

Sheila-nk force-pushed the dependencies branch from 2646f36 to 6eb2002 Compare March 5, 2024 07:43

Sheila-nk requested a review from ev-br March 5, 2024 07:47

ev-br reviewed Mar 5, 2024

View reviewed changes

Sheila-nk added 2 commits March 6, 2024 11:53

add workflow for tests that don't require scipy and MPL

1ba20a0

install project in editable mode

7ae6152

run self-tests with and without optional dependencies

6647b29

ev-br mentioned this pull request Mar 7, 2024

MAINT: close the figures before backend change #145

Merged

try import of MPL at test level

e27f36f

make self-test names more descriptive

07ae218

Sheila-nk requested a review from ev-br March 8, 2024 16:01

ev-br mentioned this pull request Mar 9, 2024

MAINT: Adapt to NumPy 2.0 #146

Merged

import matplotlib.pyplot

2f7de2a

ev-br merged commit 4e33c90 into scipy:main Mar 12, 2024
4 checks passed

cr313 added a commit to cr313/doctest-scipy that referenced this pull request Apr 19, 2024

MAINT: refine installation of core and test dependencies (#131)

49a66db

Split off optional dependencies (scipy & matplotlib), skip tests when these are not available. Reviewed at scipy/scipy_doctest#131

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: refine installation of core and test dependencies #131

MAINT: refine installation of core and test dependencies #131

Sheila-nk commented Feb 28, 2024

Sheila-nk commented Feb 28, 2024

ev-br left a comment

Sheila-nk commented Feb 28, 2024

ev-br left a comment

ev-br Mar 5, 2024

Sheila-nk Mar 5, 2024

Sheila-nk commented Mar 5, 2024

ev-br commented Mar 5, 2024

Sheila-nk commented Mar 6, 2024

ev-br commented Mar 6, 2024

Sheila-nk commented Mar 6, 2024

Sheila-nk commented Mar 6, 2024

ev-br commented Mar 6, 2024

Sheila-nk commented Mar 7, 2024

ev-br commented Mar 7, 2024

ev-br commented Mar 7, 2024 •

edited

Loading

Sheila-nk commented Mar 8, 2024

Sheila-nk commented Mar 8, 2024

ev-br commented Mar 8, 2024

Sheila-nk commented Mar 8, 2024 •

edited

Loading

ev-br commented Mar 8, 2024

ev-br commented Mar 8, 2024

Sheila-nk commented Mar 11, 2024

ev-br commented Mar 11, 2024

Sheila-nk commented Mar 11, 2024

ev-br commented Mar 12, 2024

MAINT: refine installation of core and test dependencies #131

MAINT: refine installation of core and test dependencies #131

Conversation

Sheila-nk commented Feb 28, 2024

Sheila-nk commented Feb 28, 2024

ev-br left a comment

Choose a reason for hiding this comment

Sheila-nk commented Feb 28, 2024

ev-br left a comment

Choose a reason for hiding this comment

ev-br Mar 5, 2024

Choose a reason for hiding this comment

Sheila-nk Mar 5, 2024

Choose a reason for hiding this comment

Sheila-nk commented Mar 5, 2024

ev-br commented Mar 5, 2024

Sheila-nk commented Mar 6, 2024

ev-br commented Mar 6, 2024

Sheila-nk commented Mar 6, 2024

Sheila-nk commented Mar 6, 2024

ev-br commented Mar 6, 2024

Sheila-nk commented Mar 7, 2024

ev-br commented Mar 7, 2024

ev-br commented Mar 7, 2024 • edited Loading

Sheila-nk commented Mar 8, 2024

Sheila-nk commented Mar 8, 2024

ev-br commented Mar 8, 2024

Sheila-nk commented Mar 8, 2024 • edited Loading

ev-br commented Mar 8, 2024

ev-br commented Mar 8, 2024

Sheila-nk commented Mar 11, 2024

ev-br commented Mar 11, 2024

Sheila-nk commented Mar 11, 2024

ev-br commented Mar 12, 2024

ev-br commented Mar 7, 2024 •

edited

Loading

Sheila-nk commented Mar 8, 2024 •

edited

Loading