python312Packages.blis: 0.7.11 -> 1.0.0 #333221
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,46 +3,55 @@ | |
| buildPythonPackage, | ||
| fetchFromGitHub, | ||
| setuptools, | ||
| cython, | ||
| hypothesis, | ||
| numpy, | ||
| pytestCheckHook, | ||
| pythonOlder, | ||
| blis, | ||
| numpy_2, | ||
| gitUpdater, | ||
| }: | ||
|
|
||
| buildPythonPackage rec { | ||
| pname = "blis"; | ||
| version = "1.0.0"; | ||
| pyproject = true; | ||
|
|
||
| disabled = pythonOlder "3.9"; | ||
|
|
||
| src = fetchFromGitHub { | ||
| owner = "explosion"; | ||
| repo = "cython-blis"; | ||
| rev = "refs/tags/release-v${version}"; | ||
| hash = "sha256-XS6h2c+8BJ9pAvIX8340C4vRZEBRmEZc6/6tH7ooqNU="; | ||
| }; | ||
|
|
||
| postPatch = '' | ||
| # The commit pinning numpy to version 2 doesn't have any functional changes: | ||
| # https://github.com/explosion/cython-blis/pull/108 | ||
| # BLIS should thus work with numpy and numpy_2. | ||
| substituteInPlace pyproject.toml setup.py \ | ||
| --replace-fail "numpy>=2.0.0,<3.0.0" numpy | ||
|
|
||
| # See https://github.com/numpy/numpy/issues/21079 | ||
| # has no functional difference as the name is only used in log output | ||
| substituteInPlace blis/benchmark.py \ | ||
| --replace-fail 'numpy.__config__.blas_ilp64_opt_info["libraries"]' '["dummy"]' | ||
| ''; | ||
|
|
||
| preCheck = '' | ||
| # remove src module, so tests use the installed module instead | ||
| rm -rf ./blis | ||
| ''; | ||
|
|
||
| build-system = [ | ||
| setuptools | ||
| cython | ||
| numpy | ||
| ]; | ||
|
|
||
| dependencies = [ numpy ]; | ||
|
|
||
| nativeCheckInputs = [ | ||
| hypothesis | ||
|
|
@@ -52,16 +61,18 @@ buildPythonPackage rec { | |
| pythonImportsCheck = [ "blis" ]; | ||
|
|
||
| passthru = { | ||
|
Comment on lines
-55
to
-56
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. |
||
| tests = { | ||
| numpy_2 = blis.overridePythonAttrs (old: { | ||
| numpy = numpy_2; | ||
| }); | ||
| }; | ||
| updateScript = gitUpdater { | ||
| rev-prefix = "release-v"; | ||
| }; | ||
| }; | ||
|
|
||
| meta = with lib; { | ||
| changelog = "https://github.com/explosion/cython-blis/releases/tag/release-v${version}"; | ||
| description = "BLAS-like linear algebra library"; | ||
| homepage = "https://github.com/explosion/cython-blis"; | ||
| license = licenses.bsd3; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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:https://github.com/explosion/cython-blis/blob/b33c2ed9cf7ed51d999a926c4950dd8dec4d0286/setup.py#L86C39-L86C58
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-systemlist 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.
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.
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.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 support this motivation and this goal, but in this case, due to the goal of upstream's
import numpyinsetup.py, it is completely incorrect to addnumpytobuild-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
dependenciesare 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.
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.