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
Automatically move y
to the same device and namespace as X
#28668
Comments
After thinking about this for a bit I think I like the idea. It solves a problem for which we haven't found a good solution in a while now (how to move from one device to another). I think allowing So right now I'm +1 |
y
to same device and namespace as X
y
to the same device and namespace as X
+1 in general, and even more so to make it possible to train classifier pipelines on a GPU with str object-typed class labels passed to the pipeline as we usually allow. |
Note, on the implementation side, to move NumPy arrays and Python lists to a specific array namespace/device combo, But if the things to move are already already on another namespace/device combo, that will most likely fail in general, for instance with torch: >>> import array_api_strict as xp_strict
>>> import array_api_compat.torch as xp_torch
>>> reference = xp_strict.ones(3)
>>> reference.device
CPU_DEVICE
>>> xp_strict.asarray(xp_torch.ones(3, device="mps"), device=reference.device)
Traceback (most recent call last):
Cell In[49], line 1
xp_strict.asarray(xp_torch.ones(3, device="mps"), device=reference.device)
File ~/miniforge3/envs/dev/lib/python3.11/site-packages/array_api_strict/_creation_functions.py:91 in asarray
res = np.array(obj, dtype=_np_dtype, copy=copy)
File ~/miniforge3/envs/dev/lib/python3.11/site-packages/torch/_tensor.py:1030 in __array__
return self.numpy()
TypeError: can't convert mps:0 device type tensor to numpy. Use Tensor.cpu() to copy the tensor to host memory first. In this case, we should instead use the new improvements made to the dlpack and Array API specs:
https://github.com/dmlc/dlpack/blob/main/docs/source/python_spec.rst But note that there is no guarantee that cross-device transfers work in general, so we might always have to move through a CPU device if needed. Furthermore, this is not yet implemented in data-apis/array-api-compat#86 (comment) In the mean time we will have to rely on our For now we can decide to implement the "auto-follow" thing only when X is a namespace and |
A priori, I‘m +1 of only allowing X and y on the same device. I‘m also +1 to allow y on a different device and scikit-learn moving it to the device of X. But only if it is clearly documented. We could even think to start with a warning (that we can remove later.) |
(From #27800 (comment) by @ogrisel)
The proposal/idea is to allow
y
to not be on the same device (and namespace?) asX
when using Array API inputs. Currently we require/assume that they are on the same device and namespace, it is a requirement. However pipelines can't modifyy
which means it is not possible to move from CPU to GPU as one of the steps of the pipeline, the whole pipeline has to stay on one device. The below details some example and code to motivate allowingX
ady
being on different devices (and namespaces).Suppose we have:
I did a quick benchmark with timeit on a host with a 32 cores CPU and an A100 GPU: we get a bit more than 10x speed-up (which is in the range of what I would have expected):
I also tried the following:
I think it might be reasonable to expect this pattern to fail in general. explicitly ask the the user to provide inputs with consistently allocated data buffers.
However, we might want to be more lenient for the particular case of
y
(andsample_weight
) and changeRidge.fit
to automatically movey
to the same namespace and device asX
:The reason would be to improve the usability of the Array API for the following pipelines:
The pipeline steps can only transform
X
and noty
(orsample_weight
). So it means that user would have to instead call:which feels a bit weird/cumbersome to me.
This might not be a big deal though and I don't want to delay this PR to get a consensus on this particular point: I think it's fine the way it is for now but we might want come back to the UX of pipelines with Array API steps later. I will open a dedicated issue.
If we take action there are a few estimators that need double checking. For example
LinearDiscriminantAnalysis
currently raises low level PyTorch exceptions whenX
andy
are on different devices.The text was updated successfully, but these errors were encountered: