Ensure sample encapsulation in Tensor Vector #3701

klecki · 2022-02-23T19:15:49Z

Category: New feature, Breaking change

Description:

Add APIs matching TensorList to TensorVector:

sample pointer accessors
Set/GetMeta

Change operator[] to return [Const]SampleView.
Introduce UnsafeSetSample and UnsafeCopySample
to replace TensorVector[i].ShareData(tensor)
and TensorVector[i].Copy(tensor) - they work with current
code base, but for proper sample-based data structure
more checks should be introduced - intended for follow up.

Adjust code where necessary:

where possible use data accessors directly on the TensorVector
instead of the sample, as it should be faster than create temporary, so:
tv[i].mutable_data() -> tv.mutable_tensor(i) etc.
Using SampleViews is compatible with code that uses view<T>,
as view<T>(Tensor) is equivalent to view<T>(sample_view(Tensor))

Adjustments:

allow views to work with scalar Tensors (they treated them as empty)
introduce distinct SampleView and ConstSampleView as they need to
be returned by value and we need sensible overloads for view<>.
allow to access capacity and nbytes of individual samples,
introduce total_capacity and total_nbytes for sum of them.

Next steps written as TODO in TensorVector dosctring.

Keep the SampleView accessors temporarily with leading underscore
so they can be easily adjusted during review.

Current naming:
The sample view has temporary leading underscores in the API (_raw_data),
that would be removed before merging the PR - it's to facilitate renaming in case
it's requested and to help with verifying all calls are reworked to the
intended tv.raw_tensor(i) etc.

The Unsafe prefix in SetSample and CopySample is intended to temporary stay
there to discourage introduction of new use cases till the followup introduces
remaining checks.

Remove prefix _ from sample view naming before merge.

Additional information:

Affected modules and functionalities:

TensorVector, SampleView, SampleWorkspace-based operators, CPU operators,
C API, small bits of executor.

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

klecki · 2022-03-09T18:09:40Z

!build

dali-automaton · 2022-03-09T19:22:45Z

CI MESSAGE: [4111684]: BUILD STARTED

dali-automaton · 2022-03-09T21:35:40Z

CI MESSAGE: [4111684]: BUILD FAILED

klecki · 2022-03-10T14:32:18Z

!build

dali-automaton · 2022-03-10T14:35:50Z

CI MESSAGE: [4118107]: BUILD STARTED

dali-automaton · 2022-03-10T16:12:44Z

CI MESSAGE: [4118107]: BUILD FAILED

klecki · 2022-03-10T18:42:03Z

!build

dali-automaton · 2022-03-10T19:14:37Z

CI MESSAGE: [4119744]: BUILD STARTED

dali-automaton · 2022-03-10T20:24:28Z

CI MESSAGE: [4119744]: BUILD FAILED

klecki · 2022-03-14T18:39:42Z

!build

dali-automaton · 2022-03-14T18:45:55Z

CI MESSAGE: [4145105]: BUILD STARTED

dali-automaton · 2022-03-14T19:37:52Z

CI MESSAGE: [4145105]: BUILD FAILED

klecki · 2022-03-15T09:56:45Z

!build

dali-automaton · 2022-03-15T10:00:59Z

CI MESSAGE: [4151450]: BUILD STARTED

dali-automaton · 2022-03-15T12:00:42Z

CI MESSAGE: [4151450]: BUILD FAILED

klecki · 2022-03-16T16:22:10Z

!build

dali-automaton · 2022-03-16T16:25:44Z

CI MESSAGE: [4163058]: BUILD STARTED

dali-automaton · 2022-03-17T01:16:03Z

CI MESSAGE: [4163058]: BUILD FAILED

klecki · 2022-03-17T15:32:43Z

There is some issue with CPU-only test, I will debug it tomorrow, the rest works fine.

mzient · 2022-03-21T13:30:59Z

dali/benchmark/operator_bench.h


    if (fill_in_data) {
-      for (auto &in_ptr : *data_in) {
-        auto *ptr = in_ptr->template mutable_data<T>();
+      for (int sample_id = 0; sample_id < batch_size; sample_id++) {


Suggested change

for (int sample_id = 0; sample_id < batch_size; sample_id++) {

for (int sample_idx = 0; sample_idx < batch_size; sample_idx++) {

id suggest it's something more than an ordinal.

Signed-off-by: Krzysztof Lecki <[email protected]>

klecki · 2022-03-31T11:22:44Z

!build

dali-automaton · 2022-03-31T11:25:09Z

CI MESSAGE: [4281403]: BUILD STARTED

Signed-off-by: Krzysztof Lecki <[email protected]>

klecki · 2022-03-31T11:46:13Z

!build

dali-automaton · 2022-03-31T11:50:14Z

CI MESSAGE: [4281513]: BUILD STARTED

dali-automaton · 2022-03-31T12:52:51Z

CI MESSAGE: [4281513]: BUILD PASSED

Signed-off-by: Krzysztof Lecki <[email protected]>

mzient · 2022-03-31T13:44:03Z

dali/pipeline/data/tensor_vector.h

+   * @brief Returns the size in bytes of the underlying data chunks
+   * TODO(klecki): Temporary API to be reworked, do not use.
+   */
+  std::vector<size_t> _chunks_nbytes() const noexcept;


Suggested change

std::vector<size_t> _chunks_nbytes() const noexcept;

std::vector<size_t> _chunks_nbytes() const;

After all, a vector is dynamically allocated...

mzient · 2022-03-31T13:44:13Z

dali/pipeline/data/tensor_vector.h

+   * @brief Returns the real size of the underlying allocations
+   * TODO(klecki): Temporary API to be reworked, do not use.
+   */
+  std::vector<size_t> _chunks_capacity() const noexcept;


Signed-off-by: Krzysztof Lecki <[email protected]>

dali/pipeline/data/tensor_vector.cc

dali/pipeline/operator/builtin/external_source.h

dali/operators/reader/nemo_asr_reader_op.cc

Signed-off-by: Krzysztof Lecki <[email protected]>

klecki · 2022-03-31T16:28:05Z

!build

dali-automaton · 2022-03-31T16:30:16Z

CI MESSAGE: [4283355]: BUILD STARTED

dali-automaton · 2022-03-31T19:01:51Z

CI MESSAGE: [4283355]: BUILD PASSED

Add APIs matching TensorList to TensorVector: * sample pointer accessors * Set/GetMeta Change operator[] to return [Const]SampleView. Introduce UnsafeSetSample and UnsafeCopySample to replace TensorVector[i].ShareData(tensor) and TensorVector[i].Copy(tensor) - they work with current code base, but for proper sample-based data structure more checks should be introduced - intended for follow up. Adjust code where necessary: * where possible use data accessors directly on the TensorVector instead of the sample, as it should be faster than create temporary, so: `tv[i].mutable_data<T>()` -> `tv.mutable_tensor<T>(i)` etc. * Using SampleViews is compatible with code that uses `view<T>`, as `view<T>(Tensor)` is equivalent to `view<T>(sample_view(Tensor))` Adjustments: * allow views to work with scalar Tensors (they treated them as empty) * introduce distinct SampleView and ConstSampleView as they need to be returned by value and we need sensible overloads for `view<>`. * allow to access `capacity` and `nbytes` of individual samples, introduce _chunks_capacity and _chunks_nbytes for that. Next steps written as TODO in TensorVector dosctring. Current naming: The `Unsafe` prefix in SetSample and CopySample is intended to temporary stay there to discourage introduction of new use cases till the followup introduces remaining checks. Capacity and nbytes of individual allocations have leading underscore as the API is to be reworked and is not intended for new usages. Signed-off-by: Krzysztof Lecki <[email protected]>

klecki changed the title ~~Introduce runtime typed TensorView~~ Ensure sample encapsulation in Tensor Vector Feb 23, 2022

klecki force-pushed the encapsulate-tensor-vector-operator-subscript branch 2 times, most recently from bd0e2c5 to bc72179 Compare February 25, 2022 18:07

klecki force-pushed the encapsulate-tensor-vector-operator-subscript branch from d8d5052 to 11ca202 Compare March 9, 2022 18:05

klecki force-pushed the encapsulate-tensor-vector-operator-subscript branch 2 times, most recently from ec135cf to a296052 Compare March 16, 2022 16:12

klecki marked this pull request as ready for review March 16, 2022 16:13

klecki mentioned this pull request Mar 16, 2022

Eradicate TensorVector::operator[] #3636

Closed

23 tasks

mzient self-assigned this Mar 17, 2022

jantonguirao assigned stiepan Mar 17, 2022

mzient reviewed Mar 21, 2022

View reviewed changes

klecki added 4 commits March 31, 2022 12:33

Revert

6f5e264

Reverts

90b92a4

Review comments

74afeb5

Signed-off-by: Krzysztof Lecki <[email protected]>

Remove underscore from the names

1e98555

Signed-off-by: Krzysztof Lecki <[email protected]>

Remaining underscores

19e9b80

Signed-off-by: Krzysztof Lecki <[email protected]>

klecki added 3 commits March 31, 2022 15:29

Review adjustments

abfad38

Signed-off-by: Krzysztof Lecki <[email protected]>

Revert newline

ed6fc07

Signed-off-by: Krzysztof Lecki <[email protected]>

Move to .h

256f6b8

Signed-off-by: Krzysztof Lecki <[email protected]>

mzient reviewed Mar 31, 2022

View reviewed changes

sadadas

d264afe

Signed-off-by: Krzysztof Lecki <[email protected]>

stiepan reviewed Mar 31, 2022

View reviewed changes

dali/pipeline/data/tensor_vector.cc Show resolved Hide resolved

mzient approved these changes Mar 31, 2022

View reviewed changes

stiepan reviewed Mar 31, 2022

View reviewed changes

dali/pipeline/operator/builtin/external_source.h Show resolved Hide resolved

stiepan reviewed Mar 31, 2022

View reviewed changes

dali/operators/reader/nemo_asr_reader_op.cc Outdated Show resolved Hide resolved

stiepan approved these changes Mar 31, 2022

View reviewed changes

Fixup

735caf1

Signed-off-by: Krzysztof Lecki <[email protected]>

klecki merged commit 0cd1cf1 into NVIDIA:main Mar 31, 2022

klecki deleted the encapsulate-tensor-vector-operator-subscript branch March 31, 2022 19:24

klecki restored the encapsulate-tensor-vector-operator-subscript branch April 1, 2022 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure sample encapsulation in Tensor Vector #3701

Ensure sample encapsulation in Tensor Vector #3701

klecki commented Feb 23, 2022 •

edited

Loading

klecki commented Mar 9, 2022

dali-automaton commented Mar 9, 2022

dali-automaton commented Mar 9, 2022

klecki commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

klecki commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

klecki commented Mar 14, 2022

dali-automaton commented Mar 14, 2022

dali-automaton commented Mar 14, 2022

klecki commented Mar 15, 2022

dali-automaton commented Mar 15, 2022

dali-automaton commented Mar 15, 2022

klecki commented Mar 16, 2022

dali-automaton commented Mar 16, 2022

dali-automaton commented Mar 17, 2022

klecki commented Mar 17, 2022

mzient Mar 21, 2022 •

edited

Loading

klecki Mar 22, 2022

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

mzient Mar 31, 2022

klecki Mar 31, 2022

mzient Mar 31, 2022

klecki Mar 31, 2022

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

	for (int sample_id = 0; sample_id < batch_size; sample_id++) {
	for (int sample_idx = 0; sample_idx < batch_size; sample_idx++) {

	std::vector<size_t> _chunks_nbytes() const noexcept;
	std::vector<size_t> _chunks_nbytes() const;

Ensure sample encapsulation in Tensor Vector #3701

Ensure sample encapsulation in Tensor Vector #3701

Conversation

klecki commented Feb 23, 2022 • edited Loading

Category: New feature, Breaking change

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

klecki commented Mar 9, 2022

dali-automaton commented Mar 9, 2022

dali-automaton commented Mar 9, 2022

klecki commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

klecki commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

dali-automaton commented Mar 10, 2022

klecki commented Mar 14, 2022

dali-automaton commented Mar 14, 2022

dali-automaton commented Mar 14, 2022

klecki commented Mar 15, 2022

dali-automaton commented Mar 15, 2022

dali-automaton commented Mar 15, 2022

klecki commented Mar 16, 2022

dali-automaton commented Mar 16, 2022

dali-automaton commented Mar 17, 2022

klecki commented Mar 17, 2022

mzient Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

klecki Mar 22, 2022

Choose a reason for hiding this comment

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

mzient Mar 31, 2022

Choose a reason for hiding this comment

klecki Mar 31, 2022

Choose a reason for hiding this comment

mzient Mar 31, 2022

Choose a reason for hiding this comment

klecki Mar 31, 2022

Choose a reason for hiding this comment

klecki commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

dali-automaton commented Mar 31, 2022

klecki commented Feb 23, 2022 •

edited

Loading

mzient Mar 21, 2022 •

edited

Loading