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

theano: init at 0.8.1 #14605

Merged
merged 2 commits into from
Apr 25, 2016
Merged

theano: init at 0.8.1 #14605

merged 2 commits into from
Apr 25, 2016

Conversation

bcdarwin
Copy link
Member

Built on Linux. See inline comments regarding testing.

No GPU support yet as I haven't built libgpuarray -- maybe the WIP tag should be added?

@FRidh FRidh added 6.topic: python 8.has: package (new) This PR adds a new package 2.status: work-in-progress This PR isn't done labels Apr 11, 2016
@FRidh FRidh self-assigned this Apr 12, 2016
@FRidh FRidh mentioned this pull request Apr 18, 2016
4 tasks
@teh
Copy link
Contributor

teh commented Apr 18, 2016

IMO even without GPU support it's useful to get this in, and then to update in a second commit. I was going to have a construct like this:

  theano = callPackage ({ withGPU ? false }: buildPythonPackage rec {
    name = "Theano-0.8.1";
    ...
  }) {};

So people can pick the with-gpu version like this:

(theano.override { withGPU = true; }) 

My reasoning is that for libgpuarray to be useful it needs to pull in all of cuda, but theano is also useful without having the full cuda dependency pulled in.

@FRidh
Copy link
Member

FRidh commented Apr 18, 2016

When I added numba I also did not add GPU support. We can indeed add that later.

There are quite some differences in dependencies between this expression and the one from @teh in #14790

# the fix for which hasn't been merged yet.

# keep Nose around since running the tests by hand is possible from Python or bash
propagatedBuildInputs = [ stdenv pkgs.blas ] ++ (with self; [ nose numpy pydot_ng scipy six ]);
Copy link
Member

Choose a reason for hiding this comment

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

does it really need blas during runtime? Likely it compiles something against it during build time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't tested without this, but I suspect it's necessary since compiling Theano itself is relatively trivial, but Theano itself compiles code at 'runtime'.

Copy link
Member

@FRidh FRidh Apr 23, 2016

Choose a reason for hiding this comment

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

Okay. Well, use numpy.blas instead. That way we can guarantee numpy, scipy, scikitlearn and also now theano use the same blas.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need pkgs.blas explicitly in here because it's already in numpy which is a hard dependency

Copy link
Member

Choose a reason for hiding this comment

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

But blas is a buildInput to numpy, not a propagatedBuiltInput. When theano is build, blas would not be available, unless explicitly added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - for some reason theano works without for me, I think it might be picked up by scipy if I'm reading that correctly. Probably better to explicitly communicate the blas dependency in any case.

Copy link
Member

@FRidh FRidh Apr 25, 2016

Choose a reason for hiding this comment

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

I just saw I was wrong. blas is added as a propagatedBuildInput to both numpy and scipy. Therefore you get it for free in theano, and I don't think that is correct. I'll move blas to buildInput for both numpy and scipy (if it works, it should...).

Anyway, for here we need a (propagated) buildinput on numpy.blas. I would imagine theno would build a wrapper around blas, so just buildInput would be sufficient, but who knows, maybe it really does compile against blas during runtime.

edit: no we really need blas as propagatedBuildInput! silly me

@bcdarwin
Copy link
Member Author

@teh I was going to do something similar as well, but haven't made time yet since that depends on libgpuarray being packaged.

@FRidh It could be merged now as-is, certainly.

@bcdarwin
Copy link
Member Author

OK, I've switched to numpy.blas, and the tests still seem to work. Let me know if you'd prefer this commit squashed.

I guess we can revisit whether to change blas to a buildInput once the same is done for numpy and scipy (is there an issue/PR to refer to yet?).

@FRidh
Copy link
Member

FRidh commented Apr 25, 2016

@bcdarwin I was wrong there. It is supposed to be a propagatedBuildInput because blas is needed during runtime because it is a shared library. I imagined for a moment they would statically compile it.

This looks good to me. Thanks.

@FRidh FRidh merged commit a6f7e0a into NixOS:master Apr 25, 2016
@fpletz fpletz removed the 2.status: work-in-progress This PR isn't done label Apr 25, 2016
@bcdarwin bcdarwin deleted the python_theano branch May 30, 2016 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants