Skip to content
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

pythonPackages.tensorflow: Don't change the rpath to point to gcc4.9 #26213

Merged
merged 1 commit into from
May 30, 2017

Conversation

jyp
Copy link
Contributor

@jyp jyp commented May 29, 2017

When using cuda, the rpath was set to include GCC lib version 4.9.
I am not sure what this was attempting to do, but an effect was to
prevent certain python libraries to find the correct (newer) version
of the std lib.

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@jyp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edwtjo and @FRidh to be potential reviewers.

@FRidh
Copy link
Member

FRidh commented May 29, 2017

It was added in #19701. No idea why.

@FRidh
Copy link
Member

FRidh commented May 29, 2017

@teh, do you recall?

@@ -106,7 +106,7 @@ buildPythonPackage rec {
postFixup = let
rpath = stdenv.lib.makeLibraryPath
(if cudaSupport then
[ gcc49.cc.lib zlib cudatoolkit cudnn
[ gcc.cc.lib zlib cudatoolkit cudnn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdenv.cc.cc.lib should be more idiomatic.

@@ -106,7 +106,7 @@ buildPythonPackage rec {
postFixup = let
rpath = stdenv.lib.makeLibraryPath
(if cudaSupport then
[ gcc49.cc.lib zlib cudatoolkit cudnn
[ gcc.cc.lib zlib cudatoolkit cudnn
linuxPackages.nvidia_x11 ]
else
[ gcc.cc.lib zlib ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change it here as well.

@FRidh
Copy link
Member

FRidh commented May 29, 2017

Actually, I think I recall. Tensorflow requires the compiler during runtime.
Some time after @teh proposed adding extra paths to python.buildEnv. I think that was related.

What should be done, is hardcoding the paths to the compiler.

See also #18273 (comment)

@teh
Copy link
Contributor

teh commented May 29, 2017

cuda's nvcc is tied to specific gcc versions (at least it was, maybe it's no longer?), that's why we needed to pass through a specific gcc library.

Edit: See table here: http://docs.nvidia.com/cuda/cuda-installation-guide-linux/index.html#axzz3x7mwvZrG

@jyp
Copy link
Contributor Author

jyp commented May 30, 2017

@teh: The CUDA compatibility table seems to allow for many different versions of GCC, but it does not mention version 4.9 (which is in place before the PR). It seems to me that CUDA is in fact compatible with most GCC versions, so I'd say to go for the default GCC of nixpkgs until tensorflow break. (With the added benefit to make tensorflow compatible with the rest of nixpkgs --- before this PR tensorflow is eg. incompatible with matplotlib)

Furthermore, if it is CUDA that requires a specifc GCC version, shouldn't the version-pinning happen in the CUDA package instead of tensorflow?

@FRidh: I do not know how to hardcode the path the compiler. Also, it does not seem that tensorflow requires the GCC at runtime in the current version. LLVM is used, according to https://www.tensorflow.org/performance/xla/

@FRidh
Copy link
Member

FRidh commented May 30, 2017

@jyp then I suggest you take into accounts @dezgeg 's comments and then it should be ready to go in.

When using cuda, the rpath was set to include GCC lib version 4.9.
I am not sure what this was attempting to do, but an effect was to
prevent certain python libraries to find the correct (newer) version
of the std lib.

Also avoid mentions of any specifc version in the
propagatedBuildInputs
@jyp
Copy link
Contributor Author

jyp commented May 30, 2017

@dezgeg I've applied the changes as you suggested. In fact I've removed all explicit mentions of gcc. Let me know if the code is now idiomatic or not. :)

@FRidh FRidh merged commit 84a13a8 into NixOS:master May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants