DLPack overhaul #5661
Merged
DLPack overhaul #5661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI MESSAGE: [19098946]: BUILD STARTED |
banasraf
reviewed
Oct 7, 2024
dali/pipeline/data/dltensor.h
Outdated
| if (dl_type.code < std::size(code_str)) | ||
| ss << code_str[dl_type.code]; | ||
| else | ||
| ss << "<unknown" << dl_type.code + 0 << ">"; |
There was a problem hiding this comment.
Suggested change
| ss << "<unknown" << dl_type.code + 0 << ">"; | |
| ss << "<unknown:" << dl_type.code + 0 << ">"; |
banasraf
reviewed
Oct 7, 2024
dali/pipeline/data/dltensor.h
Outdated
| * | ||
| * The text representation looks like: | ||
| * <type><bits>[x<lanes>] | ||
| * with <lanes>x present only if the number of lanes is > 1 |
There was a problem hiding this comment.
Suggested change
| * with <lanes>x present only if the number of lanes is > 1 | |
| * with x<lanes> present only if the number of lanes is > 1 |
banasraf
reviewed
Oct 7, 2024
dali/pipeline/data/dltensor.h
Outdated
| * +-- ... | ||
| * ``` | ||
| * | ||
| * You can use any payload structure of your choice, but it must privde the storage for DLTensor's |
There was a problem hiding this comment.
Suggested change
| * You can use any payload structure of your choice, but it must privde the storage for DLTensor's | |
| * You can use any payload structure of your choice, but it must provide the storage for DLTensor's |
banasraf
approved these changes
Oct 7, 2024
|
CI MESSAGE: [19098946]: BUILD PASSED |
- Remove inheritance. - Make more stuff inline. - Make function names consistent. - Add DLPack resources which share tensor ownership - Add reference counting tests. - Add CPU pinned memory support. TODO(michalz): Unify with JAX. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
ef4502c
to
9529a5a
Compare
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [19136647]: BUILD STARTED |
|
CI MESSAGE: [19136647]: BUILD PASSED |
klecki
reviewed
Oct 9, 2024
| @@ -19,18 +19,18 @@ | |||
|
|
|||
| namespace dali { | |||
|
|
|||
| DLDataType GetDLType(DALIDataType type) { | |||
| DLDataType ToDLType(DALIDataType type) { | |||
There was a problem hiding this comment.
Just a side note, the TYPE_SWITCH here seems like an overkill.
There was a problem hiding this comment.
Perhaps, but then again it's almost certainly faster than using TypeTable.
dali/pipeline/data/dltensor.h
Outdated
Comment on lines
287
to
296
| template <typename Backend> | ||
| std::vector<DLMTensorPtr> GetSharedDLTensorList(TensorList<Backend> &tensor_list) { | ||
| int device_id = tensor_list.device_id(); | ||
| bool pinned = tensor_list.is_pinned(); | ||
|
|
||
| std::vector<DLMTensorPtr> dl_tensors{}; | ||
| dl_tensors.reserve(tensor_list.num_samples()); | ||
|
|
||
| for (int i = 0; i < tensor_list.num_samples(); ++i) | ||
| dl_tensors.push_back(GetDLTensorView(tensor_list[i], pinned, device_id)); |
There was a problem hiding this comment.
Looks like a copy-paste for GetDLTensorListView. It probably should use unsafe_sample_owner.
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [19179834]: BUILD STARTED |
klecki
approved these changes
Oct 9, 2024
|
CI MESSAGE: [19179834]: BUILD PASSED |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Category:
New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)
Description:
This PR reworks the DLPack support in DALI.
New features:
Refactoring:
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Existing tests: ExternalSource, PythonFunction, JAX, dali_test.bin:DL
New tests: buffer sharing (reference counting test).
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A