MDAnalysis / cookiecutter-mdakit

Cookiecutter for Python packages based on MDAnalysis
MIT License
7 stars 5 forks source link

Installing MDAnalysis from pip deps triggers numpy error #129

Closed lilyminium closed 2 months ago

lilyminium commented 2 months ago

From mdakit-cookie CI -- https://github.com/MDAnalysis/mdakit-cookie/actions/runs/9615305167

Across Pythons 3.10-3.12 on MDAnalysis develop, we get a header size error:

Run pyver=$(python -c 'import MDAnalysis; print(MDAnalysis.__version__)')
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/MDAnalysis/__init__.py", line 193, in <module>
    from .lib import log
  File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/MDAnalysis/lib/__init__.py", line 34, in <module>
    from . import transformations
  File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/MDAnalysis/lib/transformations.py", line 172, in <module>
    from .mdamath import angle as vecangle
  File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/MDAnalysis/lib/mdamath.py", line 63, in <module>
    from . import util
  File "/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/MDAnalysis/lib/util.py", line 221, in <module>
    from ._cutil import unique_int_1d
  File "MDAnalysis/lib/_cutil.pyx", line 1, in init MDAnalysis.lib._cutil
ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

Possibly needs pinning to numpy < 2.0?

Error not triggered by conda.

IAlibay commented 2 months ago

Yeah I suspect that a pin is the way forward until the next release - sorry about that :(

lilyminium commented 2 months ago

Thanks @IAlibay! Just thinkign about this some more wrt the workshop and future maintainability of any MDAKits that might be created here, I was thinking that maybe the best way to do this would actually be in the install-mdanalysis action -- just adding a 'numpy<2.0' around here https://github.com/MDAnalysis/install-mdanalysis/blob/60ef7aab832dad506a65e28f4c927121b028191a/action.yaml#L107 . Any thoughts on that? That way if we do start supporting 2.0 then it'll automatically propagate through to kits that use the install-mdanalysis action without them needing to change anything.

IAlibay commented 2 months ago

Actually, I'm pretty sure I started pinning an upper version of numpy a while back - what version of MDA are you picking up?

IAlibay commented 2 months ago

Ok just checked, I started putting an upper pin for numpy in the package with release 2.6 and above.

As a minimum here (in the cookiecutter) I would suggest pinning the minimum MDA to >2.6, at least that'll ensure a proper failure when trying to combine np 2.0+ and MDA right now. Thoughts on this @lilyminium ?

Something does need doing about install-mdanalysis, will think about that now.

lilyminium commented 2 months ago

The failures only happen when installing from develop, not any particular release package. Happy to move the pin upwards but I'm not sure that's the issue here. The logs from the failed run indicate that MDA wasn't installed before the install-mdanalysis action, so I don't think it could be the case where an older MDA is installed with numpy 2.0, then force-replaced with a newer one.

IAlibay commented 2 months ago

when installing develop

Ah ok, that's a key bit of information I didn't know. I have an idea, need to double check something.

IAlibay commented 2 months ago

Ok I think I got it, sorry there's just too many threads going on and I completely forgot about this https://github.com/MDAnalysis/mdanalysis/pull/4620

I'm going to aim to merge that PR and I think it'll clean things up, or at least it'll take us to a different state.

IAlibay commented 2 months ago

This should be fixed upstream - can you confirm that this is also resolved for you here @lilyminium ?

IAlibay commented 2 months ago

I re-ran the above-mentioned CI matrix and everything seems to run fine now: https://github.com/MDAnalysis/mdakit-cookie/actions/runs/9615305167 - probably means we're ok?

lilyminium commented 2 months ago

Thanks for putting in the fix @IAlibay! I'm happy that CI is all green now on https://github.com/MDAnalysis/mdakit-cookie. Closing now :-)