Functional API + improved ExternalSource + improved Pipeline #1598

mzient · 2019-12-20T13:18:45Z

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Why we need this PR?

simplified usage of operators - avoid double call; use operators directly, without creating "instances" in constructor
greatly simplified ExternalSource usage - pass data generating callbacks directly to ExternalSource
pipeline outputs can be set with set_outputs
added context manager (with block) to manage current pipeline

What happened in this PR?

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

What solution was applied:
- Wrapper functions and type-dependent argument grouping
Affected modules and functionalities:
- Python front-end
Key points relevant for the review:
- ops.py and the example notebook
Validation and testing:
- Regression: existing tests
- New API: TODO
Documentation (including examples):
- TODO

JIRA TASK: DALI-1289

mzient · 2019-12-20T13:18:56Z

!build

dali-automaton · 2019-12-20T13:20:14Z

CI MESSAGE: [1042036]: BUILD STARTED

JanuszL · 2019-12-20T13:41:43Z

dali/python/nvidia/dali/ops.py

+
+  if nupper > 0:
+    out += camel[start:i].lower()
+  out = out.replace("b_box", "bbox").replace("mx_net", "mxnet")


I think we should have a dict of special cases in one place. Here is easy to miss and extend.

Yes and no. BBox is not a special case - rather a pattern. With MXNet - currently we have just one operator, but should we have more, it's already handled.

dali/python/nvidia/dali/ops.py

dali-automaton · 2019-12-20T14:06:41Z

CI MESSAGE: [1042036]: BUILD PASSED

klecki

As an experimental API, if we even merge this I would first consider putting the whole thing into another module like:
nvidia.dali.experimental.ops or nvidia.dali.experimental.eager.
Ideally in a way, that would later allow removing the .experimental. from the name (and maybe leaving the fallback for a while).
Still, I think it would be better to have them in module other than ops.

If someone want's to use it, he can always alias the imported functionality.

Some documentation or at least PR description could say, that it reexports the Operators as snake_cased functions.

We may need to introduce the EdgeReference as some first class citizen, so we can describe what it represents and how to use it in the docs. It would be also helpful for the error messages.

I would also be interested in how you plan to show those functions in docs? It would not be ideal to duplicate everything. The tensor argument inputs/python values/python lists of values would be especially tricky, as now they can be mixed in the same call.

There was also different idea to abuse the python even more and:

pretend that we have the named inputs that can work as keyword arguments - we can do this by counting the positional inputs and checking if someone passed input as keyword.
actually create the proper signature. This can be also useful here, we can use python-forge (the even have Dali picture on the project page :P)
https://pypi.org/project/python-forge/

JanuszL · 2019-12-20T15:41:21Z

We may need to introduce the EdgeReference as some first class citizen, so we can describe what it represents and how to use it in the docs. It would be also helpful for the error messages.

Definitely it is not an eager mode.

klecki · 2019-12-20T15:42:30Z

Definitely it is not an eager mode.

Yeah, just a name.

klecki · 2019-12-20T15:47:04Z

Actually it is kinda eager in the regard how you create and parametrize the ops.
My other worry is that having device="gpu" everywhere in the define_graph really clutters the concise graph definition.

mzient · 2019-12-20T19:38:34Z

@klecki It's not "just a name". It's not eager mode nor is it intended to be.
As for the "creation and parametrization" - it's no more eager than the old design.
As for device="something" - I have two suggestions:
Aliases:

resize
resize_cpu
resize_gpu

Submodules/namespaces/whatever:

ops.resize
ops.cpu.resize
ops.gpu.resize

I would definitely like to keep the parametric version so the user can still provide the device as an argument - though the second solution provides a workaround:

device = ops.cpu
device.resize(...)

klecki · 2019-12-20T23:44:15Z

@klecki It's not "just a name". It's not eager mode nor is it intended to be.
As for the "creation and parametrization" - it's no more eager than the old design.

You all misinterpret my intention and focus on details.
I say that it would be better to put the functions in a separate dedicated module,
like dali.ops.fn, dali.ops.functions, dali.shortcut, dali.functions, dali.whatever.we.name.them.just.make.them.distinct.and.not.pay.attention.to.a.misnamed.example.

As for the device, I think pytorch had some experience and recent changes in that regard, we should look for hints in regard for the usability of the solution.

The device argument is not the only one that will clutter the visuals of defining graph.

Maybe a good thing to do would be to take some of the example pipelines that we have (especially some of the more complex/configurable ones) and to rewrite/mock them with this approach to identify what becomes harder and what easier, what are the weak points that a change in API could fix.

mzient · 2020-01-07T14:40:37Z

!build

dali-automaton · 2020-01-07T14:45:10Z

CI MESSAGE: [1060592]: BUILD STARTED

dali-automaton · 2020-01-07T15:33:58Z

CI MESSAGE: [1060592]: BUILD FAILED

mzient · 2020-01-07T16:27:50Z

!build

dali-automaton · 2020-01-07T16:30:27Z

CI MESSAGE: [1060685]: BUILD STARTED

mzient · 2020-01-07T16:36:27Z

!build

dali-automaton · 2020-01-07T16:40:12Z

CI MESSAGE: [1060699]: BUILD STARTED

dali-automaton · 2020-01-07T18:00:22Z

CI MESSAGE: [1060699]: BUILD FAILED

JanuszL · 2020-01-10T15:02:42Z

docs/examples/index.rst

@@ -22,3 +22,4 @@ Tutorials
   audio_decoder.ipynb
   hsv_example.ipynb
   brightness_contrast_example.ipynb
+   experimental_new_api.ipynb


I don't know if we should call it experimental?

JanuszL · 2020-01-10T15:05:18Z

docs/examples/pytorch/pytorch-various-readers.ipynb

+    "\n",
+    "        crop_pos = ops.Uniform(range = (0.0, 1.0))\n",
+    "        output = ops.crop_mirror_normalize(\n",
+    "            images, crop_pos_x = crop_pos(),\n",


Suggested change

" images, crop_pos_x = crop_pos(),\n",

" images,\n"

" crop_pos_x = crop_pos(),\n",

JanuszL · 2020-01-10T15:08:05Z

I say that it would be better to put the functions in a separate dedicated module,
like dali.ops.fn, dali.ops.functions, dali.shortcut, dali.functions, dali.whatever.we.name.them.just.make.them.distinct.and.not.pay.attention.to.a.misnamed.example.

I'm very much agree to use a separate module/namespace for that.

mzient · 2020-02-24T17:00:43Z

!build

dali-automaton · 2020-02-24T17:05:35Z

CI MESSAGE: [1147244]: BUILD STARTED

mzient · 2020-03-06T13:40:39Z

!build

dali-automaton · 2020-03-06T13:45:45Z

CI MESSAGE: [1173371]: BUILD STARTED

Added functional API documentation, marked as experimental. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2020-03-06T14:36:02Z

CI MESSAGE: [1173447]: BUILD STARTED

dali-automaton · 2020-03-06T20:48:15Z

CI MESSAGE: [1173447]: BUILD PASSED

mzient · 2020-03-10T09:46:11Z

!build

dali-automaton · 2020-03-10T09:50:24Z

CI MESSAGE: [1180016]: BUILD STARTED

…edAPI_PoC

dali-automaton · 2020-03-10T10:39:38Z

CI MESSAGE: [1180145]: BUILD STARTED

dali-automaton · 2020-03-10T12:48:36Z

CI MESSAGE: [1180145]: BUILD PASSED

klecki · 2020-03-11T10:25:34Z

dali/python/nvidia/dali/ops.py

-            t = _EdgeReference(t_name, output_device, self)
+            t_name = self._name
+            if num_output > 1:
+                t_name += "[{}]".format(i)


Possible output name collision if the _name is duplicated - either requires more checking or additional mangling.

klecki

The docstrings for build() and set_outputs() are not rendered properly.

klecki · 2020-03-11T11:47:09Z

dali/python/nvidia/dali/pipeline.py

+                groups.add(group)
+        self._input_callbacks = list(groups)
+
+    def build(self, define_graph = None):


This adds yet another way to build a pipeline and at some point we will start causing confusion without clear doc how to do something.

Especially as this argument will now appear in docs, but just not documented.

klecki · 2020-03-11T11:47:48Z

dali/python/nvidia/dali/pipeline.py

+        """Set the outputs of the pipeline.
+
+        Use of this function is an alternative to overriding `define_graph` in a derived class.
+
+        Args
+        ----
+        `*output_data_nodes` : unpacked list of :class:`DataNode` objects
+        The outputs of the pipeline
+        """


The effective docstring is all over the place in sphinx.

klecki · 2020-03-11T12:01:39Z

docs/api.rst

-.. autoclass:: nvidia.dali.backend.TensorGPU
-   :members:
-   :undoc-members:
+`Functioal API <functional_api.rst>`_ (Experimental!) section describes a psuedo-imperative API


Suggested change

`Functioal API <functional_api.rst>`_ (Experimental!) section describes a psuedo-imperative API

`Functional API <functional_api.rst>`_ (Experimental!) section describes a psuedo-imperative API

klecki · 2020-03-11T12:03:00Z

docs/api.rst


-Tensor
------
+`Pipeline <pipeline.rst>`_ section describes the :class:`Pipeline` object - the central and most


Suggested change

`Pipeline <pipeline.rst>`_ section describes the :class:`Pipeline` object - the central and most

`Pipeline <pipeline.rst>`_ section describes the :class:`~nvidia.dali.pipelinePipeline` class- the central and most

Adding the tilde is supposed to render it as only Pipeline.

lgtm-com · 2020-03-11T12:38:23Z

This pull request introduces 1 alert when merging f0b1c7d167e298affbdab07836666d45e8643dc6 into 107f603 - view on LGTM.com

new alerts:

1 for Unused import

mzient · 2020-03-11T12:45:49Z

!build

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

dali-automaton · 2020-03-11T12:50:35Z

CI MESSAGE: [1182951]: BUILD STARTED

dali-automaton · 2020-03-11T13:46:11Z

CI MESSAGE: [1182951]: BUILD PASSED

mzient requested a review from a team December 20, 2019 13:18

JanuszL reviewed Dec 20, 2019

View reviewed changes

dali/python/nvidia/dali/ops.py Outdated Show resolved Hide resolved

klecki previously requested changes Dec 20, 2019

View reviewed changes

mzient force-pushed the SimplifiedAPI_PoC branch from 373ab3d to 5c23243 Compare January 7, 2020 14:37

mzient force-pushed the SimplifiedAPI_PoC branch from 5c23243 to ff07c34 Compare January 7, 2020 16:27

mzient force-pushed the SimplifiedAPI_PoC branch from ff07c34 to 789194d Compare January 7, 2020 16:35

JanuszL reviewed Jan 10, 2020

View reviewed changes

mzient mentioned this pull request Feb 11, 2020

Add 'Compose()' style interface for DALI IO and transforms #1731

Closed

mzient force-pushed the SimplifiedAPI_PoC branch from 789194d to 2e14dc7 Compare February 24, 2020 17:00

JanuszL mentioned this pull request Mar 6, 2020

why ops.ImageDecoder use "cpu" mode error ? #1795

Closed

mzient force-pushed the SimplifiedAPI_PoC branch from 55332b5 to 29fd4c1 Compare March 6, 2020 13:39

mzient force-pushed the SimplifiedAPI_PoC branch from 29fd4c1 to a826318 Compare March 6, 2020 14:07

Split Python API documentation into multiple files.

e25ddcd

Added functional API documentation, marked as experimental. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the SimplifiedAPI_PoC branch from a826318 to e25ddcd Compare March 6, 2020 14:33

mzient force-pushed the SimplifiedAPI_PoC branch from 7082edc to 1945b46 Compare March 9, 2020 10:42

Merge commit 'e25ddcd70d12a7b57af5550fbffcf6e444f2499c' into Simplifi…

34428a9

…edAPI_PoC

mzient force-pushed the SimplifiedAPI_PoC branch from e6447e8 to 34428a9 Compare March 10, 2020 10:33

klecki reviewed Mar 11, 2020

View reviewed changes

Fix review issues.

687f080

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the SimplifiedAPI_PoC branch from f0b1c7d to 687f080 Compare March 11, 2020 12:46

klecki approved these changes Mar 11, 2020

View reviewed changes

mzient changed the title ~~Functional API + improved ExternalSource~~ Functional API + improved ExternalSource + improved Pipeline Mar 11, 2020

mzient merged commit c984e34 into NVIDIA:master Mar 11, 2020

mzient deleted the SimplifiedAPI_PoC branch March 27, 2020 13:40

	" images, crop_pos_x = crop_pos(),\n",
	" images,\n"
	" crop_pos_x = crop_pos(),\n",

	`Functioal API <functional_api.rst>`_ (Experimental!) section describes a psuedo-imperative API
	`Functional API <functional_api.rst>`_ (Experimental!) section describes a psuedo-imperative API

	`Pipeline <pipeline.rst>`_ section describes the :class:`Pipeline` object - the central and most
	`Pipeline <pipeline.rst>`_ section describes the :class:`~nvidia.dali.pipelinePipeline` class- the central and most

Functional API + improved ExternalSource + improved Pipeline #1598

Functional API + improved ExternalSource + improved Pipeline #1598

Conversation

mzient commented Dec 20, 2019 • edited Loading

Why we need this PR?

What happened in this PR?

mzient commented Dec 20, 2019

dali-automaton commented Dec 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Dec 20, 2019

klecki left a comment

Choose a reason for hiding this comment

JanuszL commented Dec 20, 2019

klecki commented Dec 20, 2019

klecki commented Dec 20, 2019

mzient commented Dec 20, 2019 • edited Loading

klecki commented Dec 20, 2019

mzient commented Jan 7, 2020

dali-automaton commented Jan 7, 2020

dali-automaton commented Jan 7, 2020

mzient commented Jan 7, 2020

dali-automaton commented Jan 7, 2020

mzient commented Jan 7, 2020

dali-automaton commented Jan 7, 2020

dali-automaton commented Jan 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL commented Jan 10, 2020

mzient commented Feb 24, 2020

dali-automaton commented Feb 24, 2020

mzient commented Mar 6, 2020

dali-automaton commented Mar 6, 2020

dali-automaton commented Mar 6, 2020

dali-automaton commented Mar 6, 2020

mzient commented Mar 10, 2020

dali-automaton commented Mar 10, 2020

dali-automaton commented Mar 10, 2020

dali-automaton commented Mar 10, 2020

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

lgtm-com bot commented Mar 11, 2020

mzient commented Mar 11, 2020

dali-automaton commented Mar 11, 2020

dali-automaton commented Mar 11, 2020

mzient commented Dec 20, 2019 •

edited

Loading

mzient commented Dec 20, 2019 •

edited

Loading