Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numpy 2.0 + scipy + matplotlib pre-release workflow #89

Merged
merged 37 commits into from
Apr 25, 2024

Conversation

mscheltienne
Copy link
Collaborator

@mscheltienne mscheltienne commented Mar 12, 2024

Closes #88

  • Pin pytest to 8.0 and above. pytest introduced backward incompatible changes especially in the handling of warnings through pytest.warns.
  • Move conftest.py to the root of the repository, mimicking the structure from other scientific python project (numpy, scipy, mne, ...)
  • Configure pytest to turn warnings into errors, and add structure in conftest.py to ignore warnings (example here in MNE: https://github.com/mne-tools/mne-python/blob/90067893e330c1941c055be22bc5f442ad320ec3/mne/conftest.py#L129-L210)
  • Add workflow to test against pre-release versions, especially numpy 2.0 and its numerous API/ABI breaks.
  • Update CircleCI to run on python 3.9 to 3.12 (c.f. SPEC: https://scientific-python.org/specs/spec-0000/#support-window)
  • Keep support of Python 3.7+ through compat workflow on github actions
  • Fix deprecated readfp (since python 3.2..) from configparser
  • Fix deprecation and removal in python 3.12 of loader.find_module
  • Fix numpy 2.0 depreations/removals of np.NINF and np.trapz
  • Don't forget to close log file (and prevent stray resources)
  • Suppress sphinx warnings due to function in sphinx gallery configuration
  • Fix types/values of variables in codecov configuration

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

@mscheltienne mscheltienne self-assigned this Mar 12, 2024
@mscheltienne mscheltienne changed the title Np2.0 Numpy 2.0 + scipy + matplotlib pre-release workflow Mar 12, 2024
@smoia smoia added the Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog label Mar 12, 2024
@smoia smoia added the Testing This is for testing features, writing tests or producing testing code label Mar 12, 2024
@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Mar 12, 2024

@smoia I could use your input here. This import mechanism you have in the root __init__.py:

nigsp/nigsp/__init__.py

Lines 13 to 18 in a3ff818

__all__ = []
for loader, module_name, is_pkg in pkgutil.walk_packages(__path__):
if "tests" not in module_name:
__all__.append(module_name)
_module = loader.find_module(module_name).load_module(module_name)
globals()[module_name] = _module

is causing deprecation warnings (or even AttributeError for already removed members) between python versions; and at the same time it's handling in its way circular imports making from nigsp import surrogates possible despite the circular import with operations/__init__.py.

If you really want to have all functions/members accessible from the nigsp namespace, then I think the way to go is to use lazy_loader, c.f. SPEC: https://scientific-python.org/specs/spec-0001/
But it's not yet widely adopted and adds a level of complexity that I'm not in favor of. Instead, could we remove some functions from the nigsp namespace, namely functions in operations or in cli, e.g. turning from nigsp import surrogates into from nigsp.operations import surrogates.

Alternatively, I can fix the circular import by keeping the import at the root level (nigsp) and removing it from the submodule, thus supporting from nigsp import surrogates and removing from nigsp.operations import surrogates, but I'm less in favor of this option as having everything in your root namespace leads to clogging the API.

@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Mar 12, 2024

@smoia FYI af4d907 would be my proposal.

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 97.56098% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.90%. Comparing base (cb99342) to head (493756d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   90.85%   90.90%   +0.05%     
==========================================
  Files          14       15       +1     
  Lines        1236     1287      +51     
  Branches        0      310     +310     
==========================================
+ Hits         1123     1170      +47     
- Misses        113      117       +4     
Flag Coverage Δ
unittests 83.31% <95.45%> (?)

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

Files Coverage Δ
nigsp/blocks.py 94.54% <100.00%> (ø)
nigsp/cli/run.py 97.05% <100.00%> (ø)
nigsp/io.py 87.42% <100.00%> (ø)
nigsp/objects.py 100.00% <100.00%> (ø)
nigsp/operations/surrogates.py 89.91% <ø> (ø)
nigsp/operations/timeseries.py 95.53% <100.00%> (+0.16%) ⬆️
nigsp/viz.py 96.72% <100.00%> (ø)
nigsp/workflow.py 74.55% <100.00%> (+0.11%) ⬆️
nigsp/conftest.py 91.30% <94.73%> (ø)

@mscheltienne
Copy link
Collaborator Author

@smoia If you agree with the proposed import changes above, this PR is good to go.
Also, it requires CODECOV_TOKEN to be added to the repository secrets, https://app.codecov.io/gh/MIPLabCH/nigsp, which I can not do as I lack permissions on CodeCov to view the token for this project.
(required because codecov/codecov-action@v4 does not support tokenless uploads anymore)

@smoia
Copy link
Collaborator

smoia commented Apr 18, 2024

@mscheltienne do you still need any action on the admin side? I see all checks are successful, but I don't know if you need something more?

@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Apr 19, 2024

@smoia Looks like the codecov token is already setup, the uploads did not fail (CI would have been green anyway, fail_ci_if_error defaults to False on the codecov action).
And then it's a matter of accepting the breaking change on the imports, i.e. from nigsp.operations import surrogate instead of from nigsp import surrogate.

@smoia
Copy link
Collaborator

smoia commented Apr 19, 2024

Nice - can we change the default for failing to true so that we can catch issues if they happen?

As for import, breaking API at this stage of development is fine, we just need to signal it with different labels.

@smoia smoia added the Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility label Apr 19, 2024
@smoia
Copy link
Collaborator

smoia commented Apr 19, 2024

Actually, any reason conftest needs to move to root, beside emulation of other packages?

@mscheltienne
Copy link
Collaborator Author

mscheltienne commented Apr 19, 2024

can we change the default for failing to true so that we can catch issues if they happen?

yes, but in practice it's not a great idea and that's why the default is False. An upload of a codecov report can fail for a number of reason, especially if you have like 10 workflows each uploading a report, it's not unlikely to get a couple failing because of the upload and it's more annoying than informative.

any reason conftest needs to move to root, beside emulation of other packages?

To apply the configuration to all tests. For now, it's not that important since you have all tests in the same folder, but it's common to have:

package
|_ submodule1
    |_ tests
|_ submodule2
    |_ tests
|_ tests

And in this case, if conftest.py is in package/tests, it only applies to package/tests.

@smoia
Copy link
Collaborator

smoia commented Apr 19, 2024

I see, makes sense! I'll have a look this weekend at all the changes! Thank you Mathieu!

@github-actions github-actions bot added the Documentation This issue or PR is about the documentation label Apr 22, 2024
Copy link
Collaborator

@smoia smoia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like much more/slightly different than what described in the title , but thank you for looking into all of this @mscheltienne !

Just a quick change that is more style than anything else.

Is it ready to merge or would you like to do more?

nigsp/operations/timeseries.py Outdated Show resolved Hide resolved
@mscheltienne
Copy link
Collaborator Author

Good to merge, pre-commit.ci seems to have a bad day, pre-commit is green locally.

@mscheltienne
Copy link
Collaborator Author

I'll merge as is and open a second PR to replace black and flake8 (which is failing pre-commit.ci) by ruff.
ruff is amazing, it's stupidly fast and incorporate more and more static analysis tools/formatters into a single package.

@mscheltienne mscheltienne merged commit 3a8bd18 into MIPLabCH:master Apr 25, 2024
10 of 11 checks passed
@mscheltienne mscheltienne deleted the np2.0 branch April 25, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation This issue or PR is about the documentation Internal Changes affect the internal API. It doesn't increase the version, but produces a changelog Minormod-breaking For development only, this PR increments the minor version (0.+1.0) but breaks compatibility Testing This is for testing features, writing tests or producing testing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test against numpy 2.0
2 participants