Add documentation to functional API (all fn.*) + New documentation layout #2653

jantonguirao · 2021-02-02T14:59:01Z

Signed-off-by: Joaquin Anton [email protected]

Why we need this PR?

Pick one, remove the rest

Changes of documentation, removing the "experimental" note about functional API and adding proper documentation to it

What happened in this PR?

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

What solution was applied:
- Replaced documentation of functional API wrappers (currently a link to operator documentation page) to contain the body of the documentation instead
- It adds operator documentation to functional API wrappers
- It creates a new layout for the documentation, some sections have been expanded in the navigation bar and captions are used for delimitation
- Getting started section has been reduced by moving advanced topics to a separate section (for instance, compiling from source)
- Functional API documentation is now the main "Operations" documentation and the object API (ops.*) is marked as Legacy
- A new table has been added, mapping the functional API operations to the legacy name (ops.*)
Affected modules and functionalities:
Documentation
Key points relevant for the review:
Documentation
Validation and testing:
N/A
Documentation (including examples):
*Documentation of fn *

JIRA TASK: [DALI-1823]

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao · 2021-02-02T14:59:18Z

!build

dali-automaton · 2021-02-02T15:05:37Z

CI MESSAGE: [2037447]: BUILD STARTED

jantonguirao · 2021-02-02T15:17:54Z

dali-automaton · 2021-02-02T16:49:56Z

CI MESSAGE: [2037447]: BUILD PASSED

dali/python/nvidia/dali/fn.py

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao · 2021-02-09T17:41:01Z

!build

dali-automaton · 2021-02-09T17:46:25Z

CI MESSAGE: [2059546]: BUILD STARTED

dali-automaton · 2021-02-09T19:33:59Z

CI MESSAGE: [2059546]: BUILD PASSED

Signed-off-by: Joaquin Anton <[email protected]>

dali/operators/decoder/image_decoder.cc

JanuszL · 2021-02-11T16:46:18Z

dali/pipeline/operator/builtin/external_source.cc

-:meth:`nvidia.dali.pipeline.Pipeline.feed_input` and
+  .DocStr(R"code(Allows externally provided data to be passed as an input to the pipeline.
+
+See :meth:`nvidia.dali.ops.ExternalSource.__init__`,


This leads to strange link from fn API to the legacy one. Maybe we should use only fn here?

Signed-off-by: Joaquin Anton <[email protected]>

JanuszL · 2021-02-11T17:17:13Z

docs/supported_ops.rst

+DALI's operations or "functions" are contained in the ``dali.fn`` module and their names are snake-cased
+(for example ``dali.fn.image_decoder``).


I don't think we need that here.

The example or the whole sentence?

The whole sentence. Like we don't describe the naming convention in the legacy page. It has only value (IMHO) when you compare the legacy with the fn API. In this page we don't have to compare these two approaches.

docs/supported_ops_legacy.rst

docs/operations_table.py

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao · 2021-02-11T18:23:09Z

!build

dali-automaton · 2021-02-11T18:30:28Z

CI MESSAGE: [2067617]: BUILD STARTED

dali-automaton · 2021-02-11T19:51:16Z

CI MESSAGE: [2067617]: BUILD PASSED

Signed-off-by: Joaquin Anton <[email protected]>

dali/pipeline/operator/builtin/external_source.cc

docs/framework_plugins.rst

Signed-off-by: Joaquin Anton <[email protected]>

szalpal · 2021-02-12T12:40:51Z

One minor comment from my side:
I feel, that the "CPU" and "GPU" in the table are hard to distinguish, especially when there's a mix like this:

How about making GPU bold?

Signed-off-by: Joaquin Anton <[email protected]>

klecki

Mostly ok, but:

I would consider adding one or two sentences about the DataNodes in the operations (actual type of input and return value) and their use in pipeline definition.
I would replace all mentions of nvidia.dali.ops. in the Operations and replace them with nvidia.dali.fn (you would need to adjust docstrings).
There is off by one error.
I had some comments to the naming of several sections, but it's rather minor.

klecki · 2021-02-15T12:23:07Z

docs/index.rst

@@ -24,24 +24,48 @@ This library is open sourced and it is available in the `NVIDIA GitHub repositor
 .. toctree::
   :hidden:

-   Documentation home <self>
+   Documentation Home <self>


Just a preference, but I would consider removing it.

If we remove it would be impossible to navigate to the documentation home (no link in the navigation bar)

There are at least two other home page links. But that's just my preference.

klecki · 2021-02-15T12:31:18Z

docs/advanced_topics.rst

 Sharding
--------
+========


Nitpick: I feel better, when the top-level navigation leads to the same page, but are distinct sub-pages.

klecki · 2021-02-15T12:44:23Z

docs/advanced_topics.rst

 C++ API
-------
+=======


I feel like this section still doesn't deserve to be a top-level link in our docs.

klecki · 2021-02-15T12:44:43Z

docs/advanced_topics.rst

@@ -243,8 +190,60 @@ When this occurs, use the first formula.
 To address these challenges, use the ``reader_name`` parameter and allow the iterator
 handle the details.

+
+Pipeline Run Methods
+====================


klecki · 2021-02-15T13:14:59Z

dali/python/nvidia/dali/ops.py

+            ret += _numpydoc_formatter(input_name, input_type_str, dox, True) + "\n"
+        elif extra_opt_args > 1:
+            input_type_str = "TensorList"
+            input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()}]"


Suggested change

input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()}]"

input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()-1}]"

klecki · 2021-02-15T14:17:02Z

docs/installation.rst


  - |mxnet link|_ ``mxnet-cu100`` or later.
  - |pytorch link|_ or later.
  - |tf link|_ or later.


-Installation
-^^^^^^^^^^^^
+NGC Containers


Suggested change

NGC Containers

DALI in NGC Containers

klecki · 2021-02-15T14:18:44Z

docs/installation.rst

-.. note::
+----
+
+pip


I'm missing something like: install DALI releases or official DALI wheels.
pip lacks the meaning in the navigation on the right.

klecki · 2021-02-15T14:19:33Z

docs/installation.rst

-
-Nightly and weekly release channels
-----------------------------------
+pip (Nightly and weekly releases)


Suggested change

pip (Nightly and weekly releases)

pip - Nightly and weekly releases

Here we have the nightly and weekly releases mentioned.

klecki · 2021-02-15T14:20:28Z

docs/installation.rst

+  pip install --extra-index-url https://developer.download.nvidia.com/compute/redist/weekly nvidia-dali-tf-plugin-weekly-cuda110
+
+
+pip (DALI 0.22 and lower)


Suggested change

pip (DALI 0.22 and lower)

pip - old/legacy DALI versions/legacy DALI packages

klecki · 2021-02-15T14:28:55Z

docs/operations_table.py

+                    break
+            if m is not None and hasattr(m, op_name):
+                op_string = link_formatter.format(op = op_full_name, module = module_name)
+                fn_string = link_formatter.format(op = to_fn_name(op_full_name), module = module_name.replace('.ops', '.fn'))


Maybe add to_fn_module, I see that module_name.replace('.ops', '.fn') appears several times in this file.

Signed-off-by: Joaquin Anton <[email protected]>

klecki · 2021-02-16T12:09:32Z

docs/advanced_topics_pipe_run.rst

+
+- | Simple run method, which runs the computations and returns the results.
+  | This option corresponds to the :meth:`nvidia.dali.types.PipelineAPIType.BASIC` API type.
+- | :meth:`nvidia.dali.pipeline.Pipeline.schedule_runs`,


Suggested change

- | :meth:`nvidia.dali.pipeline.Pipeline.schedule_runs`,

- | :meth:`nvidia.dali.pipeline.Pipeline.schedule_run`,

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao · 2021-02-16T13:08:53Z

!build

dali-automaton · 2021-02-16T13:10:55Z

CI MESSAGE: [2079096]: BUILD STARTED

dali-automaton · 2021-02-16T14:32:22Z

CI MESSAGE: [2079096]: BUILD FAILED

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao · 2021-02-16T15:41:52Z

!build

dali-automaton · 2021-02-16T15:45:50Z

CI MESSAGE: [2079410]: BUILD STARTED

dali-automaton · 2021-02-16T18:22:50Z

CI MESSAGE: [2079410]: BUILD PASSED

JanuszL · 2021-02-18T16:37:55Z

docs/math.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^

 DALI allows you to use regular Python arithmetic operations and other mathematical functions in
-the :meth:`~nvidia.dali.pipeline.Pipeline.define_graph` method on the values that are returned
+the :meth:`~nvidia.dali.Pipeline.define_graph` method on the values that are returned


Should we mention define_graph here?

JanuszL · 2021-02-18T16:41:14Z

docs/operations_table.py

+    return longest_str
+
+op_name_max_len = len(longest_fn_string())
+name_bar = op_name_max_len * '='


I don't see it used anywhere.

JanuszL · 2021-02-18T16:41:28Z

docs/operations_table.py

+                    longest_str = fn_string
+    return longest_str
+
+op_name_max_len = len(longest_fn_string())


Does this need to be in a global scope?

Add operator documentation to all fn.*

e103498

Signed-off-by: Joaquin Anton <[email protected]>

mzient reviewed Feb 2, 2021

View reviewed changes

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

WIP First proposal

8c65b8f

Signed-off-by: Joaquin Anton <[email protected]>

WIP

156cf09

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao force-pushed the fn_doc branch from dc187d4 to 156cf09 Compare February 10, 2021 08:10

New documentation layout

0ca079f

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao force-pushed the fn_doc branch from 59a7b06 to 0ca079f Compare February 10, 2021 15:35

Next round

798ec15

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao assigned JanuszL and klecki Feb 11, 2021

JanuszL reviewed Feb 11, 2021

View reviewed changes

dali/operators/decoder/image_decoder.cc Outdated Show resolved Hide resolved

JanuszL reviewed Feb 11, 2021

View reviewed changes

Small changes

08a11b0

Signed-off-by: Joaquin Anton <[email protected]>

JanuszL reviewed Feb 11, 2021

View reviewed changes

docs/supported_ops_legacy.rst Outdated Show resolved Hide resolved

JanuszL reviewed Feb 11, 2021

View reviewed changes

docs/operations_table.py Outdated Show resolved Hide resolved

Code review fixes

748ea0a

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao changed the title ~~Add operator documentation to functional API (all fn.*)~~ Add documentation to functional API (all fn.*) + New documentation layout Feb 11, 2021

Merge remote-tracking branch 'upstream/master' into fn_doc

e198115

Signed-off-by: Joaquin Anton <[email protected]>

Make navigation bar titles consistent

5afd8d0

Signed-off-by: Joaquin Anton <[email protected]>

JanuszL reviewed Feb 12, 2021

View reviewed changes

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

JanuszL reviewed Feb 12, 2021

View reviewed changes

docs/framework_plugins.rst Show resolved Hide resolved

JanuszL approved these changes Feb 12, 2021

View reviewed changes

Remove scrollbar from operations table

6d770f4

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao added 2 commits February 12, 2021 14:42

Code review fixes

420a455

Signed-off-by: Joaquin Anton <[email protected]>

Rebase

eaaf76d

Signed-off-by: Joaquin Anton <[email protected]>

klecki reviewed Feb 15, 2021

View reviewed changes

jantonguirao added 3 commits February 15, 2021 17:21

Code review fixes

68019e3

Signed-off-by: Joaquin Anton <[email protected]>

More code review fixes

3d2f4dc

Signed-off-by: Joaquin Anton <[email protected]>

More code review fixes

de02f99

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao force-pushed the fn_doc branch from f3ed605 to de02f99 Compare February 16, 2021 09:54

klecki reviewed Feb 16, 2021

View reviewed changes

jantonguirao added 2 commits February 16, 2021 13:16

Fix links

cce6ba9

Signed-off-by: Joaquin Anton <[email protected]>

More changes

95d7b8b

Signed-off-by: Joaquin Anton <[email protected]>

jantonguirao force-pushed the fn_doc branch from c065abb to 95d7b8b Compare February 16, 2021 13:06

klecki approved these changes Feb 16, 2021

View reviewed changes

Test fix

7df8c84

Signed-off-by: Joaquin Anton <[email protected]>

JanuszL reviewed Feb 18, 2021

View reviewed changes

JanuszL approved these changes Feb 18, 2021

View reviewed changes

jantonguirao merged commit c5cf044 into NVIDIA:master Feb 22, 2021

JanuszL mentioned this pull request May 19, 2021

DALI 2021 roadmap #2978

Closed

		DALI's operations or "functions" are contained in the ``dali.fn`` module and their names are snake-cased
		(for example ``dali.fn.image_decoder``).

	input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()}]"
	input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()-1}]"

	pip (Nightly and weekly releases)
	pip - Nightly and weekly releases

		pip install --extra-index-url https://developer.download.nvidia.com/compute/redist/weekly nvidia-dali-tf-plugin-weekly-cuda110


		pip (DALI 0.22 and lower)

	pip (DALI 0.22 and lower)
	pip - old/legacy DALI versions/legacy DALI packages

	- \| :meth:`nvidia.dali.pipeline.Pipeline.schedule_runs`,
	- \| :meth:`nvidia.dali.pipeline.Pipeline.schedule_run`,

Add documentation to functional API (all fn.*) + New documentation layout #2653

Add documentation to functional API (all fn.*) + New documentation layout #2653

Conversation

jantonguirao commented Feb 2, 2021 • edited Loading

Why we need this PR?

What happened in this PR?

jantonguirao commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

jantonguirao commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

jantonguirao commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

dali-automaton commented Feb 9, 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

jantonguirao commented Feb 11, 2021

dali-automaton commented Feb 11, 2021

dali-automaton commented Feb 11, 2021

szalpal commented Feb 12, 2021

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

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

jantonguirao commented Feb 16, 2021

dali-automaton commented Feb 16, 2021

dali-automaton commented Feb 16, 2021

jantonguirao commented Feb 16, 2021

dali-automaton commented Feb 16, 2021

dali-automaton commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Feb 2, 2021 •

edited

Loading