ENH: Implement DLPack version 1 by seberg · Pull Request #26501 · numpy/numpy

seberg · 2024-05-22T13:53:16Z

This implements DLPack version 1 to close gh-26411.

Note that I had first tried to use C++ templating to avoid the duplication, but in the end the additional complexity of it didn't really seem worthwhile compared to duplicating the few functions.

Although a lot is just moved around, this is an unfortunately large diff. The new and old code paths should be (mostly) tested, but the nature of dlpack is that it is hard to test interaction with others (e.g. gpu failure paths).

I decided to always pass device/copy as None to simplify the call sites.

It does fix a few smaller issues as well.

seberg · 2024-05-22T13:54:12Z

I had started with "could backport this", but I am not sure as it isn't trivial. Anyway, i still needs release notes and all that changes would be whether it we write 2.1 or 2.2 in a few places.

seberg · 2024-05-22T13:55:48Z

numpy/_core/src/multiarray/dlpack.c

-{
-    /* an exception may be in-flight, we must save it in case we create another one */
-    PyObject *type, *value, *traceback;
-    PyErr_Fetch(&type, &value, &traceback);


This was always unnecessary, just missed that it was also here when removing it in the version above...

seberg · 2024-05-22T13:56:59Z

numpy/_core/src/multiarray/dlpack.c

-    }
-
-    // ensure alignment
-    int offset = sizeof(DLManagedTensor) % sizeof(void *);


This offset calculation was just wrong. It simply didn't matter because the struct size is a multiple of this anyway in practice.

seberg · 2024-05-22T13:58:01Z

numpy/_core/src/multiarray/dlpack.c

+     * can then be removed again.
+     */
+    PyObject *res = create_dlpack_capsule(
+            self, major_version >= 1, &result_device);


Maybe to explain, the result_device is checked for validity already during argument parsing, so it's OK to just adopt it.
No matter what device we have, if NumPy supports it, we can re-export it as CPU.

mattip · 2024-05-23T06:10:03Z

I think the failing test is fixed in #26503

mattip

This could use a release note.

It seems the major change is supporting both DLManagedTensor and DLManagedTensorVersioned, but then I am confused since the DLManagedTensorVersioned struct already existed. Could you explain what is going on to help reviewers?

mattip · 2024-05-23T06:12:25Z

numpy/_core/src/common/dlpack/dlpack.h

   *
   * Note that as of Nov 2021, multiply libraries (CuPy, PyTorch, TensorFlow,
-   * TVM, perhaps others) do not adhere to this 256 byte alignment requirement
+   * TVM, perhaps others) do not adhere to this 256 byte aligment requirement


Typo, alignment is correct

I changed it, but since it is vendored, I don't think it matters.

seberg · 2024-05-23T10:54:22Z

ut then I am confused since the DLManagedTensorVersioned struct already existed.

I supsect someone copied over a version of the header that already included it just to add bool support: Nothing was actually done about it (or could it have been at the time).

mattip

Seems to work. It is a bit difficult to review because of the refactoring, but I think it all makes sense. I do wonder a bit about the GIL being needed in the various deleter functions since the they can be called by non-python code.

mattip · 2024-05-30T06:39:34Z

numpy/_core/src/common/dlpack/dlpack.h

 // Taken from:
-// https://github.com/dmlc/dlpack/blob/ca4d00ad3e2e0f410eeab3264d21b8a39397f362/include/dlpack/dlpack.h
+// https://github.com/dmlc/dlpack/blob/bbd2f4d32427e548797929af08cfe2a9cbb3cf12/include/dlpack/dlpack.h
+// but added typedef to DLManagedTensorVersioned


Should we upstream that change?

Probably, I mentioned it in an issue there.

mattip · 2024-05-30T07:09:27Z

numpy/_core/src/multiarray/dlpack.c

+        (DLManagedTensorVersioned *)PyCapsule_GetPointer(
+            self, NPY_DLPACK_VERSIONED_CAPSULE_NAME);
    if (managed == NULL) {
-        PyErr_WriteUnraisable(self);


Doesn't this need the GIL?

No, a capsule is a Python object, so it can't be deleted without the GIL. It is just the DLManagedTensor deleter that needs to grab the GIL.

mattip · 2024-05-30T07:18:27Z

numpy/_core/src/multiarray/multiarraymodule.c

        "Give a warning on reload and big warning in sub-interpreters."},
    {"from_dlpack", (PyCFunction)from_dlpack,
-        METH_O, NULL},
+        METH_FASTCALL | METH_KEYWORDS, NULL},


Using fastcall - nice.

mattip · 2024-05-30T07:21:42Z

numpy/_core/tests/test_dlpack.py

+
+        with pytest.raises(RuntimeError):
+            x.__dlpack__(stream=1)
+


Did you mean to test copy here?

Strange, yeah, seems like I copied it, but then didn't adapt it

Fixed, copy= parsing is too forgiving, IMO, but I opened an issue on that. It doesn't matter here.

seberg · 2024-05-30T11:09:43Z

Forgot to set the IS_COPIED flag; if we copy (which we could refuse probably) then need to set it. No way to test it within NumPy though without adding another unpacker for the capsule, it's easy enough.

mattip · 2024-06-05T06:52:08Z

Thanks @seberg

seberg added 2 commits May 22, 2024 14:24

ENH: Support dlpack version 1 ABI

fb60521

Fixups and support dl_device and copy kwargs

5324283

seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 22, 2024

github-actions bot added the 01 - Enhancement label May 22, 2024

seberg commented May 22, 2024

View reviewed changes

Small fixups for linter/compiler warnings

902e9b1

mattip reviewed May 23, 2024

View reviewed changes

DOC: Fix typo in vendored code and add release fragment for dlpack v1

64570d1

mattip removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 29, 2024

mattip reviewed May 30, 2024

View reviewed changes

seberg added 2 commits May 30, 2024 12:51

TST: Fix copy __dlpack__ test and also test copy=False

e86cca6

BUG: Should indicate copied if we can

fde3a37

mattip merged commit 29409dc into numpy:main Jun 5, 2024

seberg deleted the dlpack-v1 branch June 5, 2024 13:13

mtsokol mentioned this pull request Jun 26, 2024

MNT: Update dlpack docs and typing stubs #26799

Merged

leofang mentioned this pull request Sep 9, 2024

Tag an official v1.0 release dmlc/dlpack#149

Closed

lucyleeow mentioned this pull request Jun 30, 2025

Automatically move y (and sample_weight) to the same device and namespace as X scikit-learn/scikit-learn#28668

Open

Uh oh!

Conversation

seberg commented May 22, 2024

Uh oh!

seberg commented May 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented May 23, 2024

Uh oh!

mattip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented May 23, 2024

Uh oh!

mattip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented May 30, 2024

Uh oh!

mattip commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants