Merge uniform input/argument pairs in ndd, rename Clang build tag by jantonguirao · Pull Request #6503 · NVIDIA/DALI

jantonguirao · 2026-09-22T17:04:57Z

Category:

Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

Some operators expose a positional input and an optional argument that carry the same value,
differing only in device placement (Mode spec, "Uniform inputs and arguments"). In the dynamic
(ndd) API this showed up as two redundant parameters for the same thing. This PR merges each
pair into one canonical parameter in the generated ndd signatures:

Reshape/Reinterpret — merged into shape.
WarpAffine — merged into matrix, with GPU-placed values routed through as a positional
input since arguments must stay CPU-only.
Slice — merged start/rel_start/shape/rel_shape, without GPU routing, since Slice
requires anchor+shape together or not at all, so a lone GPU value can't be routed.

The fn/ops APIs are unaffected — both parameters remain available there, as before.

Also renamed "Building DALI with Clang (Experimental)" to "... (Unofficial)" in the docs, aligning
it with the docs' own existing definition of "unofficial" and avoiding a clash with the
actively-maintained nvidia.dali.experimental namespace.

During verification, a regression was found and fixed: the initial version of this PR hid
WarpAffine's merged mtx input from the generated ndd signature, but the code that decides
whether the signature keeps a positional catch-all ran before that filtering — so hiding mtx
also broke backward-compatible positional calls (ndd.warp_affine(img, matrix_tensor)) and made
the new GPU-routing feature itself throw a TypeError on every call. This was caught by actually
building, installing, and running the test suite on GPU hardware (not just static review),
root-caused, and fixed — see commit history. Applying the Slice merge surfaced and fixed the same
class of bug for its two-input case.

Additional information:

Affected modules and functionalities:

nvidia.dali.experimental.dynamic (ndd) signature generation for Reshape, Reinterpret,
WarpAffine, and Slice.
Docs: renamed the Clang build page from "Experimental" to "Unofficial".

Key points relevant for the review:

The fix to the positional-catch-all ordering bug (see Description) — it affects any future
operator that hides a merged input from its ndd signature.
Correctness of GPU-routed matrix for WarpAffine (mixed CPU-image/GPU-matrix and full-GPU
cases) and of the merged start/shape args for Slice.

Tests:

Existing tests apply
- dali/test/python/experimental_mode/test_output_metadata.py: full suite (24/24), including
  test_per_frame_warp (previously broken by the regression described above)
- dali/test/python/ndd_vs_fn/test_ndd_vs_fn_image.py: full suite (124/124)
- dali/test/python/ndd_vs_fn/test_ndd_vs_fn.py::test_slice
New tests added
- Python tests
- GTests
- Benchmark
- Other
N/A

Verified end-to-end with a real incremental build + install on GPU hardware (not just static
analysis):

make -j incremental build and pip install --force-reinstall of this branch succeed.
reshape/warp_affine/slice: merged inputs (shape_input, mtx, anchor) confirmed absent
from the generated ndd signatures and docstrings; fn API signatures unchanged.
warp_affine(img, matrix=<GPU Tensor>) produces numerically correct output, verified against
the same warp with a CPU-placed matrix, for both mixed CPU-image/GPU-matrix and full-GPU inputs.
slice with the merged start/shape args matches fn.slice and plain NumPy slicing
byte-for-byte.
flake8/black --check clean on all edited files.

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-4444, DALI-4507

copy-pr-bot · 2026-09-22T17:05:01Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

greptile-apps · 2026-09-22T17:10:18Z

Confidence Score: 5/5

[Medium risk] Merges positional inputs into arguments for the dynamic API.

The PR appears safe to merge from a correctness standpoint, with only the previously reported naming issue outstanding.

Findings

Boolean helper naming ▶

Summary

The PR merges redundant positional inputs and arguments in generated dynamic API signatures, routes GPU-placed WarpAffine matrices through the input path, and renames the Clang build documentation heading.

No changes were made after the previous review.
No new findings were identified.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Dynamic operator call] --> B{Merged argument}
    B -->|GPU WarpAffine matrix| C[Route as positional input]
    B -->|Other value| D[Keep as argument]
    C --> E[Resolve backend and execute]
    D --> E

_{Reviews (8) · Last reviewed commit: "Fix GPU-matrix capture wiring and GPU va..."}

Copilot

Copilot review overview

🟡 Changes recommended

Dynamic GPU routing and generated dynamic signatures still have unresolved correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Generated operator calls omit GPU keyword routing · New
Dynamic stubs expose hidden positional inputs · New

What changed in this PR

This pull request merges redundant dynamic API input/argument pairs and renames the Clang build documentation label.

Changes:

Hides merged dynamic inputs and updates related documentation.
Routes GPU WarpAffine matrices through positional inputs.
Renames “Experimental” Clang build wording to “Unofficial”.

File	Description
`docs/compilation.rst`	Updates Clang build terminology.
`dali/python/nvidia/dali/ops/_names.py`	Defines merged input/argument mappings.
`dali/python/nvidia/dali/ops/_docs.py`	Hides merged inputs from dynamic documentation.
`dali/python/nvidia/dali/experimental/dynamic/_op_builder.py`	Updates dynamic signatures and GPU routing.
`dali/operators/image/remap/warp_affine.cc`	Documents dynamic GPU matrix handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

JanuszL · 2026-09-22T17:34:59Z

Would you mind adding tests for that functionality?

jantonguirao · 2026-09-23T09:52:54Z

!build

dali-automaton · 2026-09-23T09:55:34Z

CI MESSAGE: [69437123]: BUILD STARTED

dali-automaton · 2026-09-23T14:49:57Z

CI MESSAGE: [69437123]: BUILD FAILED

jantonguirao · 2026-09-23T15:33:38Z

!jenkins

Some operators expose a positional input and an optional argument that carry the same value, differing only in device placement (Mode spec, "Uniform inputs and arguments"). In the dynamic (ndd) API this showed up as two redundant parameters for the same thing. Merge them into one canonical parameter for Reshape/Reinterpret (`shape`) and WarpAffine (`matrix`, with GPU-placed values routed through as a positional input since arguments must stay CPU-only). Slice is left for a follow-up: its redundancy is many-to-one (six mutually exclusive args map onto two inputs) rather than a clean 1:1 pair. Also rename "Building DALI with Clang (Experimental)" to "... (Unofficial)", aligning it with the docs' own existing definition of "unofficial" and avoiding a clash with the actively-maintained nvidia.dali.experimental namespace (DALI-4507). Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

f5c9c81 merged WarpAffine's redundant "mtx" positional input into the "matrix" argument for the dynamic (ndd) API, hiding "mtx"/"mtx=None" from the generated call signature by name. But `_get_inputs` had already picked a hard-stop "*" (not "*inputs") for WarpAffine's signature, since NumInput(1, 2) with InputDox present makes num_separate_inputs equal max_inputs before that filtering runs. With the merged name removed but the hard stop left untouched, the generated signature could no longer accept any further positional argument, which broke two things: - Backward-compatible positional calls, e.g. `ndd.warp_affine(input, matrices)` (raising "warp_affine() takes 1 positional argument but 2 were given"). - The matrix= GPU-routing path itself: fn_call tries to smuggle a GPU-placed `matrix=` value through as an extra positional input to op_inst(*inputs, ...), which needs that same positional slot. Restore the "*inputs" catch-all for GPU-routable merged cases once the merged name is filtered out, so the positional slot survives. Verified empirically post-fix: fn-style positional calls work again, and matrix= now correctly accepts GPU-placed Tensor/Batch values for both mixed CPU-image/GPU-matrix and full-GPU inputs, matching same-backend CPU-matrix results exactly. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

Slice's redundancy is many-to-one: its `anchor` input is equivalent to giving either `start` (absolute) or `rel_start` (relative), and its `shape` input (disambiguated as `shape_input`) is equivalent to giving either `shape` or `rel_shape`. f5c9c81 left this case for a follow-up since it doesn't fit the clean 1:1 pair merged for Reshape/Reinterpret/ WarpAffine. Generalize the merge machinery (`_names.MERGED_MULTI_INPUT_ARGS` / `get_merged_input_names`) to hide a set of inputs per operator instead of a single one, and wire it into the ndd call/fn-wrapper builders and docs generator alongside the existing single-input case. Unlike WarpAffine's `matrix`, there is no GPU-routing fallback for Slice: the operator requires `anchor` and `shape` to be given together as positional inputs or not at all, so a GPU value for only one of the merged argument groups can't be routed as a lone extra positional input. `anchor`/`shape_input` were still reachable positionally before this merge (`slice(x, 0.1, 0.5, axes=...)`, mirroring `fn.slice`, exercised by the pre-existing dali/test/python/ndd_vs_fn/test_ndd_vs_fn.py:: test_slice). Extend `_filter_merged_inputs`'s positional catch-all restoration (added in the previous commit for WarpAffine) to multi-input merged cases too, so hiding the inputs by name doesn't also take away their positional slots. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

…s __call__ too Greptile flagged that a GPU-placed `matrix=` kwarg for WarpAffine wasn't visible to backend resolution: `_capture_intercept`'s `wrapper` calls `_resolve_backend` using only the raw positional `inputs`, before `fn_call` pops `matrix` out of kwargs and appends it to `inputs`. So `ndd.warp_affine(cpu_image, matrix=gpu_matrix)` silently resolved to the CPU backend instead of GPU. Separately, Copilot flagged that the class-based `__call__` (`build_call_function`) never did this routing at all - it forwards `raw_kwargs` straight to `_process_params`, which forces every kwarg argument to CPU, so `ndd._ops.WarpAffine(device="gpu")(img, matrix=gpu_m)` silently copied the GPU matrix to CPU. Factor the routing into a shared `_route_gpu_merged_arg` helper and call it in both places: at the top of `_capture_intercept`'s `wrapper` (before backend resolution) and in `build_call_function`'s `call()` (before `_process_params`). `build_fn_wrapper`'s `fn_call` now calls the same helper too (mostly a no-op there once `wrapper` already routed it, but keeps `fn_call` correct if ever invoked directly). Move `_MERGED_ARG_GPU_INPUT` into `nvidia.dali.ops._names` alongside `MERGED_INPUT_ARGS`/`MERGED_MULTI_INPUT_ARGS`, and add `merge_restores_catchall`, so this GPU-routing information is available to typing/signature generation as well as the dynamic-API runtime. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

…ument merge map Copilot flagged that `_get_positional_input_params` (used to generate the dynamic (ndd) API's typed signatures, including `.pyi` stubs) had no knowledge of `_names.MERGED_INPUT_ARGS`/`MERGED_MULTI_INPUT_ARGS`. For non-GPU-routable merges like Reshape/Reinterpret's `shape_input`, the runtime signature deliberately drops the positional slot (see `dynamic._op_builder._filter_merged_inputs`), but the generated typing kept advertising it, so an IDE-suggested positional call fails at runtime with a TypeError. Add `_filter_merged_input_params`, mirroring `_op_builder`'s filtering logic for the dynamic API only: hide merged input names, and restore a variadic catch-all where the runtime keeps one (GPU-routable and multi-input merged cases). Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

…ture wrapper Routing the GPU `matrix=` value into `inputs` inside `_capture_intercept`'s wrapper also changed what capture classification sees. Classification maps `inputs`/`raw_kwargs` back to the call's source by position/name, so a value moved from a keyword to an extra positional slot no longer has a source node and can't be resolved as a capturable constant, making the call fall back to eager. Route only a copy for `_resolve_backend`, and leave the actual routing to `fn_call` as before. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

- CPU `matrix=` keeps going through the argument path (fn and class API). - GPU `matrix=` results match the CPU-argument and GPU-positional forms. - Batch and per-frame GPU matrices. - Capture mode gives the same results as eager mode for a GPU `matrix=`. - Signature generation: Slice (multi-input merge keeps the catch-all), Reinterpret, and fn/ops APIs still listing the merged inputs. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

…order _spy_process_params()'s calls[-1] assumed the op under test is always the last _process_params call recorded, but a GPU-routed argument makes the runtime insert its own internal Copy calls around the op, so the op's own call isn't reliably last. Record the operator class name per call and pick the WarpAffine call explicitly instead. Also test_warp_affine_ops_class_gpu_matrix_routes_to_input reused the same stateful WarpAffine instance for a second call with a different input count (CPU-argument matrix instead of GPU-routed input), which Operator._check_compatible correctly rejects; use a fresh instance for that comparison call. Verified by rebuilding DALI from this branch and running all 8 test_merged_input_args tests across all 4 eval modes on a real GPU. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

…eager Addresses Greptile review comment: the eager/captured comparison assumes equal-length sequences, so make that invariant explicit at the iteration site instead of relying only on the preceding length assertion. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

Copilot

Copilot review overview

🟡 Changes recommended

GPU routing during capture and for supported GPU value types remains unresolved.

Review effort: Lite
Findings: 2 · 1

Open (3)

Capture graph loses GPU placement for matrix kwargs · New
GPU values incorrectly routed through CPU-only argument conversion · New
Add Slice coverage for all dynamic start and shape paths · New

Resolved since last review (2)

Generated operator calls omit GPU keyword routing
Dynamic stubs expose hidden positional inputs

…, extend Slice merge coverage Addresses Copilot review comments on NVIDIA#6503: a GPU-placed WarpAffine matrix recorded during capture was always forced back to CPU at graph-wiring time since it's classified as a kwarg regardless of routing; route it into the positional input at wiring time instead, mirroring the eager path. _is_gpu_tensor_or_batch only recognized concrete Tensor/Batch, missing invariant-wrapped and raw GPU array/DLPack values accepted elsewhere on the same path; detect device the same way Operator._process_params does. Also extends ndd-vs-fn Slice coverage to the start/shape/rel_shape named forms.

greptile-apps · 2026-09-29T14:45:59Z

+}
+
+
+def merge_restores_catchall(schema_name):


Boolean helper naming

The new merge_restores_catchall helper returns a boolean, but its name does not follow the repository directive that Python boolean queries use an is_ or has_ prefix. Please rename the helper and its callers to satisfy this requirement before merging.

Rule Used: Methods that return bool should be named with Is*, Has*, Can*, or Should* (C++) / is_*, has_* (Python). Imperative-sounding names (HideArgument, Convert) read as commands, not queries. (source)

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!

…, extend Slice merge coverage Addresses Copilot review comments on NVIDIA#6503: a GPU-placed WarpAffine matrix recorded during capture was always forced back to CPU at graph-wiring time since it's classified as a kwarg regardless of routing; route it into the positional input at wiring time instead, mirroring the eager path. _is_gpu_tensor_or_batch only recognized concrete Tensor/Batch, missing invariant-wrapped and raw GPU array/DLPack values accepted elsewhere on the same path; detect device the same way Operator._process_params does. Also extends ndd-vs-fn Slice coverage to the start/shape/rel_shape named forms. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>

Copilot AI lite review requested due to automatic review settings September 22, 2026 17:04

Copilot started reviewing on behalf of jantonguirao September 22, 2026 17:05 View session

greptile-apps Bot reviewed Sep 22, 2026

View reviewed changes

Comment thread dali/python/nvidia/dali/experimental/dynamic/_op_builder.py Outdated

Comment thread dali/python/nvidia/dali/experimental/dynamic/_op_builder.py Outdated

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Comment thread dali/python/nvidia/dali/experimental/dynamic/_op_builder.py Outdated

Comment thread dali/python/nvidia/dali/ops/_names.py

greptile-apps Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread dali/test/python/experimental_mode/test_merged_input_args.py Outdated

dali-automaton assigned mdabek-nvidia and mzient Sep 23, 2026

greptile-apps Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread dali/test/python/experimental_mode/test_merged_input_args.py Outdated

jantonguirao added 9 commits September 29, 2026 13:06

jantonguirao force-pushed the fix/dali-4444-4507-uniform-args-clang-tag branch from 7d29c4f to 0c1548c Compare September 29, 2026 11:07

jantonguirao requested a lite review from Copilot September 29, 2026 11:10

Copilot started reviewing on behalf of jantonguirao September 29, 2026 11:10 View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Comment thread dali/python/nvidia/dali/experimental/dynamic/_capture.py

Comment thread dali/python/nvidia/dali/experimental/dynamic/_op_builder.py Outdated

Comment thread dali/python/nvidia/dali/ops/_names.py

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

jantonguirao force-pushed the fix/dali-4444-4507-uniform-args-clang-tag branch from 4ad3c25 to 37da0fa Compare September 29, 2026 15:58

		}


		def merge_restores_catchall(schema_name):

Conversation

jantonguirao commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Uh oh!

copy-pr-bot Bot commented Sep 22, 2026

Uh oh!

greptile-apps Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Findings

Diagram

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

JanuszL commented Sep 22, 2026

Uh oh!

Uh oh!

jantonguirao commented Sep 23, 2026

Uh oh!

dali-automaton commented Sep 23, 2026

Uh oh!

dali-automaton commented Sep 23, 2026

Uh oh!

jantonguirao commented Sep 23, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jantonguirao commented Sep 22, 2026 •

edited

Loading

greptile-apps Bot commented Sep 22, 2026 •

edited

Loading