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. |
dotlambda
force-pushed
the
python3Packages.blis
branch
from
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.
@davidak Can thinc's blis pin be removed?
There was a problem hiding this comment.
Actually upstream removed it in explosion/thinc@efda3b4 so this should be fine.
| numpy | ||
| ]; | ||
|
|
||
| propagatedBuildInputs = [ numpy ]; | ||
| dependencies = [ numpy ]; |
There was a problem hiding this comment.
Isn't that a bit weird to put it in both places?
There was a problem hiding this comment.
That's what upstream specifies. IIRC numpy is imported in setup.py.
There was a problem hiding this comment.
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.
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.
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.
The order is reversed.
Without proper separation, we cannot start #272179.
There was a problem hiding this comment.
you'd get the host's Numpy available anyway, so there is no need for numpy in the
build-systemlist 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.
you'd get the host's Numpy available anyway, so there is no need for numpy in the
build-systemlist IMO.There is because in the future
dependenciesmight 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.
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.
I agree that cross doesn't work as is. But at least this continues to work if
dependenciesare 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.
dotlambda
force-pushed
the
python3Packages.blis
branch
from
18e8282
to
8719eb4
Compare
dotlambda
force-pushed
the
python3Packages.blis
branch
from
1fbe72e
to
1f03dd2
Compare
dotlambda
force-pushed
the
python3Packages.blis
branch
from
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 = relaxedsandbox = truenix-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.