Skip to content

API: Specify that DLPack should use BufferError #498

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

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Oct 20, 2022

I would actually like MUST, but since most implementations currently don't actually give a BufferError, writing it as SHOULD instead.

First part of closing numpy/numpy#20742

I would actually like MUST, but since most implementations currently don't
actually give a ``BufferError``, writing it as SHOULD instead.

First part of closing numpy/numpy#20742
@oleksandr-pavlyk
Copy link
Contributor

@seberg I assume that this recommendation does not fix the namespace for the actual exception object, right?

@seberg
Copy link
Contributor Author

seberg commented Oct 20, 2022

BufferError is a python builtin.

@leofang leofang self-requested a review October 20, 2022 14:39
@oleksandr-pavlyk
Copy link
Contributor

While DLPack is a buffer conceptually, it is not the buffer as in Py_buffer. Why not introduce dedicated DLPackCreationError in array namespace?

Here is what I had in my POC:

Raises:
    DLPackCreationError: when array can be represented as
        DLPack tensor. This may happen when array was allocated
        on a partitioned sycl device, or its USM allocation is
        not bound to the platform default SYCL context.
    MemoryError: when host allocation to needed for `DLManagedTensor`
        did not succeed.
    ValueError: when array elements data type could not be represented
        in `DLManagedTensor`.

@seberg
Copy link
Contributor Author

seberg commented Oct 20, 2022

Because then we have to create a new class and put it somewhere? The standard bails on defining most errors, but having a BufferError was useful at least once (mpi4py can retry with the buffer protocol/__array_interface__).

BufferError seems decent for that, it signals the same thing as it does for the Python buffer interface: Export is not possible as requested. And it seems exceedingly unlikely that BufferError is triggered by something else during __dlpack__.

@kgryte
Copy link
Contributor

kgryte commented Nov 3, 2022

Based on 11/3/2022 meeting, we are good moving forward with this PR.

Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@seberg Removed a duplicated word. Otherwise, LGTM.

@leofang leofang added this to the v2022 milestone Nov 4, 2022
seberg added a commit to seberg/numpy that referenced this pull request Nov 7, 2022
See also data-apis/array-api#498.

I think we should just change this.  It is a niche feature and just
an error type change.

Closes numpygh-20742
seberg added a commit to seberg/numpy that referenced this pull request Nov 7, 2022
See also data-apis/array-api#498.

I think we should just change this.  It is a niche feature and just
an error type change.

Closes numpygh-20742
@kgryte kgryte added topic: DLPack DLPack. API change Changes to existing functions or objects in the API. labels Nov 14, 2022
@kgryte
Copy link
Contributor

kgryte commented Nov 14, 2022

This has received two approvals and was discussed in workgroup meetings. Will merge.

@kgryte kgryte merged commit 7a92139 into data-apis:main Nov 14, 2022
@seberg seberg deleted the BufferError branch November 15, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: DLPack DLPack.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants