Skip to content
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 type annotations for numba and pytorch plugins #5129

Merged
merged 10 commits into from
Nov 8, 2023

Conversation

klecki
Copy link
Contributor

@klecki klecki commented Oct 26, 2023

Category: New feature

Description:

Type annotations are added for numba and pytorch plugins, covering
the operators (Numba function and Torch Python function) available
in the plugin module.

Additionally, type annotations are added to Pytorch data iterators for DALI.

The code is rearranged so the dictionaries reflect the actual module
hierarchy and allow for inserting the interface .pyi files, rather than
generating the modules at runtime (especially the fn ones).

Additional information:

Affected modules and functionalities:

Plugins for Numba and Pytorch (annotations only)

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@klecki klecki force-pushed the plugin_function_annotations branch from dd5b04d to 23e5577 Compare November 3, 2023 16:35
@klecki klecki changed the title Reorganize plugins to reflect actual module hierarchy Add type annotations for numba and pytorch plugins Nov 3, 2023
@klecki klecki marked this pull request as ready for review November 3, 2023 19:36
Comment on lines 37 to 55
class DALIGenericIterator(_DaliBaseIterator):

def __init__(
self,
pipelines: Union[List[Pipeline], Pipeline],
output_map: List[str],
size: int = -1,
reader_name: Optional[str] = None,
auto_reset: Union[str, bool, None] = False,
fill_last_batch: Optional[bool] = None,
dynamic_shape: Optional[bool] = ...,
last_batch_padded: bool = False,
last_batch_policy: LastBatchPolicy = LastBatchPolicy.FILL,
prepare_first_batch: bool = True,
) -> None:
...

def __next__(self) -> List[Dict[str, torch.Tensor]]:
...
Copy link
Contributor

@mzient mzient Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that retyping everything here is the right thing to do - rather the contrary; it's prone to go stale in many subtle ways. Why can't we just annotate the proper code instead?

Even the PEP that defies .pyi files specifies them as a tool rather for injecting type hints than the regular way to do it. I know that we have interface files for automatically generated code or code where the interfaces are mangled in some way (basically all operators). Here it just doesn't apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The technical reason was to keep the TorchPythonFunction without changes, so I put the annotated signatures (that differs from the runtime ones) in the stub file, and thanks to that I need to put all the annotations in the stub, otherwise they become hidden (the annotation file has precedence over which symbols are visible, you can't mix and match this way).

I can move the TorchPythonFunction to a submodule, and move annotations for the iterators back to the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a submodule for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can have TorchPythonFunction annotated with stub file, as this is needed to expand the definition of **kwargs.

Copy link
Contributor Author

@klecki klecki Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@klecki klecki force-pushed the plugin_function_annotations branch 3 times, most recently from 2ba7519 to de70afe Compare November 7, 2023 11:27
@klecki
Copy link
Contributor Author

klecki commented Nov 7, 2023

I extracted some adjustments that I found needed for Python function into a separate PR: #5149

@klecki
Copy link
Contributor Author

klecki commented Nov 7, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10633692]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10633692]: BUILD FAILED

@klecki
Copy link
Contributor Author

klecki commented Nov 7, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10637571]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10637571]: BUILD FAILED

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

WIP

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

WIP

Continue with numba function

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

Numba and python

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

Torch Python Functon

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

Move annotations around

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

Minor revisions

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

Write tests for pytorch, extend TorchFunction overload

Fixup the None return in Python Functions (Sequence doesn't count here).

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

Adjust the type of required return type in the PythonFunction callback

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

Extend tests

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki klecki force-pushed the plugin_function_annotations branch from 7cef8d7 to 4611987 Compare November 8, 2023 12:13
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 8, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10656214]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10656214]: BUILD PASSED

@klecki klecki merged commit aadb7f8 into NVIDIA:main Nov 8, 2023
5 checks passed
@klecki klecki deleted the plugin_function_annotations branch November 8, 2023 14:48
@klecki klecki added the signatures Python API signatures and type hints label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
signatures Python API signatures and type hints
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants