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

[REVIEW] DLPack Support #913

Merged
merged 26 commits into from Mar 11, 2019
Merged

[REVIEW] DLPack Support #913

merged 26 commits into from Mar 11, 2019

Conversation

@harrism
Copy link
Member

@harrism harrism commented Feb 10, 2019

This PR adds initial support for converting between cuDF DataFrame (or gdf_column) and DLPack DLTensor. https://github.com/dmlc/dlpack

Closes #912.

Initial support is for converting between 1D DLTensor and gdf_column/DataFrame.

  • Add DLPack submodule
  • Add C API gdf_from_dlpack()
  • Add C API gdf_to_dlpack()
  • Add C++ API Google Tests for above
  • Add Python cuDF DataFrame.from_dlpack() and DataFrame.to_dlpack() APIs
  • Add pytests for above

Python / Cython work will move to a separate PR. Tracked by #1046

@harrism harrism self-assigned this Feb 10, 2019
@harrism harrism added this to PR-WIP in v0.6 Release via automation Feb 10, 2019
@harrism harrism requested a review from jrhemstad Mar 6, 2019
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Biggest issues are just about clarifying and documenting the desired behavior of the conversion functions and trying to minimize user surprise about who owns what data and what data is safe to use.

cpp/include/cudf/functions.h Outdated Show resolved Hide resolved
cpp/include/cudf/functions.h Outdated Show resolved Hide resolved
}

// Call the managed tensor's deleter since our "borrowing" is done
tensor->deleter(const_cast<DLManagedTensor*>(tensor));
Copy link
Contributor

@jrhemstad jrhemstad Mar 6, 2019

I'm confused about this.

I assume this is deleting the tensor and freeing it's allocation.

As such, as a user of this function, I would be surprised that a function to copy a DLPack tensor into gdf_columns is also deleting/freeing the tensor.

I would expect it to just do the allocation of the gdf_columns and do the copies and leave my Tensor alone, especially since it is being passed in as const I would doubly expect it to not be modified (but you're casting away the const here).

If this is desired behavior, I would be sure to add in the documentation that the tensor will be deleted and remove the const qualifier to prevent confusion.

Copy link
Member Author

@harrism harrism Mar 6, 2019

The documentation on this from dlpack is limited. I think it should really be called release rather than deleter. Here is what the doc comment for DLManagedTensor says:

/*!
 * \brief C Tensor object, manage memory of DLTensor. This data structure is
 *  intended to faciliate the borrowing of DLTensor by another framework. It is
 *  not meant to transfer the tensor. When the borrowing framework doesn't need
 *  the tensor, it should call the deleter to notify the host that the resource
 *  is no longer needed.
 */

https://github.com/dmlc/dlpack/blob/5c792cef3aee54ad8b7000111c9dc1797f327b59/include/dlpack/dlpack.h#L149-L155

Copy link
Contributor

@jrhemstad jrhemstad Mar 7, 2019

Ah, okay. Then I just misunderstood the semantics of the Tensor object here. It sounds like this object doesn't actually "own" any memory and calling it's deleter doesn't really delete anything important.

In that case, I think you're probably doing the right thing here.

Copy link
Member Author

@harrism harrism Mar 7, 2019

I guess in the future, our deleters (in to_dlpack), will probably decrement a refcount which may lead to deletion of the memory, or may not if a reference is still owned (say by the original column).

Copy link
Collaborator

@kkraus14 kkraus14 Mar 11, 2019

I believe this is causing a double free segfault for me in working with CuPy:

#0  0x00007ffff6a1f512 in free () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007fff7dc92f5e in __pyx_f_4cupy_4core_6dlpack_deleter (__pyx_v_tensor=0x2b60c6c0) at cupy/core/dlpack.cpp:2778
#2  0x00007fff7dc949cb in __pyx_f_4cupy_4core_6dlpack_pycapsule_deleter (__pyx_v_dltensor=<optimized out>)
    at cupy/core/dlpack.cpp:2705
#3  0x00007ffff796d7df in capsule_dealloc (o=0x7fff7c705600) at Objects/capsule.c:261
...

Copy link
Collaborator

@kkraus14 kkraus14 Mar 11, 2019

The function CuPy registers with DLPack definitely looks to free the memory:

https://github.com/cupy/cupy/pull/1082/files#diff-ddf01ff512087ef616db57ecab88c6aeR78

cpp/src/io/convert/dlpack/cudf_dlpack.cpp Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Outdated Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Outdated Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Outdated Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Outdated Show resolved Hide resolved
cpp/src/io/convert/dlpack/cudf_dlpack.cpp Outdated Show resolved Hide resolved
v0.6 Release automation moved this from PR-WIP to PR-Needs review Mar 6, 2019
cpp/tests/io/convert/dlpack_test.cu Outdated Show resolved Hide resolved
@harrism
Copy link
Member Author

@harrism harrism commented Mar 7, 2019

rerun tests

@kkraus14
Copy link
Collaborator

@kkraus14 kkraus14 commented Mar 7, 2019

@harrism I think we can ignore those 4 hung tests for now. Remnant of the CI issue.

@harrism
Copy link
Member Author

@harrism harrism commented Mar 7, 2019

@jrhemstad As soon as you approve we can merge.

v0.6 Release automation moved this from PR-Needs review to PR-Reviewer approved Mar 7, 2019
@harrism harrism moved this from PR-Reviewer approved to PR-WIP in v0.6 Release Mar 8, 2019
@harrism harrism merged commit 1c8a48f into rapidsai:branch-0.6 Mar 11, 2019
8 of 12 checks passed
v0.6 Release automation moved this from PR-WIP to Done Mar 11, 2019
@harrism harrism changed the title [WIP] DLPack Support [REVIEW] DLPack Support Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v0.6 Release
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants