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

python3Packages: don't propagate numpy_1 if also numpy_2 is supported #338334

Closed
wants to merge 14 commits into from

Conversation

doronbehar
Copy link
Contributor

Many Python packages nowadays support both numpy versions, and even when they have components compiled against either, they don't retain references to either numpy, unless of course the numpy used is propagated. Hence it would be nice to enable users to choose either, and not force them to stick them to numpy_1 just because some dependency is propagating numpy_1 and you don't want multiple numpy versions in your nix shell or whatever.

Description of changes

  • python312Packages.numpy_1: add an isNumpy2 passthru attribute
  • python312Packages.numpy_2: add an isNumpy2 passthru attribute
  • python312Packages.numpy_1: add a coreIncludeDir passthru attribute
  • python312Packages.numpy_2: add a coreIncludeDir passthru attribute
  • python312Packages.h5py: don't propagate numpy_1
  • python312Packages.scipy: use numpy.coreIncludeDir
  • python312Packages.scipy: don't propagate numpy_1
  • python312Packages.contourpy: don't propagate numpy_1
  • python312Packages.matplotlib: don't propagate numpy_1

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@dotlambda
Copy link
Member

What's the issue with using packageOverrides if you want to use numpy_2? That way all Python packages use numpy_2.

@doronbehar
Copy link
Contributor Author

What's the issue with using packageOverrides if you want to use numpy_2? That way all Python packages use numpy_2.

Even in the context of private nix shells, I don't want to rebuild Scipy and/or Matplotlib, and a few other slightly heavy to build packages just because I want to use them with Numpy 2. Also, I want to contribute a package to Nixpkgs that requires both scipy along with numpy_2, and not numpy_1 - for that package it wouldn't be suitable to use packageOverrides - as that will still cause collisions.

@dotlambda
Copy link
Member

Even in the context of private nix shells, I don't want to rebuild Scipy and/or Matplotlib, and a few other slightly heavy to build packages just because I want to use them with Numpy 2.

You wouldn't have to once there is a top-level package in Nixpkgs using the same overrides so Hydra builds them.

@dotlambda
Copy link
Member

Also, I want to contribute a package to Nixpkgs that requires both scipy along with numpy_2, and not numpy_1 - for that package it wouldn't be suitable to use packageOverrides - as that will still cause collisions.

Is that because SciPy doesn't yet support NumPy 2?

@doronbehar
Copy link
Contributor Author

Also, I want to contribute a package to Nixpkgs that requires both scipy along with numpy_2, and not numpy_1 - for that package it wouldn't be suitable to use packageOverrides - as that will still cause collisions.

Is that because SciPy doesn't yet support NumPy 2?

Scipy does support Numpy 2:

https://github.com/scipy/scipy/blob/v1.14.1/pyproject.toml#L32-L39

You wouldn't have to once there is a top-level package in Nixpkgs using the same overrides so Hydra builds them.

If I understand you correctly, then you are supporting the idea of #327437 , which I got convinced is not that good.

@dotlambda
Copy link
Member

dotlambda commented Sep 1, 2024

If I understand you correctly, then you are supporting the idea of #327437 , which I got convinced is not that good.

No. I suggest you add a top-level package (outside of python3Packages) that uses packageOverrides to set numpy = self.numpy_2. It would probably be the package you mention here:

Also, I want to contribute a package to Nixpkgs that requires both scipy along with numpy_2, and not numpy_1 - for that package it wouldn't be suitable to use packageOverrides - as that will still cause collisions.

@doronbehar
Copy link
Contributor Author

No. I suggest you add a top-level package (outside of python3Packages) that uses packageOverrides to set numpy = self.numpy_2. It would probably be the package you mention here:

OK I understand you now, but the package I was talking about is a Python package I want to add to python-packages.nix - that's why I said it wouldn't be suitable to use packageOverrides.

@dotlambda
Copy link
Member

OK I understand you now, but the package I was talking about is a Python package I want to add to python-packages.nix - that's why I said it wouldn't be suitable to use packageOverrides.

You can still add it but set something like meta.broken = lib.versionOlder numpy.version "2".
It would still be very useful to have a top-level package that makes Hydra build an overridden Python package set with NumPy 2.

@doronbehar
Copy link
Contributor Author

You can still add it but set something like meta.broken = lib.versionOlder numpy.version "2".

I think I failed to explain to you the situation. Let's talk about that Python package I want to add in a less hypothetical environment, at #327446 - try to solve that dependency nightmare with what ever idea you have in mind.

It would still be very useful to have a top-level package that makes Hydra build an overridden Python package set with NumPy 2.

That's indeed a nice idea but I am not able to imagine how would you do that. I will say though that I liked the idea of adding to passthru.tests an overriden version of the package with the other numpy - like you did in #333221 , and I did that for Scipy as well here.

BTW I tested that Scipy builds with this change applied on master so I'm marking this as non-draft.

A very similar change to this was done in #323171 - a Python package that explicitly states that it supports two variants of one of its dependencies, shouldn't propagate only 1 of them.

@doronbehar doronbehar marked this pull request as ready for review September 1, 2024 07:00
@@ -14204,7 +14204,9 @@ self: super: with self; {

scikit-tda = callPackage ../development/python-modules/scikit-tda { };

scipy = callPackage ../development/python-modules/scipy { };
scipy = callPackage ../development/python-modules/scipy {
numpy = numpy_2;
Copy link
Member

Choose a reason for hiding this comment

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

We can't do that within python3Packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do that within python3Packages.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a semantic change - I won't mind using numpy_2 directly in scipy/default.nix.

Copy link
Member

Choose a reason for hiding this comment

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

I won't mind using numpy_2 directly in scipy/default.nix.

That's not the issue.
Your change breaks python3.withPackages (ps: with ps; [ numpy scipy ]).

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, that's not true.
But it does break python3.withPackages (ps: with ps; [ scipy ]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, that's not true. But it does break python3.withPackages (ps: with ps; [ scipy ]).

OK so what is your opinion on the matter? Use numpy = numpy_2 in python-packages.nix, or use numpy_2 directly in the scipy/default.nix expression?

Copy link
Member

Choose a reason for hiding this comment

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

Use numpy everywhere. And propagate it.

@doronbehar doronbehar marked this pull request as draft September 1, 2024 07:26
@doronbehar
Copy link
Contributor Author

I marked it again as a draft because something is not working currently with scipy and numpy_2 - both dependencies got mixed somehow - headers from numpy_2 were used with shared objects of numpy_1. Should be fixable.

@doronbehar
Copy link
Contributor Author

I'm mostly interested in your view on #327446 .

@doronbehar doronbehar force-pushed the pkg/no-propagate-numpy_1 branch from 6265010 to 3541ac7 Compare September 1, 2024 10:11
@doronbehar doronbehar marked this pull request as ready for review September 1, 2024 10:11
@doronbehar doronbehar force-pushed the pkg/no-propagate-numpy_1 branch from 3541ac7 to 6e75b64 Compare September 1, 2024 13:57
@doronbehar
Copy link
Contributor Author

Fixed the nixfmt issue in the last push.

@doronbehar doronbehar requested a review from dotlambda September 1, 2024 14:06
@natsukium
Copy link
Member

I'm against moving numpy to buildInputs because I think that the lack of propagation of numpy greatly impacts downstream packages.
Also, having a separate package tree for numpy2 seems to put an undue burden on all python maintainers.
I like dotlambda's suggestion, passthru.tests.

@dotlambda
Copy link
Member

dotlambda commented Sep 2, 2024

I like dotlambda's suggestion, passthru.tests.

The problem is that only works well if none of your dependencies propagate numpy.
In the other case a simple override won't work and you need packageOverrides. But then something like #327446 (comment) feels like a better solution to me.

@doronbehar doronbehar force-pushed the pkg/no-propagate-numpy_1 branch from 6e75b64 to 0e9a56e Compare September 4, 2024 21:44
@doronbehar
Copy link
Contributor Author

I'm against moving numpy to buildInputs because I think that the lack of propagation of numpy greatly impacts downstream packages.

Another thing I can try to do, is iterate (with git grep) all the direct dependent packages of h5py, pythran, scipy, matplotlib & contourpy and make sure that they get a numpy from other dependencies and if not, I'll add it myself.

@dotlambda dotlambda mentioned this pull request Sep 5, 2024
13 tasks
@infinisil
Copy link
Member

Just passing by, but I do agree that this isn't great because it breaks downstream packages, requiring people to add numpy to their own non-Nixpkgs derivations to get them to build again. I can't imagine doing this for all packages with new versions, and from what I can tell, there's nothing special about numpy's new version.

I do like @dotlambda's #327446 (comment) to just get a single package for Numpy 2. #339657 also seems nice, I left some comments.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/several-comments-about-priorities-and-new-policies-in-the-python-ecosystem/51790/9

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar closed this Sep 9, 2024
@doronbehar doronbehar deleted the pkg/no-propagate-numpy_1 branch October 30, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants