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

NEP-18: diag() fails working on CuPy array #4490

Open
pentschev opened this issue Feb 15, 2019 · 18 comments

Comments

Projects
None yet
6 participants
@pentschev
Copy link
Member

commented Feb 15, 2019

Small sample to reproduce and traceback:

import cupy
import dask.array as da

x = cupy.random.random(5000)

d = da.from_array(x, chunks=(1000), asarray=False)

print(da.diag(d).compute())
Traceback (most recent call last):
  File "diag.py", line 8, in <module>
    print(da.diag(d).compute())
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/base.py", line 156, in compute
    (result,) = compute(self, traverse=False, **kwargs)
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/base.py", line 399, in compute
    return repack([f(r, *a) for r, (f, a) in zip(results, postcomputes)])
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/base.py", line 399, in <listcomp>
    return repack([f(r, *a) for r, (f, a) in zip(results, postcomputes)])
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/array/core.py", line 779, in finalize
    return concatenate3(results)
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/array/core.py", line 3499, in concatenate3
    return _concatenate2(arrays, axes=list(range(x.ndim)))
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/array/core.py", line 227, in _concatenate2
    arrays = [_concatenate2(a, axes=axes[1:]) for a in arrays]
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/array/core.py", line 227, in <listcomp>
    arrays = [_concatenate2(a, axes=axes[1:]) for a in arrays]
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/dask/array/core.py", line 229, in _concatenate2
    return concatenate(arrays, axis=axes[0])
  File "/home/nfs/pentschev/.local/lib/python3.5/site-packages/cupy/manipulation/join.py", line 49, in concatenate
    return core.concatenate_method(tup, axis)
  File "cupy/core/_routines_manipulation.pyx", line 560, in cupy.core._routines_manipulation.concatenate_method
  File "cupy/core/_routines_manipulation.pyx", line 573, in cupy.core._routines_manipulation.concatenate_method
TypeError: Only cupy arrays can be concatenated

A little debugging shows that Dask ends up creating NumPy temporary arrays, and later tries to concatenate CuPy with NumPy arrays.

@mrocklin

@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Sounds like you are referring to this line. What would you propose to do differently?

@jakirkham jakirkham added the array label Feb 15, 2019

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Yes, that's the line. Honestly, I still don't know what to do differently to make this work. As far as I understand, once you have a Dask array, there's no way to identify it's underlying type (e.g., NumPy or CuPy). Without that information, it may be difficult to have an agnostic fix. The only thing I can think of at the moment is changing the prototype to dask.array.diag(array, module=numpy), where you could pass module=cupy, for example, but that would be very ugly and probably not too useful, since you can't automatically identify the array anyway.

If there's a way to find what type of array is the underlying Dask array that I'm missing, a fix would be very simply, just changing np.()zeros to something like getattr(array.__module__, 'zeros')().

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Hrm, well, we might consider a dispatch function like the empty_lookup that we have now, and pass in the one of our input arrays

(zeros_lookup, blocks[i], n, m)

But this is somewhat suboptimal in that it creates a data dependency, and so will likely cause most of the data to congregate on a few machines.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

It has also been proposed in the past that rather than hold onto a dtype that we instead hold onto an empty zero-shaped array that was representative of the contained arrays. There are some potential complications here, especially if we hold onto different kinds of arrays in the same dask array.

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

It has also been proposed in the past that rather than hold onto a dtype that we instead hold onto an empty zero-shaped array that was representative of the contained arrays. There are some potential complications here, especially if we hold onto different kinds of arrays in the same dask array.

I think that's probably closest to an agnostic solution we can find. In the case of a Dask array holding multiple arrays, isn't it possible to arrange the Dask array to hold the original array together with the empty zero-shaped array in the form of a tuple, for example? Maybe my question lacks depth of understanding of how Dask arrays are arranged internally, forgive me if that's the case.

@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Just to confirm, the np.diag line is ok for you? Trying to get a sense of when things typically fall down. :)

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Just to confirm, the np.diag line is ok for you? Trying to get a sense of when things typically fall down. :)

Yes, assuming that the solution is to ensure that the arrays created with np.zeros() match the underlying Dask array (either NumPy or CuPy). In that case, np.diag() would dispatch the appropriate function based on the __array_function__ protocol.

@jakirkham

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Do things like np.zeros_like behave ok in your experience?

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

I'm not sure using np.zeros_like would work, it doesn't allow changing the shape which looks necessary for dask.diag, and in this case if we had the original array, I think using np.zeros would work just fine by querying its type and dispatching .zeros from the proper library.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

I think that's probably closest to an agnostic solution we can find. In the case of a Dask array holding multiple arrays, isn't it possible to arrange the Dask array to hold the original array together with the empty zero-shaped array in the form of a tuple, for example? Maybe my question lacks depth of understanding of how Dask arrays are arranged internally, forgive me if that's the case.

Yes, I think that probably the right metadata to propagate would be a set of empty arrays with zero-length dimensions. So for a 3d array of floats we might hold onto a set with a single numpy array like...

>>> np.array([], dtype=float).reshape((0, 0, 0))
array([], shape=(0, 0, 0), dtype=float64)

This would be a significant change to Dask array, but it has come up a few times before and would, I think, be met with generally positive response among a few of the maintainers. @shoyer and @TomAugspurger have run into this I think.

@TomAugspurger

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

I like including an empty array better than my first thought (sticking the concrete array type in the name), so +1 without having given it too much thought.

@shoyer

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

The empty array seems more likely to work than simply holding the array type (because some arrays may have behavior that depends on more than just the dtype, e.g., consider different sparse array layouts), but we may still run into issues with libraries that don't handle size 0 arrays properly.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Currently all of numpy, scipy.sparse, sparse, and cupy at least allow you to construct size 0 arrays.

I think that things will get tricky when we try to emulate behavior of a function on a size-0 array. We'll probably have to try executing the function, and then if it doesn't work fall back to dtype logic and construct a new array of the same type.

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

After the *_like support in NumPy, holding empty arrays in Dask for the internal *_like operations seems to be the next big thing towards support for CuPy (and potentially others) as a Dask backend.

I'd be happy to start working with it, any suggestions on where/how we could store these empty arrays?

@mrocklin

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I recommend reading through #2977

I'll add a summary comment on the bottom there shortly.

@pentschev pentschev referenced this issue Apr 24, 2019

Open

NEP-18 Issue Tracking #4731

9 of 17 tasks complete
@TAdeJong

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

While working on #2726 I noticed that da.tril and da.triu probably have the same issue, as there is the same code pattern present. Just wanted to mention here, although I get a different traceback on my local machine currently:

>>> import numpy as np
>>> import cupy
>>> import dask.array as da
>>> x = cupy.random.random(5000)
>>> 
>>> d = da.from_array(x, chunks=(1000), asarray=False)
>>> 
>>> print(da.diag(d).compute())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tobias/code/dask-dev/dask/dask/base.py", line 156, in compute
    (result,) = compute(self, traverse=False, **kwargs)
  File "/home/tobias/code/dask-dev/dask/dask/base.py", line 398, in compute
    results = schedule(dsk, keys, **kwargs)
  File "/home/tobias/code/dask-dev/dask/dask/threaded.py", line 76, in get
    pack_exception=pack_exception, **kwargs)
  File "/home/tobias/code/dask-dev/dask/dask/local.py", line 462, in get_async
    raise_exception(exc, tb)
  File "/home/tobias/code/dask-dev/dask/dask/compatibility.py", line 112, in reraise
    raise exc
  File "/home/tobias/code/dask-dev/dask/dask/local.py", line 230, in execute_task
    result = _execute_task(task, data)
  File "/home/tobias/code/dask-dev/dask/dask/core.py", line 119, in _execute_task
    return func(*args2)
  File "/home/tobias/anaconda3/envs/cuda/lib/python3.7/site-packages/numpy/lib/twodim_base.py", line 271, in diag
    v = asanyarray(v)
  File "/home/tobias/anaconda3/envs/cuda/lib/python3.7/site-packages/numpy/core/numeric.py", line 591, in asanyarray
    return array(a, dtype, copy=False, order=order, subok=True)
ValueError: object __array__ method not producing an array
@pentschev

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@TAdeJong this is being handled in #4543, in fact I got them to work last week, but that's a long running PR, so it may take a little while to have integrated.

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Just for future reference, #4543 is integrated and fixes this issue. For it to work, NumPy 1.17 (still unreleased) is required, as well as cupy/cupy#2171 (won't be merged until NumPy 1.17 is released). I'm leaving this open for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.