Add epoch number to SampleInfo and introduce BatchInfo #3420

stiepan · 2021-10-12T20:02:19Z

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

Add epoch number to SampleInfo class whose instances are passed to callbacks in external source in sample mode.
Add SampleInfo analog for batch mode.

Additional information

Affected modules and functionalities:

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
EXTSRC.02

JIRA TASK: N/A
DALI-2406

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2021-10-12T20:03:50Z

!build

dali-automaton · 2021-10-12T21:40:59Z

CI MESSAGE: [3173809]: BUILD STARTED

dali-automaton · 2021-10-12T23:09:00Z

CI MESSAGE: [3173809]: BUILD PASSED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient · 2021-10-13T10:33:44Z

dali/test/python/test_external_source_parallel.py

+
+
+def test_epoch_idx():
+    workers_num = 4


Suggested change

workers_num = 4

num_workers = 4

mzient · 2021-10-13T10:34:00Z

dali/test/python/test_external_source_parallel.py

+        for epoch_size in (1, 3, 7):
+            for reader_queue_depth in (1, 5):
+                sample_cb = SampleCb(batch_size, epoch_size)
+                yield _test_epoch_idx, batch_size, epoch_size, sample_cb, workers_num, prefetch_queue_depth, reader_queue_depth


Suggested change

yield _test_epoch_idx, batch_size, epoch_size, sample_cb, workers_num, prefetch_queue_depth, reader_queue_depth

yield _test_epoch_idx, batch_size, epoch_size, sample_cb, num_workers, prefetch_queue_depth, reader_queue_depth

mzient · 2021-10-13T10:34:14Z

dali/test/python/test_pool.py

@@ -110,7 +110,7 @@ def test_pool_scheduled_tasks_cleanup(start_method):
        with create_pool(callbacks, queue_depth=depth, num_workers=1, start_method=start_method) as pool:
            pids = get_pids(pool)
            pid = pids[0]
-            tasks_list = [(i, [(SampleInfo(i, 0, i),)]) for i in range(depth)]
+            tasks_list = [(i, [(SampleInfo(i, 0, i, 0),)]) for i in range(depth)]


Suggested change

tasks_list = [(i, [(SampleInfo(i, 0, i, 0),)]) for i in range(depth)]

task_list = [(i, [(SampleInfo(i, 0, i, 0),)]) for i in range(depth)]

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

jantonguirao · 2021-10-13T11:46:38Z

dali/test/python/test_external_source_parallel.py

+            for sample in batch:
+                epoch_data[np.array(sample)[0]] = True
+        assert sum(epoch_data) == epoch_size * batch_size, \
+            "Epoch number {} did not contian some samples from data set".format(epoch_idx)


Suggested change

"Epoch number {} did not contian some samples from data set".format(epoch_idx)

"Epoch number {} did not contain some samples from data set".format(epoch_idx)

klecki

Repeating callbacks and a bit similar tests, don't know if it's worth to extract if it will be only small portion of code. Otherwise mostly ok.

klecki · 2021-10-13T11:27:39Z

dali/python/nvidia/dali/external_source.py

-                self.current_iter + lead)
+                self.current_iter + lead,
+                epoch_idx)
+        elif self.batch_info:


Either I missed it, or we never set self.batch_info to False, we keep the default as None, which is Falsy, but not really a False.

Added batch_info = self._batch_info or False in external_source.py:475

klecki · 2021-10-13T11:45:53Z

dali/python/nvidia/dali/external_source.py

+        elif self.batch_info:
+            arg = nvidia.dali.types.BatchInfo(
+                self.current_iter + lead,
+                epoch_idx)


Hmm, the current_iter and current_sample are members of self and tracked by the ExternalSourceGroup, but the epoch_idx is not. I guess this is fine and better from the point of scheduling, as the Pipeline keeps the epoch number.
I think I answered my question before I even asked it.

Any thoughts on moving them out of here or moving epoch in here?

Yeah, it crossed my mind as well - in fact I started with epoch_idx in external source but thought it's something common for all sources in the pipeline. Inspecting the code I believe that we could remove current_iter from external source and use pipeline._iter instead: prefetching doesn't increase the value, it seems that group.current_iter and pipline._iter remain (thankfully) in sync. I think I'll go with removing group._current_iter in favor of pipeline._iter.

On the other hand current_sample doesn't seem to belong to the pipeline really, so we'd still have some discrepancies of what is managed where.

Okay, most importantly passing iteration number from outside makes little sense in parallel mode due to prefetching. In that case the next prefetched batch is returned so argument wouldn't really control what's returned.

klecki · 2021-10-13T12:18:18Z

dali/test/python/test_external_source_parallel.py

@@ -242,3 +242,110 @@ def test_discard():
            sample_in_epoch = 0
            iteration = 0
            pipe.reset()
+
+
+class SampleCb:


I'm seeing some repetition.

It's the same callback, but there are no common utils for those tests suits, so I didn't want to introduce another dependency just for this case. Abstracting test cases itself - even though there are some similarities - seemed counterproductive too as we would end up with util with many parameters and ifs, making it less clear if the test actually tests what it is supposed to. If you are fine with it, I would stay with this level of repetition.

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2021-10-13T15:44:12Z

!build

dali-automaton · 2021-10-13T15:46:06Z

CI MESSAGE: [3179682]: BUILD STARTED

dali-automaton · 2021-10-13T17:31:40Z

CI MESSAGE: [3179682]: BUILD FAILED

dali-automaton · 2021-10-13T17:56:51Z

CI MESSAGE: [3179682]: BUILD PASSED

* Add epoch index to SampleInfo passed to sample mode callbacks in ES * Introduce BatchInfo (a batch mode SampleInfo's counterpart) containing iteration number and epoch index. Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 3 commits October 12, 2021 21:36

Add epoch index to SampleInfo, introduce BatchInfo

84d6c1e

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Fix SampleInfo usage in tf and tests

3d72b74

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Rephrase the docs for batch_info

0411eb1

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan marked this pull request as ready for review October 13, 2021 08:22

klecki self-assigned this Oct 13, 2021

Add tf iteration comment

dd8cf00

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

jantonguirao self-assigned this Oct 13, 2021

mzient reviewed Oct 13, 2021

View reviewed changes

Remove unnecessary parentheses, review remarks on variables naming

7b3f89e

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

jantonguirao approved these changes Oct 13, 2021

View reviewed changes

klecki reviewed Oct 13, 2021

View reviewed changes

Apply review remarks

301a431

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

klecki approved these changes Oct 13, 2021

View reviewed changes

mzient approved these changes Oct 13, 2021

View reviewed changes

stiepan merged commit 1477f6d into NVIDIA:main Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add epoch number to SampleInfo and introduce BatchInfo #3420

Add epoch number to SampleInfo and introduce BatchInfo #3420

stiepan commented Oct 12, 2021 •

edited

Loading

stiepan commented Oct 12, 2021

dali-automaton commented Oct 12, 2021

dali-automaton commented Oct 12, 2021

mzient Oct 13, 2021

stiepan Oct 13, 2021

mzient Oct 13, 2021

stiepan Oct 13, 2021

mzient Oct 13, 2021

stiepan Oct 13, 2021

jantonguirao Oct 13, 2021

stiepan Oct 13, 2021

klecki left a comment

klecki Oct 13, 2021

stiepan Oct 13, 2021

klecki Oct 13, 2021

stiepan Oct 13, 2021

stiepan Oct 13, 2021

stiepan Oct 13, 2021

klecki Oct 13, 2021

stiepan Oct 13, 2021

stiepan commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

	yield _test_epoch_idx, batch_size, epoch_size, sample_cb, workers_num, prefetch_queue_depth, reader_queue_depth
	yield _test_epoch_idx, batch_size, epoch_size, sample_cb, num_workers, prefetch_queue_depth, reader_queue_depth

	tasks_list = [(i, [(SampleInfo(i, 0, i, 0),)]) for i in range(depth)]
	task_list = [(i, [(SampleInfo(i, 0, i, 0),)]) for i in range(depth)]

	"Epoch number {} did not contian some samples from data set".format(epoch_idx)
	"Epoch number {} did not contain some samples from data set".format(epoch_idx)

Add epoch number to SampleInfo and introduce BatchInfo #3420

Add epoch number to SampleInfo and introduce BatchInfo #3420

Conversation

stiepan commented Oct 12, 2021 • edited Loading

Description

Additional information

Checklist

Tests

Documentation

DALI team only

Requirements

stiepan commented Oct 12, 2021

dali-automaton commented Oct 12, 2021

dali-automaton commented Oct 12, 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

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

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

stiepan commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

dali-automaton commented Oct 13, 2021

stiepan commented Oct 12, 2021 •

edited

Loading