-
Notifications
You must be signed in to change notification settings - Fork 615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add documentation to functional API (all fn.*) + New documentation layout #2653
Conversation
Signed-off-by: Joaquin Anton <[email protected]>
!build |
CI MESSAGE: [2037447]: BUILD STARTED |
CI MESSAGE: [2037447]: BUILD PASSED |
Signed-off-by: Joaquin Anton <[email protected]>
!build |
CI MESSAGE: [2059546]: BUILD STARTED |
CI MESSAGE: [2059546]: BUILD PASSED |
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
: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__`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
docs/supported_ops.rst
Outdated
DALI's operations or "functions" are contained in the ``dali.fn`` module and their names are snake-cased | ||
(for example ``dali.fn.image_decoder``). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example or the whole sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
!build |
CI MESSAGE: [2067617]: BUILD STARTED |
CI MESSAGE: [2067617]: BUILD PASSED |
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withnvidia.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.
docs/index.rst
Outdated
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a preference, but I would consider removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove it would be impossible to navigate to the documentation home (no link in the navigation bar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are at least two other home page links. But that's just my preference.
docs/advanced_topics.rst
Outdated
Sharding | ||
-------- | ||
======== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I feel better, when the top-level navigation leads to the same page, but are distinct sub-pages.
docs/advanced_topics.rst
Outdated
C++ API | ||
------- | ||
======= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this section still doesn't deserve to be a top-level link in our docs.
docs/advanced_topics.rst
Outdated
@@ -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 | |||
==================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
dali/python/nvidia/dali/ops.py
Outdated
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()}]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()}]" | |
input_name = f"input[{schema.MinNumInput()}..{schema.MaxNumInput()-1}]" |
docs/installation.rst
Outdated
|
||
- |mxnet link|_ ``mxnet-cu100`` or later. | ||
- |pytorch link|_ or later. | ||
- |tf link|_ or later. | ||
|
||
|
||
Installation | ||
^^^^^^^^^^^^ | ||
NGC Containers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NGC Containers | |
DALI in NGC Containers |
docs/installation.rst
Outdated
.. note:: | ||
---- | ||
|
||
pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing something like: install DALI releases
or official DALI wheels
.
pip
lacks the meaning in the navigation on the right.
docs/installation.rst
Outdated
|
||
Nightly and weekly release channels | ||
----------------------------------- | ||
pip (Nightly and weekly releases) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip (Nightly and weekly releases) | |
pip - Nightly and weekly releases |
Here we have the nightly and weekly releases mentioned.
docs/installation.rst
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip (DALI 0.22 and lower) | |
pip - old/legacy DALI versions/legacy DALI packages |
docs/operations_table.py
Outdated
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')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
docs/advanced_topics_pipe_run.rst
Outdated
|
||
- | 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`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- | :meth:`nvidia.dali.pipeline.Pipeline.schedule_runs`, | |
- | :meth:`nvidia.dali.pipeline.Pipeline.schedule_run`, |
Signed-off-by: Joaquin Anton <[email protected]>
Signed-off-by: Joaquin Anton <[email protected]>
!build |
CI MESSAGE: [2079096]: BUILD STARTED |
CI MESSAGE: [2079096]: BUILD FAILED |
Signed-off-by: Joaquin Anton <[email protected]>
!build |
CI MESSAGE: [2079410]: BUILD STARTED |
CI MESSAGE: [2079410]: BUILD PASSED |
^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention define_graph
here?
return longest_str | ||
|
||
op_name_max_len = len(longest_fn_string()) | ||
name_bar = op_name_max_len * '=' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it used anywhere.
longest_str = fn_string | ||
return longest_str | ||
|
||
op_name_max_len = len(longest_fn_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be in a global scope?
Signed-off-by: Joaquin Anton [email protected]
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
What solution was applied:
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]