Move decoders to decoders module #2725

klecki · 2021-02-25T15:49:53Z

Add alias test for image decoder - split decoder is broken.
Signed-off-by: Krzysztof Lecki klecki@nvidia.com

Why we need this PR?

Refactoring to introduce decoders module.

What happened in this PR?

What solution was applied:
Introduce alias operators for audio and image* decoders. Move the main ones to decoders module
Affected modules and functionalities:
Decoders operators
Key points relevant for the review:
Look out for typos?
Validation and testing:
Sanity alias tests
Documentation (including examples):
Yes.

JIRA TASK: [DALI-1882]

jantonguirao · 2021-03-01T09:38:21Z

dali/operators/decoder/image_decoder.cc

@@ -185,8 +185,8 @@ image format, it will decode the entire image and crop the selected ROI.

 .. note::
  ROI decoding is currently not compatible with hardware-based decoding. Using
-  :meth:`nvidia.dali.fn.image_decoder` automatically disables hardware accelerated
-  decoding. To use the hardware decoder, use the :meth:`nvidia.dali.fn.image_decoder` and
+  :meth:`nvidia.dali.fn.decoders.image` automatically disables hardware accelerated


That's a typo (the doc before). It should be image_decoder_crop/decoders.image_crop

jantonguirao · 2021-03-01T09:38:33Z

dali/operators/decoder/image_decoder.cc

-  :meth:`nvidia.dali.fn.image_decoder` automatically disables hardware accelerated
-  decoding. To use the hardware decoder, use the :meth:`nvidia.dali.fn.image_decoder` and
+  :meth:`nvidia.dali.fn.decoders.image` automatically disables hardware accelerated
+  decoding. To use the hardware decoder, use the :meth:`nvidia.dali.fn.decoders.image` and


That one is correct

jantonguirao · 2021-03-01T09:39:25Z

dali/operators/decoder/image_decoder.cc

+// Deprecated aliases
+
+DALI_SCHEMA(ImageDecoder)
+    .DocStr("Legacy alias for :meth:`decoders.image_decoder`.")


Suggested change

.DocStr("Legacy alias for :meth:`decoders.image_decoder`.")

.DocStr("Legacy alias for :meth:`decoders.image`.")

done for all

jantonguirao · 2021-03-01T09:39:35Z

dali/operators/decoder/image_decoder.cc

+// Fused
+
+DALI_SCHEMA(ImageDecoderCrop)
+    .DocStr("Legacy alias for :meth:`decoders.image_decoder_crop`.")


Suggested change

.DocStr("Legacy alias for :meth:`decoders.image_decoder_crop`.")

.DocStr("Legacy alias for :meth:`decoders.image_crop`.")

jantonguirao · 2021-03-01T09:39:48Z

dali/operators/decoder/image_decoder.cc

+functionality to allow for backward compatibility.)code");  // Deprecated in 1.0
+
+DALI_SCHEMA(ImageDecoderRandomCrop)
+    .DocStr("Legacy alias for :meth:`decoders.image_decoder_random`.")


Suggested change

.DocStr("Legacy alias for :meth:`decoders.image_decoder_random`.")

.DocStr("Legacy alias for :meth:`decoders.image_random_crop`.")

jantonguirao · 2021-03-01T09:39:59Z

dali/operators/decoder/image_decoder.cc

+
+
+DALI_SCHEMA(ImageDecoderSlice)
+    .DocStr("Legacy alias for :meth:`decoders.image_decoder_slice`.")


Suggested change

.DocStr("Legacy alias for :meth:`decoders.image_decoder_slice`.")

.DocStr("Legacy alias for :meth:`decoders.image_slice`.")

jantonguirao · 2021-03-01T09:41:47Z

dali/test/python/test_operator_decoder.py

+batch_size_alias_test=16
+
+@pipeline_def(batch_size=batch_size_alias_test, device_id=0, num_threads=4)
+def decoder_pipe(decoder_op, file_root, device, use_fast_idct, split_stages):


👍 For testing split_stages

If only it would work.

Add alias test for image decoder - split decoder is broken. TODO: test for audio decoder Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2021-03-01T13:22:51Z

!build

dali-automaton · 2021-03-01T13:26:13Z

CI MESSAGE: [2119200]: BUILD STARTED

szalpal

Some indentation issues, please fix them.

Also a comment from my side, if we deprecate some operators, shouldn't we keep them out of the table?

Or (at least) notify the user better, that he really shouldn't use them... But that's not a blocker, probably out of scope of this PR

szalpal · 2021-03-01T14:51:33Z

dali/test/python/test_operator_audio_decoder.py

+def check_audio_decoder_alias(sample_rate, downmix, quality, dtype):
+        new_pipe = decoder_pipe(fn.decoders.audio, sample_rate, downmix, quality, dtype)
+        legacy_pipe = decoder_pipe(fn.audio_decoder, sample_rate, downmix, quality, dtype)
+        compare_pipelines(new_pipe, legacy_pipe, batch_size_alias_test, 10)


Peculiar indent

szalpal · 2021-03-01T14:57:19Z

dali/test/python/test_operator_decoder.py

+                        yield check_image_decoder_alias, new_op, old_op, data_path, device, use_fast_idct, split_stages
+
+def test_image_decoder_split_alias():
+    raise SkipTest("Sanity tests for aliases of split decoder are temporarily disabled due to accuracy issues.")


Doesn't nose give an option in decorator to intentionally skip a test? For sure there's a @nottest, I don't know how about skipping with explanation...

This is the way. @nottest have totally different meaning than skipping a test.

szalpal · 2021-03-01T15:00:07Z

dali/test/python/test_operator_decoder.py

+    new_op, old_op = fn.decoders.image_slice, fn.image_decoder_slice
+    for device in ["cpu", "mixed"]:
+        for use_fast_idct in [True, False]:
+                    yield check_image_decoder_slice_alias, new_op, old_op, data_path, device, use_fast_idct


Peculiar indent

szalpal · 2021-03-01T15:00:37Z

dali/test/python/test_operator_decoder.py

+        new_pipe = decoder_slice_pipe(new_op, file_root, device, use_fast_idct)
+        legacy_pipe = decoder_slice_pipe(old_op, file_root, device, use_fast_idct)
+        compare_pipelines(new_pipe, legacy_pipe, batch_size_alias_test, 10)


Peculiar indent

szalpal · 2021-03-01T15:01:12Z

dali/test/python/test_operator_decoder.py

+        for device in ["cpu", "mixed"]:
+            for use_fast_idct in [True, False]:
+                for split_stages in [False]:
+                        yield check_image_decoder_alias, new_op, old_op, data_path, device, use_fast_idct, split_stages


Peculiar indent

szalpal · 2021-03-01T15:01:25Z

dali/test/python/test_operator_decoder.py

+        new_pipe = decoder_pipe(new_op, file_root, device, use_fast_idct, split_stages)
+        legacy_pipe = decoder_pipe(old_op, file_root, device, use_fast_idct, split_stages)
+        compare_pipelines(new_pipe, legacy_pipe, batch_size_alias_test, 10)


Peculiar indent

klecki · 2021-03-01T15:09:17Z

@szalpal
As for the table - I'd say it's fine. It says that this is legacy alias and points you to the correct operator. There is also the whole explanation by each of the operators and you get a warning with the same message.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

dali-automaton · 2021-03-01T15:17:27Z

CI MESSAGE: [2119200]: BUILD PASSED

klecki · 2021-03-01T15:18:02Z

!build

dali-automaton · 2021-03-01T15:24:15Z

CI MESSAGE: [2119446]: BUILD STARTED

dali-automaton · 2021-03-01T17:02:33Z

CI MESSAGE: [2119446]: BUILD PASSED

jantonguirao assigned jantonguirao and szalpal Feb 26, 2021

jantonguirao reviewed Mar 1, 2021

View reviewed changes

klecki added 3 commits March 1, 2021 10:58

Move decoders to decoders modul

f017350

Add alias test for image decoder - split decoder is broken. TODO: test for audio decoder Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Fixes and disabled tests

8326d7c

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Add audio decoder sanity tests

5cf0d89

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki force-pushed the decoders-rename branch from 7918d77 to 5cf0d89 Compare March 1, 2021 13:02

klecki marked this pull request as ready for review March 1, 2021 13:06

Typo

6f07864

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

This was referenced Mar 1, 2021

Adjust Python tests to decoders module #2741

Merged

Adjust notebooks to new decoder module #2743

Merged

szalpal approved these changes Mar 1, 2021

View reviewed changes

jantonguirao approved these changes Mar 1, 2021

View reviewed changes

Indenting

0132495

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

jantonguirao approved these changes Mar 1, 2021

View reviewed changes

klecki merged commit 95eb208 into NVIDIA:master Mar 1, 2021

JanuszL mentioned this pull request May 19, 2021

DALI 2021 roadmap #2978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move decoders to decoders module #2725

Move decoders to decoders module #2725

klecki commented Feb 25, 2021 •

edited

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

klecki Mar 1, 2021

jantonguirao Mar 1, 2021

klecki commented Mar 1, 2021

dali-automaton commented Mar 1, 2021

szalpal left a comment

szalpal Mar 1, 2021

klecki Mar 1, 2021

szalpal Mar 1, 2021

klecki Mar 1, 2021

szalpal Mar 1, 2021

klecki Mar 1, 2021

szalpal Mar 1, 2021

klecki Mar 1, 2021

szalpal Mar 1, 2021

klecki Mar 1, 2021

szalpal Mar 1, 2021

klecki Mar 1, 2021

klecki commented Mar 1, 2021 •

edited

dali-automaton commented Mar 1, 2021

klecki commented Mar 1, 2021

dali-automaton commented Mar 1, 2021

dali-automaton commented Mar 1, 2021

	.DocStr("Legacy alias for :meth:`decoders.image_decoder`.")
	.DocStr("Legacy alias for :meth:`decoders.image`.")



		DALI_SCHEMA(ImageDecoderSlice)
		.DocStr("Legacy alias for :meth:`decoders.image_decoder_slice`.")

Move decoders to decoders module #2725

Move decoders to decoders module #2725

Conversation

klecki commented Feb 25, 2021 • edited

Why we need this PR?

What happened in this PR?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klecki commented Mar 1, 2021

dali-automaton commented Mar 1, 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

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 commented Mar 1, 2021 • edited

dali-automaton commented Mar 1, 2021

klecki commented Mar 1, 2021

dali-automaton commented Mar 1, 2021

dali-automaton commented Mar 1, 2021

klecki commented Feb 25, 2021 •

edited

klecki commented Mar 1, 2021 •

edited