-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add loom package #3
Conversation
Adds loom, distributions, and some additional dependencies (parsable, tcmalloc, goftests) which aren't currently available on Nixpkgs. Currently some of the loom tests aren't Python3 compatible and so running `make test` in the checkPhase will always fail, for the time being pythonImportsCheck is used to check they can at least be imported.
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.
@srounce, overall this is majorly helpful; i have just a couple inline questions about how we will maintain this.
pkgs/distributions/src.nix
Outdated
@@ -0,0 +1,7 @@ | |||
{ fetchFromGitHub }: | |||
fetchFromGitHub { |
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 like this abstraction, but it looks like it's not called. What's the best way to import and invoke it? And what's more idiomatic -- src.nix or included in default.nix?
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.
Yes, this file was accidentally left in from an earlier iteration, it made more sense in the end to just declare it in a let
and then pass it into the callPackage
args of distributions-shared
. More idiomatic to have it in the default.nix as a let
binding and then pass it to both places it's used. I've removed this file now.
RESULTS = os.path.join(ROOT, 'results') | ||
SAMPLES = os.path.join(DATA, 'samples.json.gz') | ||
-IMAGE = scipy.misc.imread(os.path.join(ROOT, 'fox.png')) | ||
+IMAGE = imageio.imread(os.path.join(ROOT, 'fox.png')) |
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.
@Schaechtle , note this build patch
''; | ||
|
||
patches = [ | ||
./use-imread-instead-of-scipy.patch |
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.
@Schaechtle used here is a source code patch ...
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.
cc @srounce , do you have any links to github issues or other info that would contextualize how we would know this adaptation is no longer needed?
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.
This is needed due to the version of Scipy being newer than that expected by the tests. imread
has been deprecated from Scipy for a while it seems, and ImageIO provides a drop-in replacement. This can be removed when distributions updates the test to use imread
from ImageIO.
https://imageio.readthedocs.io/en/stable/user_guide/scipy.html
pkgs/goftests/default.nix
Outdated
doCheck = false; | ||
|
||
# https://github.com/numba/numba/issues/8698#issuecomment-1584888063 | ||
NUMPY_EXPERIMENTAL_DTYPE_API = 1; |
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.
How does nix handle these arbitrary keys at the top level in buildPythonPackage
config?
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.
It will be set as an environment variable in the builder context of the derivation's various phases. Gonna change this to env.*
as it's a bit clearer.
Adds loom, distributions, and some additional dependencies (parsable, tcmalloc, goftests) which aren't currently available on Nixpkgs. Currently some of the loom tests aren't Python3 compatible and so running `make test` in the checkPhase will always fail, for the time being pythonImportsCheck is used to check they can at least be imported.
Adds loom, distributions, and some additional dependencies (parsable, tcmalloc, goftests) which aren't currently available on Nixpkgs. Currently some of the loom tests aren't Python3 compatible and so running
make test
in the checkPhase will always fail, for the time being pythonImportsCheck is used to check they can at least be imported.