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

update conda env files #132

Merged
merged 6 commits into from
Jul 18, 2024
Merged

update conda env files #132

merged 6 commits into from
Jul 18, 2024

Conversation

orbeckst
Copy link
Member

Description

The conda env files that are being used for CI appeared outdated. In this PR I am trying out if they can be simplified.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • only use conda-forge channel (removed defaults)
  • install MDAnalysis via conda instead of pip

Questions

  • Was there a good reason why MDAnalysis was pip installed??

Status

  • Ready to go

- only use conda-forge channel
  (removed defaults)
- install MDAnalysis via conda instead of pip
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (44d136b) to head (d579f44).
Report is 9 commits behind head on main.

Additional details and impacted files

@orbeckst
Copy link
Member Author

Why the f*** is the Ubuntu 3.10 job https://github.com/MDAnalysis/membrane-curvature/actions/runs/9986845992/job/27600076214?pr=132 installing MDA 2.1.0

+ mdanalysis                            2.1.0  py310hd8f1fbe_1          conda-forge       2MB

even though the spec is "MDAnalysis[>=2.0.0]": shouldn't it just go and find 2.7.0?

It then dies with

MDAnalysis/lib/_cutil.pyx:1: in init MDAnalysis.lib._cutil
    ???
E   ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

which is not surprising, given that we also installed numpy 2.0.0

    + numpy                                 2.0.0  py310h515e003_0          conda-forge       8MB

and apparently our MDA 2.1 cf package did not restrict the numpy versions on the upper end.

@IAlibay
Copy link
Member

IAlibay commented Jul 18, 2024

Makes sense, the resolution order will be to pick up the highest version of numpy and the lowest version of MDAnalysis.

@IAlibay
Copy link
Member

IAlibay commented Jul 18, 2024

I pushed some changes, i.e. removed numpy as an explicit dependency. NumPy is a dependency of MDAnalysis, so there's no point in explicitly listing it. This should fix the dependency resolution issue.

This does however point to the fact that if someone wants to install MDA right now, they'll probably encounter issues if they are doing a pure install with numpy (or anything upstream that has removed its MDA pins).

dependencies:
- python
- pip
- numpy==1.20.0
- numpy==1.21.0
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest just removing 3.9 and bumping this up - but if folks are intent on keeping 3.9, then at least increasing to 1.21.0 is a need (since osx-arm64 will require 1.21+ for py39).

The main reason we're bumping here is because newer scipy will work with older numpy, but features are mostly turned off (like pkdtree).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think 3.9 is included here because it’s the minimally supported version and that’s what this test is supposed to check.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry what I meant here was "maybe the minimum should be Python 3.10 not 3.9"

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want to bump the minimally supported Python version of membrane-curvature to 3.10 then we can do that (even though its dependencies still support 3.9). If we bump then we should do a new release with advertised 3.10 - 3.12 (not sure if 3.12 is in pyproject.toml).

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in there — so it would just be removing 3.9 along spec0 Blabla.

@orbeckst
Copy link
Member Author

Thanks @IAlibay — now all tests pass again 🎉

I’d leave the ‘min’ test for now unless people think it’s useless.

We do need an approving review to merge.

@orbeckst
Copy link
Member Author

I am going to merge for right now and we can change the minimum supported version in another PR.

@orbeckst orbeckst merged commit c44be15 into main Jul 18, 2024
18 checks passed
@orbeckst orbeckst deleted the update-test-env branch July 18, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants