[INFRA] Test minimum supported version of matplotlib for min_requirements testing

[ymzayek]

• Closes None

See #3973 (comment)

[github-actions]

👋 @ymzayek Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

• PR has an interpretable title.
• PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
• Code is PEP8-compliant (see our documentation on coding style)
• Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

• There is at least one unit test per new function / class (see our documentation on testing)
• The new feature is demoed in at least one relevant example.

For bug fixes:

• There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

[ymzayek]

Maybe it's overkill but shouldn't we also test the latest job in testing.yml without matplotlib and plotly (maybe sticking to only latest python version)?

[codecov]

Codecov Report

Merging #3976 (18bd040) into main (945ac37) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3976   +/-   ##
=======================================
  Coverage   91.79%   91.79%           
=======================================
  Files         134      134           
  Lines       15776    15776           
  Branches     3286     3286           
=======================================
  Hits        14482    14482           
  Misses        751      751           
  Partials      543      543           

Flag	Coverage Δ
macos-latest_3.10	91.70% <ø> (?)
macos-latest_3.11	91.70% <ø> (ø)
macos-latest_3.8	91.67% <ø> (ø)
macos-latest_3.9	91.67% <ø> (ø)
ubuntu-latest_3.10	91.70% <ø> (ø)
ubuntu-latest_3.11	91.70% <ø> (ø)
ubuntu-latest_3.8	91.67% <ø> (ø)
ubuntu-latest_3.9	91.67% <ø> (?)
windows-latest_3.10	91.65% <ø> (?)
windows-latest_3.11	91.65% <ø> (ø)
windows-latest_3.8	91.61% <ø> (ø)
windows-latest_3.9	91.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Remi-Gau]

Maybe it's overkill but shouldn't we also test the latest job in testing.yml without matplotlib and plotly (maybe sticking to only latest python version)?

maybe I am missing something, but isn't that pretty much what the tests on "nilearn[min]" do?

nilearn/.github/workflows/testing.yml

Line 13 in 0c27826

min_requirements:

[ymzayek]

maybe I am missing something, but isn't that pretty much what the tests on "nilearn[min]" do?

nilearn/.github/workflows/testing.yml

Line 13 in 0c27826

min_requirements:

So the min_requirements job tests minimum versions of

min = [
"joblib==1.0.0",
"nibabel==3.2.0",
"numpy==1.19.0",
"pandas==1.1.5",
"scikit-learn==1.0.0",
"scipy==1.6.0"
]

The min_requirements_matplotlib job additionally takes plotting = ["matplotlib >= 3.3.0"] so it doesn't install the minimum version of matplotlib (it ends up installing 3.5.3).
This PR suggests to install matplotlib == 3.3.0 for this job.

The second proposition here #3976 (comment) is to test latest versions of everything without matplotlib and plotly, which we don't do.

Hope that's clear!

[bthirion]

LGTM.

[Remi-Gau]

So the min_requirements job tests minimum versions of

min = [ "joblib==1.0.0", "nibabel==3.2.0", "numpy==1.19.0", "pandas==1.1.5", "scikit-learn==1.0.0", "scipy==1.6.0" ]

The min_requirements_matplotlib job additionally takes plotting = ["matplotlib >= 3.3.0"] so it doesn't install the minimum version of matplotlib (it ends up installing 3.5.3). This PR suggests to install matplotlib == 3.3.0 for this job.

The second proposition here #3976 (comment) is to test latest versions of everything without matplotlib and plotly, which we don't do.

Hope that's clear!

Very clear thanks.

Though I as thinking that it may be easier to "read" with the proper "matrix" in CI, rather than different set of dependencies in the pyproject.toml.

Not sure how often users (or dev) actually use those different extra requirements. If they are only used in testing in CI may be better to declare them there.

[ymzayek]

@Remi-Gau I see what you mean and we can restructure it if there's a set up that makes more sense but for the developer it's nice to just be able to pip install -e [min] in a venv if you need to debug on the minimum versions. I've certainly used it before. So the use case would be to quickly replicate the ci environment to debug

[Remi-Gau]

@Remi-Gau I see what you mean and we can restructure it if there's a set up that makes more sense but for the developer it's nice to just be able to pip install -e [min] in a venv if you need to debug on the minimum versions. I've certainly used it before. So the use case would be to quickly replicate the ci environment to debug

OK if you use it once in a while let's keep in the pyproject.toml. I like to use makefile to keep track of my repo specific "dev recipes", so I can really blame any dev who makes themselves more at home in a repo. 🤗

[ymzayek]

Ok not a bad approach. I don't mind relying on makefile commands and we can certainly add some recipes to the nilearn Makefile to "replace" the environments in pyproject.toml

[Remi-Gau]

my bad: that was not a suggestion.
more a comment saying: I rarely use those dedicated extra requirements when debugging but if they make your life easier, then that's justification enough for me to have them.