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.

Sign up for GitHub

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

Jump to bottom

Add NumbaFunc operator #2804

Merged
merged 29 commits into from
Mar 31, 2021
Merged

Add NumbaFunc operator #2804

merged 29 commits into from
Mar 31, 2021

Conversation

majra20
Copy link
Contributor

@majra20 majra20 commented Mar 18, 2021 •
edited
Loading

Why we need this PR?

  • It adds support for numba function. We can define numba function pass it to NumbaFunc operator and invoke it in pipeline.

What happened in this PR?

  • What solution was applied:
    • Added NumbaFunc operator
  • Affected modules and functionalities:
    • NumbaFunc should invoke function passed as address of function. Function should take as arguments: output ptr, input_ptr, size of input/output (they should match)
  • Key points relevant for the review:
    • Function should be invoked and do requested logic
  • Validation and testing:
    • Python tests: test_operator_numba_func.py
  • Documentation (including examples):
    • Maybe we should add notebook

JIRA TASK: DALI-1921

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 18, 2021

This pull request introduces 3 alerts when merging 2bf15523f2178321cd145ed374578f3b50ea965a into 852ad4e - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

@majra20 majra20 marked this pull request as ready for review March 22, 2021 10:33
jantonguirao
jantonguirao reviewed Mar 22, 2021
View reviewed changes
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
dali/test/python/test_operator_numba_func.py Outdated Show resolved Hide resolved
jantonguirao
jantonguirao reviewed Mar 22, 2021
View reviewed changes
dali/test/python/test_operator_numba_func.py Outdated Show resolved Hide resolved
dali/test/python/test_operator_numba_func.py Outdated Show resolved Hide resolved
dali/operators/numba_function/numba_func.cc Outdated Show resolved Hide resolved
Rafal Maj added 10 commits March 24, 2021 10:06
First version
3c949ae 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Dummy example
560a99a 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
First version with tests
51318c0 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update doc
70f5d66 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove unused import
2692e18 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove unused import, variables
064f719 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Setup function per batch
d0bf44a 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Add output shape, type error handling
4dbe65c 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Use loc variables
975bd7d 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Uncommenting test cases
99e0049 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Change setup and run factory funcs
1547517 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove number of input, outputs
aa01754 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Rafal Maj added 3 commits March 24, 2021 13:11
Update docs
498b413 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update docs
adaa12e 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Docs update, change factory function def
88daf79 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
@majra20
Copy link
Contributor Author

majra20 commented Mar 25, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2203528]: BUILD STARTED

@jantonguirao jantonguirao removed their assignment Mar 25, 2021
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2203528]: BUILD PASSED

JanuszL
JanuszL reviewed Mar 25, 2021
View reviewed changes
dali/operators/numba_function/numba_func.cc
.. code-block:: python

@cfunc(setup_fn_sig(num_outputs, num_inputs), nopython=True)
def callback_setup_func(out1_shape_ptr, out1_ndim, out_dtype_ptr, in1_shape_ptr, in1_ndim, in1_dtype, num_samples)
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thoughts.
How to approach case when you cannot calculate the shape without the access to the input data - like WarpAffine or even Rotate what may extend canvas?

JanuszL
JanuszL reviewed Mar 25, 2021
View reviewed changes
dali/operators/numba_function/numba_func.cc

.. code-block:: python

@cfunc(run_fn_sig([out_numba_types], [in_numba_types]), nopython=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow validate if [out_numba_types], [in_numba_types] matches what callback_setup_func returns?
How about validating the input of the operator inside DALI?

nitpicks
1a65982 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
awolant
awolant approved these changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@awolant awolant left a comment

Choose a reason for hiding this comment

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

Looks good. I'm not sure how this behaves in some more advanced Numba workflows (like chaining functions, callback to Python etc.) and they are not explicitly disallowed here, but this will be probably handled in later PRs.

dali/test/python/test_operator_numba_func.py
test_data_root = get_dali_extra_path()
lmdb_folder = os.path.join(test_data_root, 'db', 'lmdb')

@cfunc(dali_numba.run_fn_sig(types.uint8, types.uint8), nopython=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would be neat to make a decorator that turns a function into NumbaFunc operator. Then we can use it in graph definition. But this is for later PR for sure.

@majra20 majra20 changed the title Numba func Add NumbaFunc operator Mar 31, 2021
@majra20
Copy link
Contributor Author

majra20 commented Mar 31, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2221186]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2221186]: BUILD PASSED

@majra20 majra20 merged commit f86a859 into NVIDIA:master Mar 31, 2021
@JanuszL JanuszL mentioned this pull request May 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jantonguirao jantonguirao jantonguirao left review comments

@awolant awolant awolant approved these changes

@JanuszL JanuszL JanuszL approved these changes

Assignees

@JanuszL JanuszL

@awolant awolant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@majra20 @dali-automaton @jantonguirao @JanuszL @awolant