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

Use oldest-supported-numpy instead of numpy version pinning for build system #1751

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

chajath
Copy link
Contributor

@chajath chajath commented Aug 31, 2022

Older numpy versions pinned were causing issues when those numpy versions were not compatible with newer mac platform.

Instead, let's use https://github.com/scipy/oldest-supported-numpy which does the right thing for most known build envs.

Older numpy versions pinned were causing issues when those numpy versions were not compatible with newer mac platform.

Leaving pinned version as-is for linux to make the impact minimal.
@chajath chajath force-pushed the inho/macos-compatibility-patch branch from 9147a5a to 686f189 Compare August 31, 2022 13:14
@mattwthompson
Copy link
Member

Thanks for the submission. I don't use Poetry myself so I will have to wait for feedback from somebody else. I'm a bit surprised to see such hard pinning, I personally prefer adding lower constraints until it's known that an upper constraint is also needed. But that's something that makes conda solvers happy, I'm not sure it works as well elsewhere.

Outside of this PR, it would be nice to drop older versions of Python. Usually NEP 29 is a good starting point; currently it amounts dropping everything older than Python 3.8 and NumPy 1.20. If people need older versions (3.6 and 1.11 are very old) the older versions of MDTraj are still floating around out there.

@sroet
Copy link
Contributor

sroet commented Sep 1, 2022

I'm a bit surprised to see such hard pinning, I personally prefer adding lower constraints until it's known that an upper constraint is also needed.

The reason for these extreme pinnings is to solve issues like #1617, the problem/solution are discussed from this comment onward.

The TLDR is that pip will pull the most recent version of numpy it can during building (in build-isolation), completely disregarding whatever numpy is present in the environment. However, numpy is only backwards compatible; newer numpy will run code that was build against an older numpy not the other way around.

This would lead to broken builds if mdtraj was installed in an environment that does not have the most recent numpy installed.

@chajath
Copy link
Contributor Author

chajath commented Sep 1, 2022

I see. It puts devs like me in a rough spot because if we are tied to an older python version, older NumPy doesn't play nicely with M1 machine. I suggest building wheels per each major NumPy version we want to support. Or, can we relax NumPy pinning on all versions, then advertise users to do pip install --no-build-isolation to use whatever NumPy version they have in runtime to do the building?

@chajath
Copy link
Contributor Author

chajath commented Sep 1, 2022

Or, maybe we should use https://github.com/scipy/oldest-supported-numpy like sklearn does. This looks like a good solution for everyone.

@chajath chajath changed the title Relax pinned numpy version for Mac Use oldest-supported-numpy instead of numpy version pinning for build system Sep 1, 2022
@sroet
Copy link
Contributor

sroet commented Sep 1, 2022

Or, maybe we should use https://github.com/scipy/oldest-supported-numpy like sklearn does. This looks like a good solution for everyone.

The current implementation was based on the scipy implementation at the time, so this would be my preferred solution.

However, I am not a recent user of mdtraj anymore, so my preference should not be taken too seriously.

@mattwthompson
Copy link
Member

It puts devs like me in a rough spot because if we are tied to an older python version, older NumPy doesn't play nicely with M1 machine.

Is there no way for pip to use Rosetta to pull down older versions? I know conda can switch between osx-64 and osx-arm64 for this sort of thing

@chajath
Copy link
Contributor Author

chajath commented Sep 1, 2022

I would rather keep the code portable and don't have to resort to Rosetta. There might be a way, but then I would have to maintain two different toolchains and try to cross-compile and all, it will be a mess tbh.

@chajath
Copy link
Contributor Author

chajath commented Sep 1, 2022

Btw while we are on the topic of platforms, wonder if we should get onboard tools like https://github.com/pypa/cibuildwheel so we publish all bdist wheels and don't have to manage all the different platforms ourselves.

@stale
Copy link

stale bot commented May 21, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 21, 2023
@mattwthompson
Copy link
Member

If I can ever get #1758 over the finish line, we should take another look at this. I think it's probably a good solution.

@stale stale bot removed the wontfix label Jun 10, 2023
@mattwthompson
Copy link
Member

I think this should be good to go - will merge sometime if no objections

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.

None yet

3 participants