Document dynamic operator wrapper arguments#6363
Conversation
| return _numpydoc_formatter( | ||
| "seed", | ||
| "int", | ||
| "The seed to use for the random number generator.", |
There was a problem hiding this comment.
Please share your ideas regarding the docs itself
|
| Filename | Overview |
|---|---|
| dali/python/nvidia/dali/experimental/dynamic/_op_builder.py | Fixes the batch_size/device signature split (was a single merged string), pre-populates used_kwargs so schema args cannot collide, and corrects the seed removal to use a list comprehension instead of the old exact-string check that missed "seed=None". |
| dali/python/nvidia/dali/ops/_docs.py | Adds _get_batch_size_doc() and _get_device_doc() helpers and emits them in _get_kwargs whenever the dynamic API wrapper includes those args; copyright year updated to 2026. |
| docs/examples/image_processing/decoder/dynamic_mode.ipynb | Removes the now-unsupported seed=seed kwarg from a random operator call, consistent with build_fn_wrapper dropping seed from the signature of random ops in favour of rng. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["build_fn_wrapper(op)"] --> B["signature_args = ['batch_size=None', 'device=None']\nused_kwargs = {'batch_size', 'device'}"]
B --> C["Loop schema.GetArgumentNames()"]
C --> D["Add arg to used_kwargs & signature_args"]
D --> E{"op._has_random_state_arg?"}
E -- Yes --> F["Append 'rng=None' to signature_args\nAdd 'rng' to used_kwargs\nRemove 'seed' from used_kwargs\nFilter 'seed*' from signature_args"]
E -- No --> G["Skip seed removal"]
F --> H["header = fn_name(inputs + signature_args)"]
G --> H
H --> I["_docstring_generator_fn(schema, api='dynamic', args=used_kwargs)"]
I --> J["_get_kwargs: schema args\n+ batch_size doc\n+ device doc\n+ rng doc (if present)"]
Reviews (12): Last reviewed commit: "Fix" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile review |
| # Remove 'seed' from used_kwargs and signature_args if present | ||
| if "seed" in used_kwargs: | ||
| used_kwargs.remove("seed") | ||
| if "seed" in signature_args: | ||
| signature_args.remove("seed") |
There was a problem hiding this comment.
@mzient does this make sense? It seems to work but maybe you know the reason why it was placed here on the first place.
There was a problem hiding this comment.
Dynamic mode doesn't have a "seed" argument. It has RNG. That's why we remove it.
There was a problem hiding this comment.
Restored and fixed 🤞
|
!build |
|
CI MESSAGE: [52151451]: BUILD STARTED |
|
CI MESSAGE: [52151451]: BUILD PASSED |
|
!build |
|
CI MESSAGE: [52221054]: BUILD STARTED |
|
!build |
|
CI MESSAGE: [52221741]: BUILD STARTED |
| for arg in signature_args: | ||
| if "seed" in arg: | ||
| signature_args.remove(arg) |
There was a problem hiding this comment.
| for arg in signature_args: | |
| if "seed" in arg: | |
| signature_args.remove(arg) | |
| signature_args = {arg for arg in signature_args if "seed" not in arg} |
|
!build |
|
CI MESSAGE: [52225103]: BUILD STARTED |
| used_kwargs.remove("seed") | ||
| if "seed" in signature_args: | ||
| signature_args.remove("seed") | ||
| signature_args = {arg for arg in signature_args if "seed" not in arg} |
There was a problem hiding this comment.
set, breaking inputs + signature_args for all random operators
{arg for arg in ...} is a set comprehension, not a list comprehension. After this line signature_args is a set, so inputs + signature_args on the very next effective statement (header = f"{fn_name}({', '.join(inputs + signature_args)})") raises TypeError: can only concatenate list (not "set") to list for every operator where _has_random_state_arg is True. All random dynamic operators (CoinFlip, GaussianNoise, Uniform, etc.) will fail to build at import time.
| signature_args = {arg for arg in signature_args if "seed" not in arg} | |
| signature_args = [arg for arg in signature_args if "seed" not in arg] |
- Exposes batch_size and device as distinct dynamic operator wrapper parameters so they can be tracked as reserved kwargs. Add generated numpydoc entries for batch_size, device, rng, and seed when those arguments are present in dynamic operator signatures. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
!build |
|
CI MESSAGE: [52230441]: BUILD STARTED |
|
CI MESSAGE: [52221741]: BUILD FAILED |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
!build |
|
CI MESSAGE: [52240621]: BUILD STARTED |
|
CI MESSAGE: [52230441]: BUILD FAILED |
|
CI MESSAGE: [52240621]: BUILD PASSED |
4 participants
Category:
Other
Description:
This PR documents implicit dynamic operator wrapper arguments and keeps
them reserved when building dynamic operator call signatures.
Dynamic operator wrappers now expose
batch_sizeanddeviceas distinctsignature arguments, while tracking them as used keyword names so schema
arguments do not collide with them. The generated dynamic API docstrings
also include numpydoc entries for
batch_size,device,rngwhen those arguments are present.
Fixes hiding of
seedargument in the dynamic mode.Additional information:
Affected modules and functionalities:
Dynamic mode Python operator wrapper generation and operator docstring
generation.
Key points relevant for the review:
Review the handling of reserved dynamic wrapper arguments in
experimental/dynamic/_op_builder.pyand the conditional documentationgeneration in
ops/_docs.py.Exact docs wording.
Tests:
No tests were run; this is a narrow Python signature/docstring generation
change.
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A