Add C API header and C language build test. #5796

mzient · 2025-01-31T16:46:30Z

Category:

New feature (non-breaking change which adds functionality)

Description:

This PR adds C API header file.

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

klecki

Some nitpicks + the gaps for the pipeline constructor parameters - are those passed only via the serialized pipeline?

Also, I suspect that we are returning pointers to internal Tensor/TensorList data directly (the layout and the shape). Any safety considerations - we break the encapsulation this way, and the user may overwrite them, if we just hand out the internal pointers.

include/dali/dali.h

klecki · 2025-02-10T15:24:00Z

include/dali/dali.h

+  daliExecType_t exec_type;
+  daliBool enable_checkpointing;
+  daliBool enable_memory_stats;
+} daliPipelineParams_t;


What were the criteria to get in this struct.
We also have set_affinity, control over queue depth and in python we can pass the information about expected output types and dimensionality. Do we consider any of those to be parts of the C API as well?
(Yes, there is stuff like callback pickler in Python which for sure doesn't reach to here).

I strongly believe that output dtype and ndim should be taken from the serialized pipeline. If they're not - then we'd better add them there instead of breaking the pipeline construction interface again.
As for set_affinity - I guess that's a simple omission and we can/should add it here.

Added daliExecFlags_t.

klecki · 2025-02-10T15:41:57Z

include/dali/dali.h

+  daliTensorList_h input_data,
+  const char *data_id,
+  daliFeedInputFlags_t options,
+  const cudaStream_t *stream);


What kind of stream is that?

You mean, missing documentation? Will fix.

klecki · 2025-02-10T15:52:36Z

include/dali/dali.h

+/** Gets a descriptor of the specified pipeline output.
+ *
+ * @param pipeline  [in]  The pipeline
+ * @param out_desc  [out] A pointer to the returned descriptor.


Maybe it's me, but it reads as if we return a pointer to a descriptor.
Dunno if it is much better:

Suggested change

* @param out_desc [out] A pointer to the returned descriptor.

* @param out_desc [out] A pointer to where the output descriptor is written to/stored.

Used slightly different wording.

klecki · 2025-02-10T16:01:43Z

include/dali/dali.h

+ *                      must contain num_samples*ndim extents
+ * @param dtype         the element type
+ * @param layout        a layout string describing the order of axes in each sample (e.g. HWC),
+ *                      if NULL, and the TensorList's number of dimensions is equal to `ndim,


Suggested change

* if NULL, and the TensorList's number of dimensions is equal to `ndim,

* if NULL, and the TensorList's number of dimensions is equal to `ndim`,

klecki · 2025-02-10T16:23:07Z

include/dali/dali.h

+   * The shape starts with the "slowest" dimension - a row-major 640x480 interleaved RGB image
+   * would have the shape [480, 640, 3].
+   *
+   * The shape can be NULL if ndim == 0


Can or is? Should we limit ambiguity?

Definitely "can". Overvalidation is evil. If you have std::vector<int> with a nonzero capacity but zero size, then .data() will return a non-null pointer. We don't want to force the user to "fix" std::vector - that'd be daft.

mzient · 2025-02-11T11:29:38Z

Some nitpicks + the gaps for the pipeline constructor parameters - are those passed only via the serialized pipeline?

Also, I suspect that we are returning pointers to internal Tensor/TensorList data directly (the layout and the shape). Any safety considerations - we break the encapsulation this way, and the user may overwrite them, if we just hand out the internal pointers.

We are returning const pointers, so it should be impossible to overwrite those by accident.

include/dali/dali.h

klecki · 2025-02-11T21:07:56Z

include/dali/dali.h

+
+  /* TODO(michalz): Make stream policy configurable in the pipeline */
+  /** Masks the part of the flags that represent the stream policy. */
+  DALI_EXEC_FLAGS_STREAM_POLICY_MASK   = 0x00000070,


Do you think we should have some blocks reserved for that? Like every byte?

A byte would be too much. I currently reserved 3 bits and now we have 3 nonzero values, so we can squeeze 4 more. I can hardly imagine we'd have that many stream policies or concurrency policies.

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

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

rostan-t · 2025-02-12T15:06:22Z

include/dali/dali.h

+
+/** Gets the readiness event associated with the TensorList or creates a new one.
+ *
+ * @param tensor_list   [in]  the tensor list to associate an even twith


Suggested change

* @param tensor_list [in] the tensor list to associate an even twith

* @param tensor_list [in] the tensor list to associate an event with

rostan-t · 2025-02-12T15:06:43Z

include/dali/dali.h

+
+/** Gets the readiness event associated with the Tensor or creates a new one.
+ *
+ * @param tensor        [in]  the tensor to associate an even twith


Suggested change

* @param tensor [in] the tensor to associate an even twith

* @param tensor [in] the tensor to associate an event with

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

dali-automaton · 2025-02-12T15:32:25Z

CI MESSAGE: [23844740]: BUILD STARTED

dali-automaton assigned mdabek-nvidia, rostan-t and klecki Feb 3, 2025

mzient force-pushed the C_API2_header branch from be30385 to 69f0aac Compare February 10, 2025 14:59

klecki reviewed Feb 10, 2025

View reviewed changes

mdabek-nvidia reviewed Feb 11, 2025

View reviewed changes

include/dali/dali.h Outdated Show resolved Hide resolved

include/dali/dali.h Outdated Show resolved Hide resolved

mzient force-pushed the C_API2_header branch from 21a28a7 to 43e1481 Compare February 11, 2025 14:21

rostan-t reviewed Feb 11, 2025

View reviewed changes

klecki approved these changes Feb 11, 2025

View reviewed changes

mzient and others added 4 commits February 12, 2025 15:30

Add C API header and C language build test.

7341e27

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

Fix documentation for XxxRefCount.

8036c73

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

Use internal linkage for inline versioning trampolines.

d57444d

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

Add executor flags. Extend and fix documentation.

3da3a7e

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

mzient force-pushed the C_API2_header branch 4 times, most recently from fc4cbfe to 2a986e1 Compare February 12, 2025 15:03

rostan-t approved these changes Feb 12, 2025

View reviewed changes

Numerous fixes to the documentation.

ebb140a

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

mzient force-pushed the C_API2_header branch from 2a986e1 to ebb140a Compare February 12, 2025 15:31

mdabek-nvidia approved these changes Feb 12, 2025

View reviewed changes

mzient merged commit d36e0ba into NVIDIA:main Feb 13, 2025
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C API header and C language build test. #5796

Add C API header and C language build test. #5796

mzient commented Jan 31, 2025

klecki left a comment

klecki Feb 10, 2025

mzient Feb 11, 2025

mzient Feb 11, 2025

klecki Feb 10, 2025

mzient Feb 11, 2025

mzient Feb 11, 2025

klecki Feb 10, 2025

mzient Feb 11, 2025

klecki Feb 10, 2025

mzient Feb 11, 2025

klecki Feb 10, 2025

mzient Feb 11, 2025 •

edited

Loading

mzient commented Feb 11, 2025

klecki Feb 11, 2025

mzient Feb 12, 2025 •

edited

Loading

rostan-t Feb 12, 2025

rostan-t Feb 12, 2025

dali-automaton commented Feb 12, 2025

	* @param out_desc [out] A pointer to the returned descriptor.
	* @param out_desc [out] A pointer to where the output descriptor is written to/stored.

	* if NULL, and the TensorList's number of dimensions is equal to `ndim,
	* if NULL, and the TensorList's number of dimensions is equal to `ndim`,

	* @param tensor_list [in] the tensor list to associate an even twith
	* @param tensor_list [in] the tensor list to associate an event with

	* @param tensor [in] the tensor to associate an even twith
	* @param tensor [in] the tensor to associate an event with

Add C API header and C language build test. #5796

Add C API header and C language build test. #5796

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

klecki 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

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

mzient Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

mzient commented Feb 11, 2025

Choose a reason for hiding this comment

mzient Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Feb 12, 2025

mzient Feb 11, 2025 •

edited

Loading

mzient Feb 12, 2025 •

edited

Loading