-
Notifications
You must be signed in to change notification settings - Fork 655
Unexperimentalize operators #6134
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| def check_numba_compatibility_gpu(if_skip=True): | ||
| import nvidia.dali.plugin.numba.experimental as ex | ||
| def check_numba_compatibility_gpu(if_skip=True, use_experimental: bool = False): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
|
!build |
|
CI MESSAGE: [40306038]: BUILD STARTED |
d372995 to
5aa731e
Compare
|
CI MESSAGE: [40306038]: BUILD FAILED |
e630529 to
51e599d
Compare
| # try importing cuda.core as it can be used later to check the compatibility | ||
| # it is okay to fail as it may not be installed, the check later can handle this | ||
| import cuda.core | ||
| except ImportError: |
Check notice
Code scanning / CodeQL
Empty except Note
|
!build |
|
CI MESSAGE: [40349534]: BUILD STARTED |
a3c3ac6 to
a424878
Compare
|
!build |
|
CI MESSAGE: [40350589]: BUILD STARTED |
Greptile SummaryThis PR promotes several DALI operators from experimental to stable status while maintaining full backwards compatibility. The following operators are now available without the
Key Implementation Details:
No issues found. The refactoring is well-executed with proper deprecation handling and comprehensive test coverage for both old and new API paths. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Code
participant ExpAPI as fn.experimental.*
participant StableAPI as fn.*
participant OpImpl as Operator Implementation
Note over User,OpImpl: Before PR (Experimental Only)
User->>ExpAPI: fn.experimental.debayer()
ExpAPI->>OpImpl: experimental__Debayer
OpImpl-->>User: Result
Note over User,OpImpl: After PR (Both APIs Work)
User->>StableAPI: fn.debayer()
StableAPI->>OpImpl: Debayer
OpImpl-->>User: Result
User->>ExpAPI: fn.experimental.debayer()
ExpAPI->>StableAPI: Deprecated alias → Debayer
StableAPI->>OpImpl: Debayer
OpImpl-->>User: Result
|
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.
Additional Comments (6)
-
dali/operators/image/color/equalize.cc, line 37 (link)style: The docstring reference should use
:meth:nvidia.dali.fn.equalize`` for consistency with DALI's documentation standardsNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
dali/python/nvidia/dali/plugin/numba/__init__.pyi, line 35 (link)logic: Return type annotation may be incorrect - setup_fn appears to return None but the signature suggests it takes parameters and returns None
Should the setup_fn callable return type be None or should it return something else based on the actual implementation?
-
dali/python/nvidia/dali/plugin/numba/fn/__init__.pyi, line 36 (link)syntax: The
setup_fnparameter type annotation appears malformed - it hasNoneas a return type in the callable signature but should likely return something or be Optional[Callable[...]] without the trailing None -
dali/operators/generic/resize/tensor_resize_cpu.cc, line 49-62 (link)style: The deprecated schema definition duplicates parent metadata (NumInput, NumOutput, SupportVolumetric, AllowSequences) that is already inherited from "TensorResize". Since AddParent("TensorResize") inherits all properties, these redundant specifications are unnecessary.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
dali/test/python/checkpointing/test_dali_stateless_operators.py, line 643-646 (link)logic: inconsistent with other tests - this test only uses fn.audio_resample but the decorator includes both experimental and non-experimental versions
-
dali/python/nvidia/dali/plugin/numba/__init__.py, line 187 (link)logic: Incorrect variable used: should be
num_insinstead ofnum_outsfor the input shapes calculation
25 files reviewed, 6 comments
a424878 to
d5291dc
Compare
|
!build |
|
CI MESSAGE: [40376844]: BUILD STARTED |
|
CI MESSAGE: [40376844]: BUILD FAILED |
|
CI MESSAGE: [40349534]: BUILD FAILED |
d5291dc to
e1c19f9
Compare
|
!build |
|
CI MESSAGE: [40397505]: BUILD STARTED |
e1c19f9 to
0d5e199
Compare
0d5e199 to
dcb2b1b
Compare
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
|
!build |
|
CI MESSAGE: [40400586]: BUILD STARTED |
|
CI MESSAGE: [40350589]: BUILD FAILED |
|
CI MESSAGE: [40397505]: BUILD FAILED |
|
CI MESSAGE: [40400586]: BUILD FAILED |
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
71135db to
8ffd54c
Compare
|
!build |
|
CI MESSAGE: [40430374]: BUILD STARTED |
|
A general remark: Do we need to extensively test experimental legacy aliases versus unexperimented operators, or would it be sufficient to have just 1–2 basic tests to confirm the operator is available? Since the underlying implementation is the same, I don’t think thorough verification is necessary. |
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 really need to test experimental here, I'd just mark both as tested and run the non-exp one.
stiepan
left a comment
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.
Echoing the message on the tests: I am not a fan of doubling the amount of tests run just to make sure old aliases run the same.
You could either run subset of the suites with experimental or when parametrizing large suite, make sure some of them (every second) is assigned the experimental op, but without changing the total number of tests.
When running with the deprecated op, it is supposed to issue a warning. We have assign_wars context mgr that would fit here nicely to: 1. test if we warn as we are supposed to, 2. prevent the deprecation warnings from showing up and cluttering the logs.
Category:
Refactoring (Redesign of existing code that doesn't affect functionality)
Description:
This PR takes out of
experimentalthe following operators:experimental.audio_resample
experimental.debayer
experimental.equalize
experimental.filter
experimental.tensor_resize
nvidia.dali.plugin.numba.fn.experimental.numba_function
The
experimentalflavours of the operators is kept to maintain backwards compatibility.Tests are configured to test both: experimental and regular flavours of the operators. Since the backwards compatibility is maintained, both of these operators need to be tested.
TODO:
experimentalflavours)decoders.videoAdditional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-4504