-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
python312Packages.blis: 0.7.11 -> 1.0.0 #333221
Conversation
Changes look good. Why is this a draft? |
It breaks thinc and we can't depend on |
And |
Then we need an overlay there
I think we can since we catch conflicts now. :) Please give it a shoot. |
Thinc is a Python package, that won't work. |
07cecaa
to
18e8282
Compare
The package now works with both |
# Do not update to BLIS 0.9.x until the following issue is resolved: | ||
# https://github.com/explosion/thinc/issues/771#issuecomment-1255825935 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidak Can thinc's blis pin be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually upstream removed it in explosion/thinc@efda3b4 so this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can be merged of war resolve the question above this
numpy | ||
]; | ||
|
||
propagatedBuildInputs = [ numpy ]; | ||
dependencies = [ numpy ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that a bit weird to put it in both places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what upstream specifies. IIRC numpy is imported in setup.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what upstream specifies. IIRC numpy is imported in setup.py.
Hmm if so, the only good reason to still put Numpy also in the native inputs is for cross compilation, which will not work on NixOS with the way upstream uses the native Numpy in their setup.py
:
I think that for cross compilation you'd have to patch that line, and for non-cross compilation, you'd get the host's Numpy available anyway, so there is no need for numpy in the build-system
list IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We plan to separate build dependencies and runtime dependencies in the future. #272178
So, we need to add numpy to both build-system and dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We plan to separate build dependencies and runtime dependencies in the future. #272178 So, we need to add numpy to both build-system and dependencies.
I'm not sure I agree with everything said there. Even if I had agreed, #272179 should have been finished before you'd apply that policy upon other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is reversed.
Without proper separation, we cannot start #272179.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'd get the host's Numpy available anyway, so there is no need for numpy in the
build-system
list IMO.
There is because in the future dependencies
might not be available at build time in order to reduce the number of rebuilds cause by updates, overrides, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'd get the host's Numpy available anyway, so there is no need for numpy in the
build-system
list IMO.There is because in the future
dependencies
might not be available at build time in order to reduce the number of rebuilds cause by updates, overrides, etc.
I support this motivation and this goal, but in this case, due to the goal of upstream's import numpy
in setup.py
, it is completely incorrect to add numpy
to build-system
, but rather you should patch (downstream or upstream) the file to support specifying manually the host machine's Numpy headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that cross doesn't work as is. But at least this continues to work if dependencies
are unavailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that cross doesn't work as is. But at least this continues to work if
dependencies
are unavailable.
OK I understand now. Still it would have been nice if you had put a TODO comment or something. Also the intentions of #272178 should be communicated in the documentation IMO, even though not everything is implemented yet.
18e8282
to
8719eb4
Compare
1fbe72e
to
1f03dd2
Compare
01addcc
to
d3f0837
Compare
Result of 2 packages failed to build:
88 packages built:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Description of changes
Diff: explosion/cython-blis@v0.7.11...release-v1.0.0
Changelog: https://github.com/explosion/cython-blis/releases/tag/release-v1.0.0
Version 1.0 depends on numpy 2.0. C.f. #327437.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.