-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
tensorflow: 0.10.0 -> 0.12.0rc0 #20794
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
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.
It's a bit messy with the urls and hashes, but looking at the available wheels it doesn't look like it can be done much prettier either.
While I haven't tested the PR it looks good.
baeb6e6
to
2596ae0
Compare
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.
Having looked at it again I think the names of the packages should be different.
|
||
tensorflowNoGpuSupport = buildPythonPackage rec { | ||
tensorflowWithoutGpu = buildPythonPackage rec { |
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.
Is this with generic GPU support, or specifically Cuda? If I am correct it is Cuda only, and therefore the name should reflect that: tensorflowWithoutCuda
and tensorflowWithCuda
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 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.
As of now, TensorFlow does not support OpenCL. However, if we were to introduce theanoWithGpu
and theanoWithoutGpu
, I would suggest to use a similar naming here (for sake of consistency). What do you think?
I updated the package as per your suggestion. |
protobuf2_6 = self.protobufBuild pkgs.protobuf2_6; | ||
protobuf2_5 = self.protobufBuild pkgs.protobuf2_5; | ||
protobufBuild = protobuf: buildPythonPackage rec { | ||
inherit (protobuf) name src; | ||
disabled = isPy3k || isPyPy; | ||
isV2_6 = versionAtLeast protobuf.version "2.6.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.
isV2_6 = protobuf.version == "2.6.0";
?
Thanks! I renamed the variables to make it clearer. |
I tested the PR on darwin, and it works great with python 3.5 👍
|
Does it work when you change the |
No, doing so has no observable effect.
…On Thu, Dec 1, 2016 at 10:32 PM, Tim Nieradzik ***@***.***> wrote:
Does it work when you change the six dependency back to google_apputils?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20794 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABsYzd7R_-fNx-ldAnCzx_i_CylWbzbks5rDzzqgaJpZM4K_Apw>
.
|
To be sure: I'd love this PR to be merged, even in its present state, because I prefer "only python 3.5" to "only python 2.7". |
@tindzk you update two separate functions, so we need two commits. The version increments are also different for each. We typically also include only releases, not RC's. @jyp it was originally added for 2.7 so we won't break that. @teh @anderspapitto you've updated tensorflow is in the past. Comments? Finally, these two packages need a maintainer. |
@FRidh The previous version of the CUDA package was also a release candidate which lead me believe that RCs are fine. Should we wait until the final version has been released? Which two functions do you refer to? I agree that Python 2.7 support would be desirable. @jyp It seems to me that |
Nice cleanup change, lgtm from me. |
@tindzk For some reason it the .so file contains a script:
|
@tindzk Is there anything I can do to help the PR moving forward? |
@jyp From my understanding, PatchELF was applied to the |
So do we need to wait for NixOS/patchelf#108 to be fixed? Or should we attempt a workaround? |
I'd prefer to avoid workarounds and get the issue fixed in patchelf instead as it will benefit all packages with symbolic links. |
No, patchelf leaves ELF files as ELF files; it doesn't change them to a script. |
Perhaps not the right place to ask but anyway: Would it be possible to compile Tensorflow from source instead of installing binary wheel? I tried it but the bazel build system seems to make it very difficult because it does a bit what Nix should do (handles and installs dependencies). But any insight whether it would be possible and how? |
@jluttine https://github.com/nixos/nixpkgs/blob/f0373f4ceea40405491d1316529b91171744dcb3/pkgs/top-level/python-packages.nix#L30928 probably the best way forward on that is to see how debian/other big distros deal with bazel |
@jluttine - I'd vastly prefer a build from source as well, but: I spent 6+ hours trying to get bazel to work for tensorflow with not much to show. I have wrangled some really byzantine build systems but bazel exceeds my frustration tolerance. I will cheer the brave soul that accomplishes this. |
If patchelf is not the culprit, I do not know what causes the problem. Perhaps we could get a lead developer involved in the conversation? Apparently, TensorFlow isn't the only package that is affected as @quanah describes the same bug in NixOS/patchelf#108. |
As an aside, I think large, non-trivial packages like TensorFlow should really be separate Nix expressions rather than being in |
@FRidh perhaps a way to move forward is to push the cudnn version, which never worked with macos before. Would that be acceptable? |
see also #22061 regarding protobuf and python3. Your version check is a bit more elegant, but my pull requests enables also tests. A combination would be good. I would also move the build expressions into an external file for both protobuf and tensorflow. |
Currently tensorflow doesn't work on any platform due to a broken protobuf dependency - I'm +1 on merging this after the updates in #22061 to make at least Linux work again. Edit: Also +1 on moving this to its own directory under |
Is anyone interested in taking over the maintenance of the TensorFlow package? I am currently not using Nix, so I do not want to prevent this from being merged. We would have to re-base this pull request and adapt it to the latest TensorFlow version (0.12.1). |
As it were, I've done the update to 0.12.1 and used this version of tensorflow/cuda sucessfully. I'll be using tensorflow in nix for my research group in the foreseeable future. Thus I can try to take it over, but I am a relative beginner in nix. |
@jyp - Im a nix cuda-tensorflow user as well. Happy to review and / or help. Let me know what I can do. |
Motivation for this change
Update to latest TensorFlow version and support Python 3.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)