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.

Sign up for GitHub

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

Jump to bottom

keras: 1.0.3 -> 1.1.1 #20798

Closed
wants to merge 1 commit into from
Closed

keras: 1.0.3 -> 1.1.1 #20798

wants to merge 1 commit into from

Conversation

tindzk
Copy link
Contributor

@tindzk tindzk commented Nov 29, 2016

Motivation for this change

This updates Keras and its dependencies to support the latest Python and CUDA versions.

As of now, only the TensorFlow backend can be used (requires #20794). When using the Theano backend, it crashes with the following message:

Using Theano backend.
ERROR (theano.sandbox.cuda): Failed to compile cuda_ndarray.cu: libcublas.so.8.0: cannot open shared object file: No such file or directory
WARNING (theano.sandbox.cuda): CUDA is installed, but device gpu0 is not available  (error: cuda unavailable)

Trying to set the missing paths, leads to a segmentation fault:

$ result/bin/bash
$ export CUDA_HOME=/nix/store/f4lxdw6pjbj40dic2p546nc05afqvgjx-cudatoolkit-8.0.44/
$ export LD_LIBRARY_PATH=/nix/store/f4lxdw6pjbj40dic2p546nc05afqvgjx-cudatoolkit-8.0.44/lib64/
$ python -m theano
*** Error in `python': free(): invalid pointer: 0x00007f42dd4fa380 ***
======= Backtrace: =========
/usr/lib/libc.so.6(+0x70c4b)[0x7f42fefb3c4b]
/usr/lib/libc.so.6(+0x76fe6)[0x7f42fefb9fe6]
/usr/lib/libc.so.6(+0x777de)[0x7f42fefba7de]
/Users/tim/.theano/compiledir_Linux-4.8--ARCH-x86_64-with-arch--3.5.2-64/cuda_ndarray/cuda_ndarray.so(_ZNSt6locale5_Impl16_M_install_facetEPKNS_2idEPKNS_5facetE+0x142)[0x7f42dd296ff2]
[...]

Would it be possible to create a "virtual" package theano with a boolean property gpuSupport? Right now, Keras pulls in the Keras package without GPU support.

I couldn't check whether the changes from patchPhase are still needed to support Python 3.

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

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

@joachifm
Copy link
Contributor

Would it be possible to create a "virtual" package theano with a boolean property gpuSupport? Right now, Keras pulls in the Keras package without GPU support.

The idiomatic solution is to simply pass the variant you want (it only "pulls in" the non-GPU variant because that's the one that happens to be bound to that name).

joachifm
joachifm requested changes Nov 29, 2016
View reviewed changes
pkgs/development/python-modules/theano/cuda/default.nix
sed -i -re "s/(ctx.bin_id\[\-2:\])/int(\1)/g" dnn.py
popd
'';
# patchPhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this if it is no longer used.

pkgs/top-level/python-packages.nix
@@ -23341,7 +23341,7 @@ in {
};
};

Theano = buildPythonPackage rec {
theano = buildPythonPackage rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing bound names like this can be annoying, consider either not doing it or adding an alias.

Copy link
Member

@FRidh FRidh Nov 30, 2016 •
edited

Choose a reason for hiding this comment

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

Within python-packages.nix we also aim to stick to the exact name upstream uses - which is Theano with a capital T.

pkgs/top-level/python-packages.nix
];

meta = {
description = "Deep Learning library for Theano and TensorFlow";
description = "Keras is a high-level neural networks library for Theano and TensorFlow";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the package name from the description, per the guidelines.

@joachifm
Copy link
Contributor

mentionbot failed to mention keras maintainer @NikolaMandic, they might be able to provide some more substantive comments on this.

@NikolaMandic
Copy link
Contributor

well it seems @tindzk version bumped keras and it floats his boat
I would not remove theano seds before figuring out what writer had in mind as a side note

@FRidh
Copy link
Member

FRidh commented Nov 30, 2016

Note 07dcc4f

@tindzk
Copy link
Contributor Author

tindzk commented Nov 30, 2016

Thanks for the review! I have rebased the commit and implemented your suggestions.

@artuuge: Is it safe to remove the sed calls?

@FRidh
Copy link
Member

FRidh commented Jan 2, 2017

This has been open for too long. What is the status of this? Some of this has been merged already if I am correct. At least, we have a

  TheanoWithCuda = callPackage ../development/python-modules/Theano/theano-with-cuda

@tindzk
Copy link
Contributor Author

tindzk commented Jan 3, 2017 •
edited

The only remaining issue is to clarify the sed calls and to get the Theano backend to work.

@tindzk
Copy link
Contributor Author

tindzk commented Feb 14, 2017

Is anyone interested in taking over the maintenance of this package?

@FRidh FRidh self-assigned this Feb 15, 2017
@FRidh
Copy link
Member

FRidh commented Feb 15, 2017

I've upgraded Keras in master but I don't know whether it works. There are some other changes in this commit here as well, but they should have been split into separate commits and so for now I've ignored them.

@FRidh FRidh closed this Feb 15, 2017
@tindzk
Copy link
Contributor Author

tindzk commented Feb 15, 2017

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@joachifm joachifm joachifm approved these changes

Assignees

@FRidh FRidh

Labels
6.topic: python 8.has: package (update)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tindzk @mention-bot @joachifm @NikolaMandic @FRidh @pSub