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

Make DALI array_interface memory writable #4800

Merged
merged 1 commit into from Apr 24, 2023

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Apr 21, 2023

  • marking DALI memory shared by array_interface read-only prevents it
    from sharing directly with Torch and possibly other libraries.
    This PR makes it writable.
  • memory can be shared anyway but it requires an intermediate step
    through cupy which makes the memory writable

Category:

Other*/Breaking change(I hope not)

Description:

  • marking DALI memory shared by array_interface read-only prevents it
    from sharing directly with Torch and possibly other libraries.
    This PR makes it writable.
  • memory can be shared anyway but it requires an intermediate step
    through cupy which makes the memory writable

Additional information:

Affected modules and functionalities:

  • tensor cuda_array_interface and array_interface

Key points relevant for the review:

  • NA

Tests:

  • Existing tests apply
    • adjusted test_array_interface_tensor_cpu and test_cuda_array_interface_tensor_gpu
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@JanuszL
Copy link
Contributor Author

JanuszL commented Apr 21, 2023

!build

@@ -63,7 +63,7 @@ def test_dlpack_tensor_list_gpu_to_cpu():


def check_dlpack_types_gpu(t):
arr = torch.tensor([[-0.39, 1.5], [-1.5, 0.33]], device="cuda", dtype=t)
arr = torch.tensor([[0.39, 1.5], [1.5, 0.33]], device="cuda", dtype=t)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest torch complains value cannot be converted to type uint8 without overflow.

@@ -149,7 +149,7 @@ def create_tmp(idx):


def check_dlpack_types_cpu(t):
arr = torch.tensor([[-0.39, 1.5], [-1.5, 0.33]], device="cpu", dtype=t)
arr = torch.tensor([[0.39, 1.5], [1.5, 0.33]], device="cpu", dtype=t)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest torch complains value cannot be converted to type uint8 without overflow.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8039230]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8039230]: BUILD PASSED

@mzient
Copy link
Contributor

mzient commented Apr 24, 2023

Shouldn't it be "writable" in the title?

@mzient mzient self-assigned this Apr 24, 2023
- marking DALI memory shared by array_interface read-only prevents it
  from sharing directly with Torch and possibly other libraries.
  This PR makes it writable.
- memory can be shared anyway but it requires an intermediate step
  through cupy which makes the memory writable

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL JanuszL changed the title Make DALI array_interface memory readable Make DALI array_interface memory writable Apr 24, 2023
@JanuszL
Copy link
Contributor Author

JanuszL commented Apr 24, 2023

Shouldn't it be "writable" in the title?

Fixed

@klecki
Copy link
Contributor

klecki commented Apr 24, 2023

Just a question: when converting form one Tensor type to another, like when we do TensorList -> Tensor, we typically pack the shared_ptr to the memory somewhere. Here, I don't think there is even Python object reference to the original tensor. Wouldn't it crash in an iteration or two in most cases?

@JanuszL
Copy link
Contributor Author

JanuszL commented Apr 24, 2023

Just a question: when converting form one Tensor type to another, like when we do TensorList -> Tensor, we typically pack the shared_ptr to the memory somewhere. Here, I don't think there is even Python object reference to the original tensor. Wouldn't it crash in an iteration or two in most cases?

I don't think the pipeline exposes a non-continuous tensor as the output. In the general case, it may not work, in most cases, we just expose the pointer to the underlying, continuous memory, so the tensor itself doesn't have to live long.
The problem you describes is rather about the conversion from TensorList to Tensor itself, and possible problems of the array_interface are just a derivate of it.

@klecki
Copy link
Contributor

klecki commented Apr 24, 2023

The problem I'm referring to is specifically how long the allocation is valid. But thinking about it, the array interface is just a property of the object, and accessing it doesn't impact the lifetime of the original object. So the user of array interface must take lifetimes into account and keep the original object alive as long as they are using the raw pointer that they obtain. I think we should be fine, until someone keeps this pointer for too long thinking it contents won't change. I don't know what Pytorch may do with such memory.

@JanuszL
Copy link
Contributor Author

JanuszL commented Apr 24, 2023

The problem I'm referring to is specifically how long the allocation is valid. But thinking about it, the array interface is just a property of the object, and accessing it doesn't impact the lifetime of the original object. So the user of array interface must take lifetimes into account and keep the original object alive as long as they are using the raw pointer that they obtain. I think we should be fine, until someone keeps this pointer for too long thinking it contents won't change. I don't know what Pytorch may do with such memory.

Yes, that is what I had in mind. I think PyTorch just wraps the memory up and does nothing with it. After all, it was possible already but with cupy in the middle.

@klecki
Copy link
Contributor

klecki commented Apr 24, 2023

Also, reading the docs of array_interface:

A reference to the object exposing the array interface must be stored by the new object if the memory area is to be secured.

I think it solves my issues.
BTW @JanuszL, maybe you want to follow up with CUDA array interface v3? https://numba.readthedocs.io/en/stable/cuda/cuda_array_interface.html

@JanuszL
Copy link
Contributor Author

JanuszL commented Apr 24, 2023

I guess that we want to use the new interface at some point.

@JanuszL JanuszL merged commit cabb4fd into NVIDIA:main Apr 24, 2023
3 of 4 checks passed
@JanuszL JanuszL deleted the modifable_arra_intf branch April 24, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants