-
Notifications
You must be signed in to change notification settings - Fork 1
[None][feat] AutoDeploy: kwargs-first pipeline #151
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
Conversation
…eploy (NVIDIA#7770) Signed-off-by: Frida Hou <[email protected]> Signed-off-by: Fridah-nv <[email protected]>
Signed-off-by: Bo Deng <[email protected]>
Signed-off-by: Izzy Putterman <[email protected]>
…IDIA#7972) Signed-off-by: Balaram Buddharaju <[email protected]>
…n models (NVIDIA#8070) Signed-off-by: Chang Liu (Enterprise Products) <[email protected]> Signed-off-by: Chang Liu <[email protected]>
…t perf regressions (NVIDIA#8099) Signed-off-by: Eran Geva <[email protected]>
…eedy sampling (NVIDIA#8110) Signed-off-by: ixlmar <[email protected]>
Signed-off-by: Erin Ho <[email protected]>
Signed-off-by: Yifei Zhang <[email protected]>
Signed-off-by: Dongfeng Yu <[email protected]> Signed-off-by: dongfengy <[email protected]> Co-authored-by: Jin Li <[email protected]>
…DIA#8114) Signed-off-by: Shunkang <[email protected]> Signed-off-by: Patrice Castonguay <[email protected]> Co-authored-by: Shunkang <[email protected]>
Signed-off-by: Daniel Campora <[email protected]>
…8116) Signed-off-by: Patrice Castonguay <[email protected]>
… backend (NVIDIA#8092) Signed-off-by: Eran Geva <[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.
Pull Request Overview
This PR transitions the AutoDeploy transform system from using positional arguments to keyword arguments throughout the pipeline. The change addresses challenges with handling mixed nn.Modules and GraphModules by standardizing on kwargs-based interfaces, making the system more robust and maintainable.
Key changes:
- Replaced
*cm.argswith**cm.named_argsacross all model invocations - Removed strict forward method implementations and related configuration
- Updated export functions to use kwargs instead of positional args
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tensorrt_llm/_torch/auto_deploy/export/export.py | Updated torch_export_to_gm to make args optional and handle empty args tuple |
| tensorrt_llm/_torch/auto_deploy/transform/library/*.py | Replaced args-based model calls with kwargs-based calls in transforms |
| tensorrt_llm/_torch/auto_deploy/models/*.py | Removed strict forward method implementations and related infrastructure |
| tensorrt_llm/_torch/auto_deploy/shim/ad_executor.py | Updated model invocation to use kwargs interface |
| tests/unittest/_torch/auto_deploy/unit/singlegpu/**/*.py | Updated test cases to use kwargs-based model calls |
| tensorrt_llm/_torch/auto_deploy/config/*.yaml | Removed kwargs-related configuration options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| gm: GraphModule, | ||
| name: str, | ||
| add_kwargs: bool = False, | ||
| add_kwargs: bool = True, |
Copilot
AI
Oct 2, 2025
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.
Changing the default value of add_kwargs from False to True is a breaking change that could affect existing callers who rely on the previous default behavior. Consider using a deprecation period or explicit migration strategy.
| add_kwargs: bool = True, | |
| add_kwargs: bool = False, |
| DynamicShape = Dict[int, Dim] # indicating the dynamic shape in tensor dimension | ||
| DynamicShapeCallback = Callable[[], DynamicShape] | ||
|
|
||
| Constant = Union[int, float, str, None] |
Copilot
AI
Oct 2, 2025
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.
The Constant type alias is moved from line 767 to line 27, but this creates a duplicate definition since the original at line 767 is not removed in the diff. This could lead to confusion about which definition to use.
Signed-off-by: greg-kwasniewski1 <[email protected]>
…8005) Signed-off-by: Yilin Fan <[email protected]> Signed-off-by: nv-yilinf <[email protected]>
Signed-off-by: Nikita Korobov <[email protected]>
Signed-off-by: Robin Kobus <[email protected]>
…ble_block_reuse (NVIDIA#8108) Signed-off-by: Lucas Liebenwein <[email protected]>
…ackend (NVIDIA#8075) Signed-off-by: Aurelien Chartier <[email protected]>
…DIA#8120) Signed-off-by: Suyog Gupta <[email protected]>
…VIDIA#7998) Signed-off-by: ziyixiong-nv <[email protected]>
…#8126) Signed-off-by: Lucas Liebenwein <[email protected]>
Signed-off-by: Mike Iovine <[email protected]> Signed-off-by: Mike Iovine <[email protected]>
Signed-off-by: Lucas Liebenwein <[email protected]>
|
Could you elaborate on what I feel this is going to affect some pattern matcher behavior because we use |
1dbb9fc to
0aed9b8
Compare
|
see NVIDIA#8137 |
will be transitioned to main repo after NVIDIA#8126 gets merged
PR switches to using kwargs instead of positional args everywhere in our transform system. Why? With mixed nn.Modules and GraphModules coming up, handling positional args reliably is becoming an increasing burden.