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

Cupy fixes #265

Open
wants to merge 6 commits into
base: cupy
Choose a base branch
from
Open

Cupy fixes #265

wants to merge 6 commits into from

Conversation

bartvm
Copy link

@bartvm bartvm commented Jul 24, 2017

A few fixes I had to make to make the Cupy branch run.

@mattjj
Copy link
Contributor

mattjj commented Jul 27, 2017

Thanks for updating this! This looks like a great step forward. (EDIT: somehow lost this line the first time)

The commit history looks a bit messy. Maybe we can clean up the commit history by first rebasing HIPS:cupy onto HIPS:master, then rebasing any new commits here onto the result? (I can do the first step if that would help, then github can probably do the second.)

Also, I think the commit with message "Add vspace support for cupy" might be better as "Add flatten support for cupy". However, it's using the old flatten code. Jamie's new flatten code dispatches to the vspace and would probably be simpler to use here. (Also I had another implementation of flatten in cupy/autograd_util.py for use with cupy, but I think Jamie's new version should make flatten work for anything that has vspaces set up.)

@bartvm
Copy link
Author

bartvm commented Jul 27, 2017

Sure, if you rebase cupy let me know and I'll rebase this branch.

Yes, it's using the flatten function from before 2 days ago, but the new one looks better. The current patch worked for CuPy arrays, but didn't work for e.g. lists of CuPy arrays (because the unflatten function would call np.split with CuPy arguments and crash) so optimizers wouldn't work.

@mattjj
Copy link
Contributor

mattjj commented Jul 27, 2017

Just pushed the rebase. Can you resolve conflicts, or should I?

Good point re: unflatten, though can you point out which np.split call you mean? More generally, it's clear that SequenceVSpace uses numpy in its flatten function and so isn't general enough. I'm not sure how to fix that, but for now we could just have a separate flatten function (e.g. in autograd/cupy/autograd_util.py). What do you think?

@bartvm
Copy link
Author

bartvm commented Jul 31, 2017

I rebased, commit history looks good now.

I generalized the vector spaces a bit so that they have a lib attribute with the correct numeric library. So instead np.flatten(x) you can write vspace(x).lib.flatten to make functions work on both NumPy and CuPy arrays. Note that this assumes that the values in lists or dictionaries are all of the same type (i.e. mixing CuPy and NumPy arrays doesn't work).

@bartvm
Copy link
Author

bartvm commented Jul 31, 2017

PS The only thing that this change breaks is that the vector spaces of [] and () don't make sense anymore since we don't know what numerical library to use to initialize empty arrays for them.

@dougalm
Copy link
Contributor

dougalm commented Aug 1, 2017

Hi Bart, thanks for working on this! Wrapping a good GPU library is probably the best thing we can do to get wider adoption of Autograd.

But I'm not totally convinced that this lib=numpy/lib=cupy approach is the right one. Here are some objections:

  • It requires the wrapped numerical libraries to provide a bunch of functions: lib.random.randn, lib.random.randint, lib.zeros and lib.concatenate. This might be fine for numpy clones, but it isn't great if we start wrapping other libraries which might not have them, or might even have functions with the same names but different semantics.
  • It adds code complexity to the Autograd core, including extra inner-loop operations like node creation.
  • As you mentioned, it breaks empty containers and it doesn't handle containers with mixed numpy/cupy arrays.

An alternative approach is to have VSpace.flatten always return a numpy array. So CupyArrayVSpace.flatten would return a regular numpy array in CPU memory. This is going to be slow for large arrays, but it's fine for testing, which is the purpose of VSpace.flatten.

A drawback of this approach is that it gives numpy a special status. We like to hold on to the idea that Autograd should be backend-agnostic. But the truth is that numpy has always been a dependency and it's likely to remain that way, at least as a fallback library. It also helps to have a concrete array type for instantiating and evaluating tests, and numpy arrays are as good a choice as any. (There's an argument that Python's native array module would be a more natural choice but we've never gone down that road.)

Are there other objections to this approach I'm not thinking of?

Then there's the question of the user-exposed function util.flatten, which we currently implement using VSpace.flatten. It's a very useful function and it needs to be fast. Like the optimizers, it's a function that should probably be considered out of scope for Autograd. But it's very handy and we've had it for a long time so we'll keep it. We've recently gone forth and back and forth again on the question of whether util.flatten should have its own separate implementation. The issues you're raising here make me think we should go back to the original approach where util.flatten doesn't use VSpace.flatten. The two use-cases have such different demands that I don't mind some code duplication. Since it's a high-level function, we could even tolerate a flag that specifies which numerical library you want to use, defaulting to numpy. That would solve the mixed types and empty container cases.

@j-towns
Copy link
Collaborator

j-towns commented Aug 1, 2017

This is exciting stuff!

Having only looked over the pr fairly briefly, I think I agree with @dougalm's points...

An alternative approach is to have VSpace.flatten always return a numpy array.

I reckon this is the right approach, provided that the performance isn't ridiculously terrible — if, say it led to the tests taking 10x longer to run I think that would be unacceptable. I have no experience with GPU computing but I'm assuming the situation won't be that bad.

Then have either @dougalm's suggestion for the high level util.flatten behaving differently according to a flag or @mattjj's suggestion of a separate flatten function just for cupy arrays (which I see is already implemented).

I'm wondering to what extent it's possible to test this new functionality on Travis, which has no GPU support, or some other CI. There's an option to install cupy without cuda, however I've tried doing that and almost none of cupy's own tests run on my machine (which has no GPU). I was hoping there might be some simulated GPU mode, but this doesn't seem to be the case. Anyone got any thoughts on testing?

@bartvm
Copy link
Author

bartvm commented Aug 1, 2017 via email

@dougalm
Copy link
Contributor

dougalm commented Aug 1, 2017

Returning a NumPy array by default in places will often just crash.

My proposal was to have CupyArrayVSpace.flatten copy data from the GPU to the CPU, returning a regular numpy array using, say, cupy.asnumpy. I haven't played with cupy much. Is there a situation where this is impossible?

The performance aspect seems easy enough to address. You can write a custom
"lib" function that acts like "vspace(x).lib" with almost no overhead.

The performance issue I was talking about wasn't overhead cost, it was the cost of copying data from the GPU to the CPU which will always be expensive! But for testing I think it's fine (subject to @j-towns' caveat that if it slows down the tests 10X it might get annoying).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants