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 baseline stubs for Python Tensor types #5153

Merged
merged 10 commits into from
Nov 17, 2023
Merged

Conversation

klecki
Copy link
Contributor

@klecki klecki commented Nov 8, 2023

Category: New feature

Description:

Base for the Python Tensor and TensorList types interface files,
generated using stubgen.

C-based types (or types not guaranteed to be present, like np.array),
are removed from signatures and left as a comment or replaced with Any.

This doesn't necessary add new information, just enables static tools
to access the types.

    imgs, labels = pipe.run()
    reveal_type(imgs)

Before:

type_annotations/test_typing_pipelines.py:63: note: Revealed type is "Union[Any, Any]"

After:

type_annotations/test_typing_pipelines.py:63: note: Revealed type is "Union[nvidia.dali.backend_impl.TensorListCPU, nvidia.dali.backend_impl.TensorListGPU]"

Note that the backend contains many more types, like OpSchema, OpSpec, Pipeline, etc.
They are internal, so I didn't expose them, but we should consider adding them to aid our development.

This file is generated and than manually adjusted, I don't see a clear automated workflow for this.
It appears harder to maintain than a list of supported enum values.

Additional information:

Affected modules and functionalities:

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 marked this pull request as ready for review November 9, 2023 10:55
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
Copy link
Contributor Author

klecki commented Nov 14, 2023

!build

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 14, 2023

After the last changes, mypy declares the return types of Pipeline.run() as

Revealed type is "builtins.tuple[Union[nvidia.dali.tensors.TensorListCPU, nvidia.dali.tensors.TensorListGPU], ...]"

I am intentionally not adjusting any other imports to see if this change doesn't break anything in the existing code.
It would be good to check the generated docs as well.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10788983]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10788983]: BUILD PASSED

@klecki
Copy link
Contributor Author

klecki commented Nov 15, 2023

It would be good to check the generated docs as well.

the docs set the module explicitly, but still, even though we adjusted the module path, the signatures utilize the backend_impl variant. This is generated by the pybind and inserted into the docstring, I don't know if we can easily override it in the backend or maybe we should replace it when it is loaded into python. Both approaches are kinda weird.

Providing the definition as the first element allows the downstream
definitions to utilize this module in pybind-generated docstrings.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 16, 2023

!build

@klecki
Copy link
Contributor Author

klecki commented Nov 16, 2023

obraz
It works

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10841126]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10841126]: BUILD FAILED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10842296]: BUILD STARTED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10842605]: BUILD STARTED

Copy link
Member

@stiepan stiepan left a comment

Choose a reason for hiding this comment

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

FYI: I did not look into the documentation as it's not built yet.

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Nov 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10842605]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10845923]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [10845923]: BUILD PASSED

@klecki klecki merged commit dfca5e9 into NVIDIA:main Nov 17, 2023
5 checks passed
@klecki klecki deleted the tensor-stubgen branch November 17, 2023 10:49
@klecki klecki added the signatures Python API signatures and type hints label Nov 29, 2023
This pull request was closed.
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.

5 participants