Add FW iterators handling of variable batch size and improve ES examples #2641

JanuszL · 2021-01-27T22:59:26Z

makes FW iterator ExternalSource examples handle properly StopIteration
adds a basic check if FW iterators handles properly variable batch size only when there is not externally provided the size of the iterator. Otherwise, it should assert

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Why we need this PR?

Pick one, remove the rest

It adds FW iterators handling of variable batch size and improve ES examples

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
makes FW iterator ExternalSource examples handle properly StopIteration
adds a basic check if FW iterators handles properly variable batch size only when there is not externally provided the size of the iterator. Otherwise, it should assert
Affected modules and functionalities:
FW data loading examples
FW iterators
Key points relevant for the review:
NA
Validation and testing:
tests are added
Documentation (including examples):
docs and examples have been updated

Relates to #2639
JIRA TASK: [NA]

review-notebook-app · 2021-01-27T22:59:30Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

JanuszL · 2021-01-27T23:03:38Z

!build

dali-automaton · 2021-01-28T00:39:35Z

CI MESSAGE: [2021336]: BUILD STARTED

dali-automaton · 2021-01-28T01:57:44Z

CI MESSAGE: [2021336]: BUILD FAILED

JanuszL · 2021-01-28T09:28:49Z

!build

dali-automaton · 2021-01-28T09:30:54Z

CI MESSAGE: [2022819]: BUILD STARTED

dali-automaton · 2021-01-28T11:09:02Z

CI MESSAGE: [2022819]: BUILD FAILED

…ples - makes FW iterator ExternalSource examples handle properly StopIteration - adds a basic check if FW iterators handles properly variable batch size only when there is not externally provided the size of the iterator. Otherwise, it should assert Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2021-01-28T13:29:05Z

!build

dali-automaton · 2021-01-28T13:30:46Z

CI MESSAGE: [2023433]: BUILD STARTED

mzient · 2021-01-28T14:56:59Z

dali/python/nvidia/dali/external_source.py

@@ -234,7 +234,7 @@ class ExternalSource():

    If set to False, StopIteration is raised when the end of data is reached.
    This flag requires that the ``source`` is a collection, for example, an iterable object where
-    ``iter(source)`` returns a fresh iterator on each call or a gensource erator function.
+    ``iter(source)`` returns a fresh iterator on each call or a gensource iterator function.


Suggested change

``iter(source)`` returns a fresh iterator on each call or a gensource iterator function.

``iter(source)`` returns a fresh iterator on each call, or a generator function.

mzient · 2021-01-28T14:58:55Z

dali/python/nvidia/dali/plugin/base_iterator.py

+                  "whenever possible. This may lead, in some situations, to miss some " +
+                  "samples or return duplicated ones. Check the Sharding section of the "


lead to something

Suggested change

"whenever possible. This may lead, in some situations, to miss some " +

"samples or return duplicated ones. Check the Sharding section of the "

"whenever possible. This may lead, in some situations, to missing some " +

"samples or returning duplicated ones. Check the Sharding section of the "

dali-automaton · 2021-01-28T15:11:06Z

CI MESSAGE: [2023433]: BUILD PASSED

dali/python/nvidia/dali/plugin/base_iterator.py

mzient · 2021-01-28T17:28:59Z

dali/test/python/test_fw_iterators.py

@@ -1051,6 +1052,33 @@ def get_data():
            counter += 1
    assert counter == iter_limit * runs

+def check_external_source_variable_size(Iterator, *args, iter_size=-1, **kwargs):
+    batch_size = 4


Nitpick:

Suggested change

batch_size = 4

max_batch_size = 4

mzient · 2021-01-28T17:32:17Z

dali/test/python/test_fw_iterators.py

+    def get_data():
+        nonlocal i
+        if i == iter_limit:
+            i = 0
+            raise StopIteration
+        out = [[np.random.randint(0, 255, size = test_data_shape, dtype = np.uint8) for _ in range(random.randint(1, batch_size))]]
+        i += 1
+        return out


Suggested change

def get_data():

nonlocal i

if i == iter_limit:

i = 0

raise StopIteration

out = [[np.random.randint(0, 255, size = test_data_shape, dtype = np.uint8) for _ in range(random.randint(1, batch_size))]]

i += 1

return out

dataset = [[[np.random.randint(0, 255, size = test_data_shape, dtype = np.uint8) for _ in range(random.randint(1, batch_size))]] for _ in range(iter_limit)]

This will allow you to actually compare outputs against this pre-generated data. Aslo, passing dataset as the source will raise StopIteration automatically.

Not possible with the current ES behavior.

mzient · 2021-01-28T17:35:20Z

dali/test/python/test_fw_iterators.py

+        for __ in enumerate(it):
+            counter += 1


You can get the data here and compare with dataset, so you'll know for sure that data is intact.

See the comment above.

mzient · 2021-01-28T17:38:16Z

docs/examples/frameworks/pytorch/pytorch-external_input.ipynb

@@ -163,7 +164,7 @@
    "eii = ExternalInputIterator(batch_size, 0, 1)\n",
    "pipe = ExternalSourcePipeline(batch_size=batch_size, num_threads=2, device_id = 0,\n",
    "                              external_data = eii)\n",
-    "pii = PyTorchIterator(pipe, size=len(eii), last_batch_padded=True, last_batch_policy=LastBatchPolicy.PARTIAL)\n",
+    "pii = PyTorchIterator(pipe, size=-1, last_batch_padded=True, last_batch_policy=LastBatchPolicy.PARTIAL)\n",


Is it mandatory?

JanuszL · 2021-01-28T18:07:07Z

!build

dali-automaton · 2021-01-28T18:10:49Z

CI MESSAGE: [2024247]: BUILD STARTED

lgtm-com · 2021-01-28T18:17:59Z

This pull request introduces 1 alert when merging 402f972d278b6417557406ae5572207f34761218 into 0af127a - view on LGTM.com

new alerts:

1 for Syntax error

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2021-01-28T23:38:04Z

!build

dali-automaton · 2021-01-28T23:42:10Z

CI MESSAGE: [2025312]: BUILD STARTED

dali-automaton · 2021-01-29T01:18:08Z

CI MESSAGE: [2025312]: BUILD PASSED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2021-01-29T08:37:36Z

!build

dali-automaton · 2021-01-29T08:41:50Z

CI MESSAGE: [2026640]: BUILD STARTED

dali-automaton · 2021-01-29T10:27:17Z

CI MESSAGE: [2026640]: BUILD PASSED

szalpal

The two nitpicks also apply to remaining changed functions.

Please double check the set_outputs scope, I don't know how it should look like.

Otherwise LGTM

szalpal · 2021-01-29T15:45:21Z

dali/test/python/test_fw_iterators.py

+def CreateCOCOReaderPipeline(data_paths, batch_size, num_threads, shard_id, num_gpus, random_shuffle, stick_to_shard, shuffle_after_epoch, pad_last_batch, initial_fill=1024, return_labels=False):
+        pipe = Pipeline(batch_size = batch_size, num_threads = num_threads, device_id = 0, prefetch_queue_depth=1)
+        with pipe:
+            _, _, labels, ids = fn.coco_reader(file_root = data_paths[0], annotations_file=data_paths[1],
+                                                shard_id = shard_id, num_shards = num_gpus, random_shuffle=random_shuffle,
+                                                image_ids=True, stick_to_shard=stick_to_shard,shuffle_after_epoch=shuffle_after_epoch,
+                                                pad_last_batch=pad_last_batch, initial_fill=initial_fill, name="Reader")
+        if return_labels:
+            pipe.set_outputs(labels, ids)
+        else:
+            pipe.set_outputs(ids)
+        return pipe


Two nitpicks:
One is that formatting around = is inconsistent in this function, maybe it would be good idea to make it consistent. The second is that maybe it would be better to use snake_case for the name, since it's a function

szalpal · 2021-01-29T15:46:31Z

dali/test/python/test_fw_iterators.py

+        if return_labels:
+            pipe.set_outputs(labels, ids)
+        else:
+            pipe.set_outputs(ids)
+        return pipe


Shouldn't pipe.put_outputs be also in the scope of with?

szalpal · 2021-01-29T16:19:27Z

dali/test/python/test_fw_iterators.py

+    pipe = Pipeline(batch_size = max_batch_size, num_threads = 1, device_id = 0)
+    with pipe:
+        outs = fn.external_source(source = get_data, num_outputs=1)
+    pipe.set_outputs(*outs)


Again, I don't really now precisely how this should work, if the set_outputs should be in the scope of with or not... Just putting attention to that

Scope matters for the operators with side effects. I don't think set_outputs has any.

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2021-01-29T16:49:20Z

!build

dali-automaton · 2021-01-29T16:56:02Z

CI MESSAGE: [2027748]: BUILD STARTED

dali-automaton · 2021-01-29T18:53:17Z

CI MESSAGE: [2027748]: BUILD PASSED

JanuszL force-pushed the fix_external_examples branch from 28ec08f to 474cde0 Compare January 27, 2021 22:59

JanuszL mentioned this pull request Jan 27, 2021

The pytorch-external example seems to be wrong #2639

Closed

JanuszL force-pushed the fix_external_examples branch from 474cde0 to 77909dc Compare January 28, 2021 09:28

JanuszL linked an issue Jan 28, 2021 that may be closed by this pull request

The pytorch-external example seems to be wrong #2639

Closed

awolant assigned szalpal and mzient Jan 28, 2021

JanuszL force-pushed the fix_external_examples branch from 77909dc to 19ca115 Compare January 28, 2021 11:41

JanuszL force-pushed the fix_external_examples branch from 19ca115 to caca1ae Compare January 28, 2021 11:51

mzient reviewed Jan 28, 2021

View reviewed changes

dali/python/nvidia/dali/plugin/base_iterator.py Show resolved Hide resolved

mzient reviewed Jan 28, 2021

View reviewed changes

Review fixes

c3b0c5a

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL force-pushed the fix_external_examples branch from 402f972 to c3b0c5a Compare January 28, 2021 23:33

mzient approved these changes Jan 29, 2021

View reviewed changes

More review fixes

aab5f4a

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

mzient approved these changes Jan 29, 2021

View reviewed changes

szalpal approved these changes Jan 29, 2021

View reviewed changes

More review fixes

ada4cee

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL force-pushed the fix_external_examples branch from 26da087 to ada4cee Compare January 29, 2021 16:40

JanuszL merged commit 3a4fc33 into NVIDIA:master Jan 29, 2021

JanuszL deleted the fix_external_examples branch January 29, 2021 23:18

JanuszL mentioned this pull request May 19, 2021

DALI 2021 roadmap #2978

Closed

	``iter(source)`` returns a fresh iterator on each call or a gensource iterator function.
	``iter(source)`` returns a fresh iterator on each call, or a generator function.

		"whenever possible. This may lead, in some situations, to miss some " +
		"samples or return duplicated ones. Check the Sharding section of the "

Add FW iterators handling of variable batch size and improve ES examples #2641

Add FW iterators handling of variable batch size and improve ES examples #2641

Conversation

JanuszL commented Jan 27, 2021 • edited Loading

Why we need this PR?

What happened in this PR?

review-notebook-app bot commented Jan 27, 2021

JanuszL commented Jan 27, 2021

dali-automaton commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

JanuszL commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

JanuszL commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

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

dali-automaton commented Jan 28, 2021

mzient Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

JanuszL Jan 28, 2021 • 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

JanuszL commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

lgtm-com bot commented Jan 28, 2021

JanuszL commented Jan 28, 2021

dali-automaton commented Jan 28, 2021

dali-automaton commented Jan 29, 2021

JanuszL commented Jan 29, 2021

dali-automaton commented Jan 29, 2021

dali-automaton commented Jan 29, 2021

szalpal 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

JanuszL commented Jan 29, 2021

dali-automaton commented Jan 29, 2021

dali-automaton commented Jan 29, 2021

JanuszL commented Jan 27, 2021 •

edited

Loading

mzient Jan 28, 2021 •

edited

Loading

mzient Jan 28, 2021 •

edited

Loading

JanuszL Jan 28, 2021 •

edited

Loading