[Feature] Bump DLPack to v0.7

[yaox12]

Description

Resolve #4268.

What's changed

1. Rename all DLContext to DGLContext and define our own DGLContext struct.
2. Define our own DGLDeviceType enum and rename all kDLGPU to kDLCUDA.
3. Define DGLArray as a substitute for DLTensor.
4. Unify DGLType and DLDataType to DGLDataType.
5. Rename all DLDeviceType to DGLDeviceType and define our own DGLDeviceType struct.
6. Move all DLPack conversion methods and APIs to include/dgl/runtime/dl_converter.h and src/runtime/dl_converter.cc.
7. Corresponding changes in FFI.

Key files to review

1. include/dgl/runtime/c_runtime_api.h
2. src/runtime/ndarray.cc
3. python/dgl/_ffi/_cython/base.pxi
4. include/dgl/runtime/dl_converter.h
5. src/runtime/dl_converter.cc

Checklist

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage
• Code is well-documented
• To the best of my knowledge, examples are either not affected by this change,
  or have been fixed to be compatible with this change
• Related issue is referred in this PR

[dgl-bot]

To trigger regression tests:

• @dgl-bot run [instance-type] [which tests] [compare-with-branch];
  For example: @dgl-bot run g4dn.4xlarge all dmlc/master or @dgl-bot run c5.9xlarge kernel,api dmlc/master

[yaox12]

BTW, I feel the coexistence and mixed use of DGLType/DLDataType, and DGLContext/DLDevice(DLContext), reduce the readability of code.

[yaox12]

CI tests with PyTorch nightly passed as well.

[jermainewang]

Pasted my comments from the linked issue:

@yaox12 I think the preferred way of implementation is to decouple dgl::runtime::NDArray with DLPack struct. Currently, it internally holds a DLPack pointer. To avoid potential huge refactor moving forward, I think we should use our own structure so that ->device or ->ctx won't matter. The ToDLPack and FromDLPack functions should be responsible of getting/setting attributes from/to DLPack structs.

The other benefit is that we can then support different versions of DLPack easily. They will be limited only to the conversion function.

[yaox12]

@jermainewang Generally agree. That's what PyTorch has been doing.

[jermainewang]

As this PR could result in many conflicts, I suggest we move fast on this.

Renaming all DLContext to DGLContext and then typedef DLDevice DGLContext;

Instead of renaming, let's create our own DGLContext struct.

Renaming all .ctx/->ctx to .device/->device

We can revert these changes because it won't matter once we have our own struct. The ctx name also corresponds to DGLContext.

Renaming all kDLGPU to kDLCUDA, and renaming one kDLCPUPinned to kDLCUDAHost (unreachable code).

I'm fine with this.

[yaox12]

@jermainewang

Attributes of DLContext didn't change when being renamed to DLDevice. So I think renaming all DLContext to DGLContext and then typedef DLDevice DGLContext; should be fine.

What's changed and breaks the current code is the DLTensor struct. DLTensor.ctx has been renamed to DLTensor.device. Instead of typedef DLTensor DGLArray;, we should create our own DGLArray struct with DGLContext ctx; as a member.

[yaox12]

@jermainewang Changes on the C++ side should be OK now, but I'm lost in the Cython FFI, for example, duplicated DGLContext defined in dgl/runtime/c_runtime_api.h and python/dgl/_ffi/runtime_ctypes.py, replacing DLTensor with DGLArray, etc...

[BarclayII]

@jermainewang Changes on the C++ side should be OK now, but I'm lost in the Cython FFI, for example, duplicated DGLContext defined in dgl/runtime/c_runtime_api.h and python/dgl/_ffi/runtime_ctypes.py, replacing DLTensor with DGLArray, etc...

You actually mentioned both Cython FFI and ctypes FFI.

According to https://cython.readthedocs.io/en/latest/src/userguide/external_C_code.html#referencing-c-header-files Cython will directly include the header file instead of declaring its own. So I think it's better to change the names or otherwise Cython will still try to use the old DLContext and DLTensor from the C++ header files, which may or may not work.

ctypes FFI should be fine as long as the memory layout stays the same.

[yaox12]

@jermainewang Changes on the C++ side should be OK now, but I'm lost in the Cython FFI, for example, duplicated DGLContext defined in dgl/runtime/c_runtime_api.h and python/dgl/_ffi/runtime_ctypes.py, replacing DLTensor with DGLArray, etc...

You actually mentioned both Cython FFI and ctypes FFI.

According to https://cython.readthedocs.io/en/latest/src/userguide/external_C_code.html#referencing-c-header-files Cython will directly include the header file instead of declaring its own. So I think it's better to change the names or otherwise Cython will still try to use the old DLContext and DLTensor from the C++ header files, which may or may not work.

ctypes FFI should be fine as long as the memory layout stays the same.

If we replace DLContext with DGLContext in C++, then in Cython FFI, this line

dgl/python/dgl/_ffi/_cython/base.pxi

Line 32 in ab0af81

ctypedef struct DLContext:

used in

dgl/python/dgl/_ffi/_cython/function.pxi

Line 116 in ab0af81

value[0].v_ctx = (<DLContext*>(

will have a name conflict with

dgl/python/dgl/_ffi/_cython/function.pxi

Line 7 in ab0af81

from ..runtime_ctypes import DGLType, DGLContext, DGLByteArray

---

Update: I have talked with Minjie and will update the code accordingly.

[yaox12]

@jermainewang I think this PR is ready for review.

---

Update: Pushed the changes that decoupled dlpack from NDArray.

[jermainewang]

For L101-102, can we just do

DGLArray* tensor = arr.data_->dl_tensor?

[yaox12]

It's a bit tricky here. We use reinterpret_cast here because we want to keep this NDArray::Container object (but treat its former part as a DGLArray), so that when a DGLArray object is passed from Python to C++, we can reinterpret_cast it back to an NDArray::Container, for example, in

dgl/src/runtime/ndarray.cc

Lines 499 to 502 in 36dbcce

	int DGLArrayToDLPack(DGLArrayHandle from, DLManagedTensor** out,
	int alignment) {
	API_BEGIN();
	auto* nd_container = reinterpret_cast<NDArray::Container*>(from);

This is also why we don't free arr.data_.

I don't know why it looks like this at the beginning, but this is how it behaves/works now...

[jermainewang]

I see. The design right now is complicated. You change is fine as it doesn't modify the original behavior. Let's simplify it later in another PR.

[jermainewang]

I think the core issue is still that NDArray::Container is coupled too tightly with DLManagedTensor. In the ideal case, DLManagedTensor should be transient object whose lifescycle is limited to the DLPack data exchange.

[jermainewang]

There seems to be a memory leak here. The data_ pointer in the NDArray is not freed before being set to nullptr.

[yaox12]

See above.

[jermainewang]

Rename this file (and the cc file) to dlpack_convert.h|cc

[yaox12]

I'll rename them after all the other comments are addressed.

[jermainewang]

Thanks for the great effort! @yaox12

With this change, I found the NDArray::Container quite redundant and can be merged with the new DGLArray. This will further simplify the logic (right now there are DGLArray, NDArray::Container and NDArray). What do you think? Of course, I won't suggest making the change in this PR as it's already quite huge. We can create a follow-up work item for this if you also think this is benefitial.

[yaox12]

Thanks for the great effort! @yaox12

With this change, I found the NDArray::Container quite redundant and can be merged with the new DGLArray. This will further simplify the logic (right now there are DGLArray, NDArray::Container and NDArray). What do you think? Of course, I won't suggest making the change in this PR as it's already quite huge. We can create a follow-up work item for this if you also think this is benefitial.

Totally Agree.

[jermainewang]

LGTM

[jermainewang]

@yaox12 I've approved the change. One last thing is to rename the dl_converter file. After that, please merge it. Thanks!

[dgl-bot]

Commit ID: 74a95ac

Build ID: 20

Status: ✅ CI test succeeded

Report path: link

Full logs path: link