gin-config: init #49676
gin-config: init #49676
Conversation
|
|
||
| }; | ||
|
|
||
| buildInputs = [ six enum34 tensorflow ]; |
There was a problem hiding this comment.
I'd add an optional parameter withTensorflow ? false here as it seems to be optional according to setup.py: https://github.com/google/gin-config/blob/master/setup.py#L51
This might look like this then:
{
# ...
propagatedBuildInputs = [ six enum34 ] ++ lib.optional withTensorflow tensorflow;
}From my understanding this project doesn't need to be used for ML projects and in such a case I doubt that you want to have tensorflow in your build closure :)
There was a problem hiding this comment.
I wrote a separate comment already, but no, there should not be an option for it.
|
|
||
| }; | ||
|
|
||
| propogatedBuildInputs = [ six enum34 ] + lib.optional withTensorflow tensorflow; |
There was a problem hiding this comment.
this should be ++.
lib.optional does basically the following: cond: a: if cond then [ a ] else [].
There was a problem hiding this comment.
oops thanks! amateur mistake :P
|
have you tested this on Python3? Let's see what borg says about this: @GrahamcOfBorg build pythonPackages.gin-config python3Packages.gin-config |
|
No attempt on x86_64-linux (full log) The following builds were skipped because they don't evaluate on x86_64-linux: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
No attempt on x86_64-darwin (full log) The following builds were skipped because they don't evaluate on x86_64-darwin: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
ahh right... should've pushed the fix at first :D @GrahamcOfBorg build pythonPackages.gin-config python3Packages.gin-config |
|
Failure on aarch64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Failure on x86_64-darwin (full log) Attempted: python3Packages.gin-config The following builds were skipped because they don't evaluate on x86_64-darwin: pythonPackages.gin-config Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Works for me. |
|
yeah, that's what I suspected, unfortunately. I obvserved such issues some times in recent history, but I'm not sure yet how to proceed here. Let's see if I have sufficient time to debug this tomorrow, if I forget about this, feel free to ping me :) |
|
fwiw i built with this command |
| , enum34 | ||
| , absl-py | ||
| , tensorflow | ||
| , withTensorflow ? true |
There was a problem hiding this comment.
This is pointless. If one (does not) want Tensorflow, they will need to override this expression. In that case they might as well use overridePythonAttrs. Tensorflow is an optional dependency, so if someone wants to use this package in conjunction with tensorflow, they can just add both to the environment, no overriding necessary at all!
There was a problem hiding this comment.
you're right, sorry, missed that during my review :/
There was a problem hiding this comment.
I made changes to the derivation based on what I think you mean, hope it's correct!
|
@jethrokuan what @FRidh wanted to say is that Sorry, I didn't think of this, that was clearly my mistake! |
|
So, I suspect that tensorflow seems to have caused the issue locally. Removing it from the list of check inputs starts at least the test suite. However, the PyPI source archive doesn't ship the tests, so now we can do two things:
Except for that issue, the PR is looking good now and should be ready to merge :) |
|
When enabling tests, I suggest disabling tests that require tensorflow because it's a very heavy dependency. |
You're right, totally didn't catch this. I'll add |
PyPI archive does not contain tests
jethrokuan
force-pushed
the
feat/gin-config
branch
from
1ccfd23
to
f8b5d1a
Compare
|
I couldn't build it locally with |
|
may I ask from which state you built this? I just checked out your branch, built Furthermore, please change |
|
@GrahamcOfBorg build pythonPackages.gin-config python3Packages.gin-config |
|
Failure on x86_64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Failure on x86_64-darwin (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
@GrahamcOfBorg build pythonPackages.gin-config python3Packages.gin-config |
|
Success on aarch64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: pythonPackages.gin-config, python3Packages.gin-config Partial log (click to expand)
|
|
@FRidh as you requested changes here as well, I'd wait for your approval ok? :) |
| @@ -15,7 +15,7 @@ buildPythonPackage rec { | |||
|
|
|||
| }; | |||
|
|
|||
| propogatedBuildInputs = [ six enum34 ]; | |||
| propagatedBuildInputs = [ six enum34 ]; | |||
|
Ah, the commits were not squashed together :( |
Motivation for this change
Add new python package
gin-configThings done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)