C API 2.0 Tensor and TensorList #5799

mzient · 2025-01-31T16:50:21Z

Category:

New feature (non-breaking change which adds functionality)

Description:

This PR adds wrappers for Tensor and TensorList that are later used to implement the C interface for those objects.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

szkarpinski

Leaving some initial comments

szkarpinski · 2025-02-10T10:58:37Z

dali/c_api_2/data_objects.h

+// Interfaces
+//////////////////////////////////////////////////////////////////////////////
+
+class ITensor : public _DALITensor, public RefCountedObject {


Do we use this ISomething convention in the code elsewhere?

dali/c_api_2/data_objects.h

szkarpinski · 2025-02-10T11:16:51Z

dali/c_api_2/data_objects_test.cc

@@ -0,0 +1,414 @@
+// Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Are you testing non-contiguous tensor lists here?

I'm testing daliTensorListAttachBuffer with custom offsets.
The test for daliTensorListAttachSamples is missing. I'll add.

dali/c_api_2/ref_counting.h

banasraf · 2025-02-16T12:14:07Z

dali/c_api_2/data_objects.cc

+}
+
+//////////////////////////////////////////////////////////////////////////////
+// Tensor


It's missing impl for daliTensorGetShape, right?

It was missing; added.

@mzient also missing:
daliTensorGetBufferPlacement
daliTensorListGetBufferPlacement
and source methods
daliTensorGetSourceInfo
daliTensorListGetSourceInfo

Done. Also added setters.

szkarpinski

Looks good overall.

My only major concern is the lack of documentation. Some of the functions perform complex operations details of which are not clear from their signatures. Even a minimalistic doxygen comments would help a lot.

szkarpinski · 2025-02-24T15:32:03Z

dali/c_api_2/data_objects.h

+  virtual const TensorShape<> &GetShape() const & = 0;
+
+  template <typename Backend>
+  const std::shared_ptr<Tensor<Backend>> &Unwrap() const &;


Some function names are self-explanatory, but some are not obvious. How about adding some minimal docstrings?

szkarpinski · 2025-02-24T15:37:43Z

dali/c_api_2/data_objects.h

+        throw std::invalid_argument(
+          "The number of dimensions must not be negative when num_samples is 0.");
+      else
+        ndim = samples[0].ndim;


This is a good feature, but definitely should be documented

It is. See daliTensorListAttachSamples:

* @param ndim the number of dimensions in each sample; * if num_samples > 0, this value can be set to -1 and the number of * dimensions will be taken from samples[0].ndim

szkarpinski · 2025-02-24T15:38:32Z

dali/c_api_2/data_objects.h

+      throw std::out_of_range(make_string("The sample index ", sample, " is out of range. "
+        "Valid indices are [0..", shape.num_samples() - 1, "]."));


Nitpick: This will look strange for 0 samples

Signed-off-by: Michał Zientkiewicz <[email protected]>

Signed-off-by: Michal Zientkiewicz <[email protected]>

* Add generic validation. * Move ToOptional to a new utils.h header. Signed-off-by: Michal Zientkiewicz <[email protected]>

Signed-off-by: Michal Zientkiewicz <[email protected]>

mzient · 2025-02-24T17:15:16Z

Looks good overall.

My only major concern is the lack of documentation. Some of the functions perform complex operations details of which are not clear from their signatures. Even a minimalistic doxygen comments would help a lot.

Would it be enough to refer to the relevant C API function? Those are quite thoroughly documented.

Signed-off-by: Michal Zientkiewicz <[email protected]>

dali-automaton · 2025-02-24T17:31:01Z

CI MESSAGE: [24487279]: BUILD STARTED

dali-automaton · 2025-02-25T04:30:36Z

CI MESSAGE: [24487279]: BUILD PASSED

mzient mentioned this pull request Jan 31, 2025

C API 2.0 helpers #5798

Merged

18 tasks

mzient force-pushed the C_API2_data_objects branch from 04ef2a5 to e27aa11 Compare February 1, 2025 15:50

dali-automaton assigned stiepan, szalpal and szkarpinski Feb 3, 2025

szkarpinski reviewed Feb 10, 2025

View reviewed changes

mzient force-pushed the C_API2_data_objects branch from e27aa11 to 361bf44 Compare February 10, 2025 15:00

banasraf reviewed Feb 12, 2025

View reviewed changes

dali/c_api_2/ref_counting.h Outdated Show resolved Hide resolved

mzient force-pushed the C_API2_data_objects branch 3 times, most recently from b0511b2 to 539de4e Compare February 13, 2025 12:01

banasraf reviewed Feb 16, 2025

View reviewed changes

mzient force-pushed the C_API2_data_objects branch 5 times, most recently from bbbca35 to 28a3201 Compare February 20, 2025 11:49

szkarpinski reviewed Feb 24, 2025

View reviewed changes

mzient force-pushed the C_API2_data_objects branch from 5eda037 to 18f285c Compare February 24, 2025 17:04

mzient and others added 6 commits February 24, 2025 18:04

Tensor and TensorList C API wrappers.

251d888

Signed-off-by: Michał Zientkiewicz <[email protected]>

Add functions that access the underlying native shared pointer.

df8652b

Signed-off-by: Michal Zientkiewicz <[email protected]>

Refactorig

c6fd6e9

* Add generic validation. * Move ToOptional to a new utils.h header. Signed-off-by: Michal Zientkiewicz <[email protected]>

Add shape queries.

7d0c696

Signed-off-by: Michal Zientkiewicz <[email protected]>

Tests and numerous fixes to AttachSamples.

468649f

Signed-off-by: Michal Zientkiewicz <[email protected]>

Add source info functions + setter APIs.

71d2513

Signed-off-by: Michal Zientkiewicz <[email protected]>

mzient force-pushed the C_API2_data_objects branch from 18f285c to 71d2513 Compare February 24, 2025 17:05

mzient added 2 commits February 24, 2025 18:22

Improved error message for empty batches.

972edf1

Signed-off-by: Michal Zientkiewicz <[email protected]>

Add reading instructions.

bdc1621

Signed-off-by: Michal Zientkiewicz <[email protected]>

szkarpinski approved these changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C API 2.0 Tensor and TensorList #5799

C API 2.0 Tensor and TensorList #5799

mzient commented Jan 31, 2025

szkarpinski left a comment

szkarpinski Feb 10, 2025

szkarpinski Feb 10, 2025

mzient Feb 20, 2025

banasraf Feb 16, 2025

mzient Feb 20, 2025

mzient Feb 20, 2025

banasraf Feb 24, 2025 •

edited

Loading

mzient Feb 25, 2025

szkarpinski left a comment

szkarpinski Feb 24, 2025

mzient Feb 25, 2025

szkarpinski Feb 24, 2025

mzient Feb 24, 2025 •

edited

Loading

szkarpinski Feb 24, 2025

mzient Feb 24, 2025

mzient Feb 25, 2025

mzient commented Feb 24, 2025

dali-automaton commented Feb 24, 2025

dali-automaton commented Feb 25, 2025

		@@ -0,0 +1,414 @@
		// Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		throw std::out_of_range(make_string("The sample index ", sample, " is out of range. "
		"Valid indices are [0..", shape.num_samples() - 1, "]."));

C API 2.0 Tensor and TensorList #5799

Are you sure you want to change the base?

C API 2.0 Tensor and TensorList #5799

Conversation

mzient commented Jan 31, 2025

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

szkarpinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banasraf Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szkarpinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient commented Feb 24, 2025

dali-automaton commented Feb 24, 2025

dali-automaton commented Feb 25, 2025

banasraf Feb 24, 2025 •

edited

Loading

mzient Feb 24, 2025 •

edited

Loading