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.spacy: 2.3.4 -> 2.3.5 #106731
Conversation
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.
- Diff mostly LGTM. see comments
- Commits should probably be squashed
- Builds via
nix-review
on x86-64:
Misspelled packages in the PR title.
https://github.com/NixOS/nixpkgs/pull/106731
6 packages built:
python37Packages.spacy python37Packages.textacy python38Packages.spacy python38Packages.textacy python39Packages.spacy python39Packages.textacy
--replace "blis>=0.4.0,<0.8.0" "blis>=0.4.0,<1.0" \ | ||
--replace "catalogue>=0.0.7,<1.1.0" "catalogue>=0.0.7,<3.0" \ | ||
--replace "plac>=0.9.6,<1.2.0" "plac>=0.9.6,<2.0" \ | ||
--replace "srsly>=1.0.2,<1.1.0" "srsly>=1.0.2,<3.0" \ | ||
--replace "thinc==7.4.1" "thinc>=7.4.1,<8" | ||
--replace "thinc>=7.4.1,<7.5.0" "thinc>=7.4.1,<8" |
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.
If you're relaxing dependencies, would it make sense to just change them to e.g. blis
vs blis>=0.4.0...
?
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 can see the arguments on both sides. But as a maintainer, I'd prefer to check compatibility upstream if thinc (or any of the other dependencies) has a new major version, than possibly silently failing (since we cannot run the regular unit tests and instead rely on the single passthru.tests
test).
Moreover, thinc should not be updated without taking spaCy into consideration anyway. spaCy is the only downstream package that uses it, so thinc is in nixpkgs for spaCy.
''; | ||
|
||
pythonImportsCheck = [ "spacy" ]; | ||
|
||
passthru.tests = callPackage ./annotation-test {}; | ||
passthru.tests.annotation = callPackage ./annotation-test { }; |
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 don't know what this does, honestly. A comment might be helpful?
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.
About what part? passtru.tests
is documented in the nixpkgs manual. annotation
is the name of the test, because it is an annotation test.
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.
more accurately, nix build commands can also build a set of derivations. This is just one way to organize many tests.
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'd usually not seen it in this format, so I got confused
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.
Clicked wrong button, see comments above
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.
Comments addressed. Build was fine, commits haven't changed.
Sorry, I missed the comment about the spelling error. I’ll fix that tomorrow, thanks! |
2f85ecd
to
57f858d
Compare
The typo is removed from the first commit message. |
I get:
but probably related to OpenMathLib/OpenBLAS#2995 |
Motivation for this change
python3Packagese.spacy: 2.3.4 -> 2.3.5
Changelog:
https://github.com/explosion/spacy/releases/tag/v2.3.5
Also reformat with
nixpkgs-fmt
and makepassthru.tests
an attrset.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)