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

Python wrapper for numba #2835

Merged
merged 18 commits into from
Apr 21, 2021
Merged

Python wrapper for numba #2835

merged 18 commits into from
Apr 21, 2021

Conversation

majra20
Copy link
Contributor

@majra20 majra20 commented Apr 1, 2021 •
edited
Loading

Signed-off-by: Rafal Maj rmaj@nvidia.com

Why we need this PR?

  • It allows to invoke numba function using dali for allocating resources.

What happened in this PR?

  • What solution was applied:
    [ I created python wrapper hiding declaration of cfunc from user. Also python wrapper is responsible for getting carrays from pointers for user. ]
  • Affected modules and functionalities:
    [ New operator was added. ]
  • Key points relevant for the review:
    [ It's new feature so everything is important. ]
  • Validation and testing:
    [ nosetests dali/test/python/test_operator_numba_func.py ]
  • Documentation (including examples):
    [ Notebook with example needs to be created. ]

JIRA TASK: DALI-1939

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 1, 2021

This pull request introduces 5 alerts when merging e0a26fae8fddbbc3fcf7180e2c67ea0cbd645af4 into f3262f7 - view on LGTM.com

new alerts:

  • 2 for First parameter of a method is not named 'self'
  • 2 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 6, 2021

This pull request introduces 5 alerts when merging c79117296af7a6b325b2293322622f16d184fadf into c191574 - view on LGTM.com

new alerts:

  • 2 for First parameter of a method is not named 'self'
  • 2 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2021

This pull request introduces 3 alerts when merging 8e199cc813bd8f89909cf586e6fc584480a3dcd8 into c191574 - view on LGTM.com

new alerts:

  • 2 for First parameter of a method is not named 'self'
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 8, 2021

This pull request introduces 5 alerts when merging ab9b8f11e867e595e6e1270d4561cfe8d7386dd1 into f8198dc - view on LGTM.com

new alerts:

  • 5 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 13, 2021

This pull request introduces 9 alerts when merging ea444be9c2a61dc40702645052670174de5e059b into f8198dc - view on LGTM.com

new alerts:

  • 7 for Unused import
  • 2 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 14, 2021

This pull request introduces 14 alerts when merging e074349a8b60cf0ecae3a88a9b65ce700efb0f9e into ed24c32 - view on LGTM.com

new alerts:

  • 8 for Unused local variable
  • 4 for Unused import
  • 1 for Unreachable code
  • 1 for Suspicious unused loop iteration variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 15, 2021

This pull request introduces 11 alerts when merging 2cd890ad13101f3e560f7e72d3e1bafdf109601c into 70ed742 - view on LGTM.com

new alerts:

  • 7 for Unused local variable
  • 4 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 15, 2021

This pull request introduces 11 alerts when merging bff01758154c7985db3fb417274255fe3470fb93 into c5794be - view on LGTM.com

new alerts:

  • 7 for Unused local variable
  • 4 for Unused import

@majra20 majra20 marked this pull request as ready for review April 16, 2021 08:35
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 16, 2021

This pull request introduces 6 alerts when merging 33c79201b49dd9bdcf814fb112dfa9999c9f8989 into dc0fa17 - view on LGTM.com

new alerts:

  • 5 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 16, 2021

This pull request introduces 2 alerts when merging 0e5c2984a30cfd9139de3717913cff21d377b92e into dc0fa17 - view on LGTM.com

new alerts:

  • 2 for Unused local variable

banasraf
banasraf reviewed Apr 19, 2021
View reviewed changes
dali/operators/numba_function/numba_func.cc Outdated
Comment on lines 81 to 83
for sample_id in range(len(out0_samples)):
out0 = out0_samples[sample_id]
in0 = in0_samples[sample_id]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

Suggested change
for sample_id in range(len(out0_samples)):
out0 = out0_samples[sample_id]
in0 = in0_samples[sample_id]
for out0, in0 in zip(out0_samples, in0_samples):

banasraf
banasraf reviewed Apr 19, 2021
View reviewed changes
Copy link
Collaborator

@banasraf banasraf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally, looks nice. I've left a few comments.

@majra20
Copy link
Contributor Author

majra20 commented Apr 20, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2285741]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2285741]: BUILD FAILED

@majra20
Copy link
Contributor Author

majra20 commented Apr 20, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2286039]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2286039]: BUILD PASSED

Rafal Maj added 17 commits April 21, 2021 11:33
initial python wrapper
06f6a4e 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Adjusting numba_func.cc
190eee0 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
a3424f0 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
7eb3ad2 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
896b1f6 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
d271462 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
cde8f4d 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Update fork
5821575 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Final version
cf3b9e3 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
rebase
b3e9dc9 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
rebase
5309cfa 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove unused imports
6162fcb 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove unused imports
2feece7 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Add operator to experimental package
ae8d330 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Remove NumbaFuncBase
b9d65a7 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Code review adjustment
67f2ff7 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
Documentation fix
2bb4e1d 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2289796]: BUILD FAILED

@JanuszL
Copy link
Contributor

JanuszL commented Apr 21, 2021

@majra20 - I see that docs ara failing because there is no numba available. Can you try to change in the docs/conf.py from

with mock(["torch"]):

to

with mock(["torch", "numba"]):

?

add numba dep to docs
a299ed6 
Signed-off-by: Rafal Maj <rmaj@nvidia.com>
@majra20
Copy link
Contributor Author

majra20 commented Apr 21, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2289953]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2289953]: BUILD PASSED

@JanuszL
Copy link
Contributor

JanuszL commented Apr 21, 2021

Looks weird now (but works!):
image

jantonguirao
jantonguirao reviewed Apr 21, 2021
View reviewed changes
dali/operators/numba_function/numba_func.cc
Comment on lines +107 to +108
.AddArg("out_types", R"code(Dali types of outputs.)code", DALI_PYTHON_OBJECT)
.AddArg("in_types", R"code(Dali types of inputs.)code", DALI_PYTHON_OBJECT)
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Replace DALI_PYTHON_OBJECT with DALI_DATA_TYPE_VEC when we support it

@majra20 majra20 merged commit 4facba5 into NVIDIA:master Apr 21, 2021
@majra20 majra20 deleted the numba_func_python_wrapper branch April 21, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awolant awolant awolant approved these changes

@banasraf banasraf banasraf approved these changes

@jantonguirao jantonguirao jantonguirao approved these changes

Assignees

@banasraf banasraf

@awolant awolant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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