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

correct upper numpy bound for scipy 1.9 #949

Closed

Conversation

gschramm
Copy link
Contributor

@gschramm gschramm commented Jan 21, 2025

Checklist

  • Used a static YAML file for the patch if possible (instructions).
  • Only wrote code directly into generate_patch_json.py if absolutely necessary.
  • Ran pre-commit run -a and ensured all files pass the linting checks.
  • Ran python show_diff.py and posted the output as part of the PR.
  • Modifications won't affect packages built in the future.

@gschramm gschramm requested a review from a team as a code owner January 21, 2025 14:46
@gschramm
Copy link
Contributor Author

  1. this patch was proposed by @zklaus following this discussion here
  2. I could not run show_diff.py, because I am struggling to get docker running on my silicon mac (classified as malware lately)

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

recipe/patch_yaml/scipy.yaml Outdated Show resolved Hide resolved
@zklaus

This comment was marked as outdated.

@gschramm
Copy link
Contributor Author

gschramm commented Jan 21, 2025

@zklaus Shouldn't we have a separate rule for 1.9.2 (which has a different lower numpy bound (1.20.3) compared to 1.9.1 and 1.9.0 (1.19.5)?

https://conda-metadata-app.streamlit.app/?q=conda-forge%2Flinux-64%2Fscipy-1.9.2-py39hddc5342_0.tar.bz2

@zklaus
Copy link

zklaus commented Jan 21, 2025

Yes, good catch!

@gschramm
Copy link
Contributor Author

@zklaus 1.9.2 and 1.9.0/1.9.1 are now separated. I used the same timestamp and hope that this is ok.

@zklaus
Copy link

zklaus commented Jan 22, 2025

With the latest changes, here is the output from show_diff.py:

╰─ ./show_diff.py
================================================================================
================================================================================
linux-armv7l
================================================================================
================================================================================
win-32
================================================================================
================================================================================
osx-arm64
osx-arm64::scipy-1.9.0-py38h4f188a7_0.tar.bz2
osx-arm64::scipy-1.9.0-py39h14896cb_0.tar.bz2
osx-arm64::scipy-1.9.1-py38h3aeb131_0.tar.bz2
osx-arm64::scipy-1.9.1-py39h737da60_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",
osx-arm64::scipy-1.9.2-py38h7b4f323_0.tar.bz2
osx-arm64::scipy-1.9.2-py39h18313fe_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
================================================================================
================================================================================
linux-ppc64le
linux-ppc64le::scipy-1.9.0-py38h7402488_0.tar.bz2
linux-ppc64le::scipy-1.9.0-py39h5352148_0.tar.bz2
linux-ppc64le::scipy-1.9.1-py38h7402488_0.tar.bz2
linux-ppc64le::scipy-1.9.1-py39h5352148_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",
linux-ppc64le::scipy-1.9.2-py38h9dbdc03_0.tar.bz2
linux-ppc64le::scipy-1.9.2-py39hcbf2e9d_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
================================================================================
================================================================================
linux-aarch64
linux-aarch64::scipy-1.9.2-py38ha629af0_0.tar.bz2
linux-aarch64::scipy-1.9.2-py39hc77f23a_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
linux-aarch64::scipy-1.9.0-py38hea604ee_0.tar.bz2
linux-aarch64::scipy-1.9.0-py39h7b076ec_0.tar.bz2
linux-aarch64::scipy-1.9.1-py38hea604ee_0.tar.bz2
linux-aarch64::scipy-1.9.1-py39h7b076ec_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",
================================================================================
================================================================================
noarch
================================================================================
================================================================================
win-64
win-64::scipy-1.9.2-py38h0f6ee2a_0.tar.bz2
win-64::scipy-1.9.2-py39hfbf2dce_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
win-64::scipy-1.9.0-py38h91810f7_0.tar.bz2
win-64::scipy-1.9.0-py39h316f440_0.tar.bz2
win-64::scipy-1.9.1-py38h91810f7_0.tar.bz2
win-64::scipy-1.9.1-py39h316f440_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",
================================================================================
================================================================================
osx-64
osx-64::scipy-1.9.0-py38hb261484_0.tar.bz2
osx-64::scipy-1.9.0-py39h29d19b3_0.tar.bz2
osx-64::scipy-1.9.1-py38hb5a21b1_0.tar.bz2
osx-64::scipy-1.9.1-py39h9488793_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",
osx-64::scipy-1.9.2-py38hfb8b963_0.tar.bz2
osx-64::scipy-1.9.2-py39h8a15683_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
================================================================================
================================================================================
linux-64
linux-64::scipy-1.9.2-py38h8ce737c_0.tar.bz2
linux-64::scipy-1.9.2-py39hddc5342_0.tar.bz2
-    "numpy >=1.20.3,<2.0a0",
+    "numpy >=1.20.3,<1.26.0",
linux-64::scipy-1.9.0-py38hea3f02b_0.tar.bz2
linux-64::scipy-1.9.0-py39h8ba3f38_0.tar.bz2
linux-64::scipy-1.9.1-py38hea3f02b_0.tar.bz2
linux-64::scipy-1.9.1-py39h8ba3f38_0.tar.bz2
-    "numpy >=1.19.5,<2.0a0",
+    "numpy >=1.19.5,<1.26.0",

I think this is ready. @conda-forge/scipy, do you agree?

@h-vetinari
Copy link
Member

So this is mostly a theoretical concern. SciPy cannot know the future, but it does know that NumPy will not break or remove things without deprecating them for at least 2 releases. Which means that, if a SciPy version was warning-free at the time of its release with NumPy 1.N, the defensive upper bound that got added was <1.{N+3}. However, I'm not aware of any breakage between 1.25 and 1.26 that would be relevant here.

So ultimately this becomes an exercise in keeping the pip check in some other package clean, which is probably a good enough reason to start patching ancient history, but first I'd like to understand why anything is still using SciPy 1.9 in this day and age (especially given that our python_min was still supported until SciPy 1.13)

@zklaus
Copy link

zklaus commented Jan 29, 2025

@h-vetinari, it's not an explicit dependency on an old version of scipy. Taking the requirements from the pyapetnet recipe, we can create an identical test environment with

mamba create -d -n pyapetnet-test "python 3.9.*" "simpleitk ~=2.0" "pymirc ~=0.29" "click ~=8.0" "tensorflow <=2.15" "pip"

This results in the solution shown in py39_wo_scipy.txt.

It includes scipy-1.9.1 pulled in via pymirc-0.30.2, which also pulls in scikit-image-0.20.0. This seems to be more or less a quirk of the solver, since it's easy to get more modern versions. For example, without the constraint on the python version, everything is more recent (which is the reason why this first shows up here in the noarch syntax migration).
One can also add scipy >1.9.1 and get scipy-1.13.1 (which I think is the last python 3.9 version), or force a newer scikit-image version.

I don't know why the solver lands on that solution, but there are no strange upper pins in the dependencies of the package, just very loose lower ones.

@h-vetinari
Copy link
Member

Can you try adding scipy >1.9 to the test requires of conda-forge/pyapetnet-feedstock#15 then? I'm not enthusiastic about patching such old packages - who knows how many environments exist where scipy 1.9 and numpy 1.26 are coinstalled without issues. We'd be breaking them for essentially no reason, so I want to make sure it's really necessary

@gschramm
Copy link
Contributor Author

@h-vetinari I added scipy >1.9 to conda-forge/pyapetnet-feedstock#15 but to run instead of test. Doesn't that make more sense?

@h-vetinari
Copy link
Member

I added scipy >1.9 to conda-forge/pyapetnet-feedstock#15 but to run instead of test. Doesn't that make more sense?

I would still only put this into test.requires, but if you prefer it this way, I don't mind. Just remember to remove that constraint when it becomes unnecessary (which you could test ~semi-regularly, i.e. every couple of months).

@gschramm
Copy link
Contributor Author

@h-vetinari moved scipy >1.9 to test.requires, but then the tests still fail. adding it to run.requires works

@gschramm gschramm closed this Jan 31, 2025
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.

4 participants