MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.26k stars 641 forks source link

BLD: NumPy 2 compat for wheel builds #4620

Closed tylerjereddy closed 1 week ago

tylerjereddy commented 1 week ago

PR Checklist

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4620.org.readthedocs.build/en/4620/

github-actions[bot] commented 1 week ago

Linter Bot Results:

Hi @tylerjereddy! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code. Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/9625990228/job/26551462207


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

tylerjereddy commented 1 week ago

some of our Windows deps may also not have NumPy 2 compatible releases yet, need to check on that

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 93.57%. Comparing base (d2d9d27) to head (7fd4925). Report is 2 commits behind head on develop.

Files Patch % Lines
package/MDAnalysis/converters/ParmEd.py 33.33% 5 Missing and 1 partial :warning:
package/MDAnalysis/converters/RDKit.py 60.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #4620 +/- ## =========================================== - Coverage 93.63% 93.57% -0.06% =========================================== Files 171 183 +12 Lines 21225 22304 +1079 Branches 3930 3933 +3 =========================================== + Hits 19873 20872 +999 - Misses 894 972 +78 - Partials 458 460 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tylerjereddy commented 1 week ago

For pytng errors, I think the cross-listed PR is roughly what we'd want to do there. Even if there's no bandwidth for a release for pytng (let alone MDA) for a bit, we could perhaps point to the GitHub master branch of pytng for installs in our CI here (i.e., point pip at the GitHub repo) to keep that testing active rather than disabling pytng testing here.

There may be a few other test failures here to check, but the pytng ones stood out first.

I've adjusted the original comment to not close the matching issue, since this may be a step in the right direction, but we may need to check other deps/things a bit still.

tylerjereddy commented 1 week ago

I restarted the failed Azure jobs to see if the new pytng release helps the situation a bit.

tylerjereddy commented 1 week ago

Looking at the logs, in terms of our dependencies, I'd say parmed seems to have moderate issues with NumPy 2 (they need to adjust their array copy semantics a bit like we did). RDKit seems to have a more serious problem, resulting in hard crashes against NumPy 2. Both projects probably need a new release to support.

IAlibay commented 1 week ago

@tylerjereddy sorry I'm going to word this terribly - these are optional dependencies, is there a path you can see here where we stick to 1.x if you need these optional dependencies and 2+ if you don't? I guess in we'd effectively need two packages with different pins?

My view is that this doesn't seem like it would work, but I've put all of five minutes of thought into it.

pep8speaks commented 1 week ago

Hello @tylerjereddy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 42:1: E402 module level import not at top of file Line 43:1: E402 module level import not at top of file

Comment last updated at 2024-06-22 13:59:40 UTC
tylerjereddy commented 1 week ago

@IAlibay I think the best I can suggest is to skip the related tests/imports for parmed and rdkit under NumPy 2 until they have compatible releases available.

I pushed in a sample commit that seems to allow the testsuite to pass locally with NumPy 2. Looking at the results in CI here, the only test failure is test_creating_multiple_universe_without_offset, which I believe is a known issue/flake.

That doesn't mean the MDA team will like this, but I pushed it in so you can take a look. Maybe not too painful?

IAlibay commented 1 week ago

@IAlibay I think the best I can suggest is to skip the related tests/imports for parmed and rdkit under NumPy 2 until they have compatible releases available.

I pushed in a sample commit that seems to allow the testsuite to pass locally with NumPy 2. Looking at the results in CI here, the only test failure is test_creating_multiple_universe_without_offset, which I believe is a known issue/flake.

That doesn't mean the MDA team will like this, but I pushed it in so you can take a look. Maybe not too painful?

O wow thanks a bunch for digging into this @tylerjereddy ! I don't mind this solution, it's not "clean", but it's the "cleanest" I think we can do here - rdkit will update eventually, parmed might not and there's nothing we can do about that.

IAlibay commented 1 week ago

Thanks @tylerjereddy - going with the merge!