-
Notifications
You must be signed in to change notification settings - Fork 655
Make EvalMode.eager the default #6152
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
Signed-off-by: Rostan Tabet <[email protected]>
Introduces a utility decorator that automatically runs tests with different EvalMode settings. Applied to all existing experimental mode tests. Signed-off-by: Rostan Tabet <[email protected]>
Signed-off-by: Rostan Tabet <[email protected]>
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.
Greptile Overview
Greptile Summary
This PR changes the default evaluation mode from deferred to eager and enhances test coverage by running tests across all evaluation modes.
Key Changes:
- Default
EvalModechanged fromdeferredtoeagerfor more immediate error detection - Fixed slice handling bug where negative stop indices (e.g.,
[:-1]) were incorrectly filtered out - Added
@eval_modes()decorator utility to automatically test functions across all eval modes (deferred, eager, sync_cpu, sync_full) - Updated 4 test files to use the new decorator, ensuring comprehensive eval mode coverage
Note: Copyright year updated to 2026 in all modified files.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- All changes are well-structured and focused: the default eval mode change is intentional and documented in JIRA-4558, the slice bug fix corrects existing behavior, and test enhancements improve coverage. The implementation of the
@eval_modes()decorator is clean and correctly handles parameter composition with existing@paramsdecorators. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| dali/python/nvidia/dali/experimental/dynamic/_eval_mode.py | 5/5 | Changed default eval mode from deferred to eager (line 43), updated copyright year to 2026 |
| dali/python/nvidia/dali/experimental/dynamic/_tensor.py | 5/5 | Fixed slice handling by removing incorrect filter on negative stop indices (line 863), updated copyright year to 2026 |
| dali/test/python/experimental_mode/eval_mode_utils.py | 5/5 | New test utility providing @eval_modes() decorator to parametrize tests across all eval modes |
Sequence Diagram
sequenceDiagram
participant User
participant Test
participant EvalMode
participant ThreadLocalState
participant Operation
Note over User,Operation: Before PR: Default was deferred
User->>Test: Run test
Test->>ThreadLocalState: Initialize eval_mode_stack
ThreadLocalState->>EvalMode: Set default = deferred
Test->>Operation: Execute operation
Operation->>Operation: Defer execution
Note over Operation: Only executes when needed
Note over User,Operation: After PR: Default is eager
User->>Test: Run test with @eval_modes()
loop For each EvalMode
Test->>EvalMode: Enter context (mode)
EvalMode->>ThreadLocalState: Push mode to stack
Test->>Operation: Execute operation
alt mode is eager
Operation->>Operation: Execute immediately
Note over Operation: Input validation happens now
else mode is deferred
Operation->>Operation: Defer execution
else mode is sync_cpu/sync_full
Operation->>Operation: Execute synchronously
end
EvalMode->>ThreadLocalState: Pop mode from stack
end
Signed-off-by: Rostan Tabet <[email protected]>
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.
Greptile Overview
Greptile Summary
This PR changes the default evaluation mode from EvalMode.deferred to EvalMode.eager for DALI's experimental dynamic API. The eager mode provides immediate evaluation start and input validation, offering better error reporting at the cost of potentially less optimization compared to deferred mode.
Key Changes:
- Changed
EvalMode.default = deferredtoEvalMode.default = eagerin_eval_mode.py - Fixed a bug in tensor slicing where negative stop indices (e.g.,
t[:-1]) weren't being passed through to the slice operator - Added comprehensive test coverage with new
@eval_modes()decorator that runs tests across all 4 eval modes (deferred, eager, sync_cpu, sync_full) - Updated 50+ test functions across 4 test files to verify behavior consistency across all eval modes
Test Coverage Enhancement:
The new eval_mode_utils.py provides a parameterization decorator that integrates with the existing @params decorator to multiply test coverage across all evaluation modes. This ensures the dynamic API works correctly regardless of eval mode.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The changes are well-structured and thoroughly tested. The default eval mode change is a behavioral change but non-breaking since all modes remain available. The tensor slicing bug fix is necessary and correct. The decorator pattern for test parameterization is elegant and properly handles both decorated and non-decorated test functions. All test files were updated consistently.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| dali/python/nvidia/dali/experimental/dynamic/_eval_mode.py | 5/5 | Changed default eval mode from deferred to eager on line 43, updated copyright year |
| dali/python/nvidia/dali/experimental/dynamic/_tensor.py | 5/5 | Fixed bug where negative slice indices weren't passed to _tensor_subscript, updated copyright year |
| dali/test/python/experimental_mode/eval_mode_utils.py | 5/5 | New utility file with @eval_modes() decorator for parameterizing tests across all eval modes |
Sequence Diagram
sequenceDiagram
participant User
participant Test as Test Function
participant Decorator as @eval_modes()
participant EvalMode
participant DynamicAPI as Dynamic API (ndd)
User->>Test: Run test
Test->>Decorator: Apply decorator
Decorator->>Decorator: Read existing paramList
Decorator->>Decorator: Generate mode combinations
Note over Decorator: Creates params for all 4 modes:<br/>deferred, eager, sync_cpu, sync_full
loop For each eval mode
Decorator->>EvalMode: __enter__() with mode
EvalMode->>EvalMode: Push mode to stack
Note over EvalMode: If mode==default, use eager<br/>(changed from deferred)
Decorator->>DynamicAPI: Execute test operations
DynamicAPI->>DynamicAPI: Use current eval mode
Note over DynamicAPI: Operations evaluated per mode:<br/>- eager: immediate start<br/>- deferred: lazy eval<br/>- sync_*: synchronous
DynamicAPI-->>Decorator: Return results
Decorator->>EvalMode: __exit__()
EvalMode->>EvalMode: Pop mode from stack
end
Decorator-->>User: All modes tested
Category: New feature (non-breaking change which adds functionality)
Description:
Make
EvalMode.eagerthe default eval mode instead ofEvalMode.deferred. This PR also run some tests with all eval modes.Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Modified tests to make them run with all eval modes:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-4558