- Notifications
You must be signed in to change notification settings - Fork376
Graph break overhead#3946
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 ourterms of service andprivacy 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?
Graph break overhead#3946
Conversation
8c9284a to46829e6CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
7541aca to10662c0CompareThere 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.
There are some changes that do not conform to Python style guidelines:
--- /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/_compiler.py2025-12-12 00:21:37.519598+00:00+++ /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/_compiler.py2025-12-12 00:22:13.058405+00:00@@ -947,11 +947,11 @@ # Here we delete the frozen parameters from the graph module. Note this does not affect the submodules. We are going to delete the frozen parameters from the submodules in the convert_module function. # This is done to release CPU memory. for attr in dir(gm): if attr.startswith("_frozen_param"): delattr(gm, attr)-+ trt_module = None for name, _ in partitioned_module.named_children(): submodule = getattr(partitioned_module, name) # filter on the GraphModule if not isinstance(submodule, torch.fx.graph_module.GraphModule):
4813804 to4615705Comparecore/runtime/execute_engine.cpp Outdated
| TORCHTRT_CHECK( | ||
| compiled_engine->exec_ctx->setInputShape(name.c_str(), dims),"Error while setting the input shape"); | ||
| if (shape_changed) { |
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.
Can't we use the same shape keys we used for cudagraphs? Why are we implementing another system?
core/runtime/TRTEngine.cpp Outdated
| num_io =std::make_pair(inputs_size, outputs); | ||
| this->current_device_id =at::cuda::current_device(); | ||
| this->stream =c10::cuda::getCurrentCUDAStream(this->current_device_id); |
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.
Is this PR supposed to implement the stream operator?
| forattrindir(gm): | ||
| ifattr.startswith("_frozen_param"): | ||
| delattr(gm,attr) | ||
| trt_module=None |
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.
What is this? Why does this variable need to be outside the of scope of iterating through the module?
| trt_module=getattr(partitioned_module,name) | ||
| trt_module.setup_engine() | ||
| iftrt_module: |
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 feel like this is a pretty round about way to get the last engine
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.
Also the order from items is not deterministic so I dont think this is guaranteed to give you the last module anyway. You might need to work back from outputs.
| settings.use_fast_partitioner=True | ||
| dryrun_stats_display(dryrun_tracker,settings.dryrun) | ||
| ifnotsettings.dryrun: |
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.
Why is this conditioned on dry run? Dry run should reflect the deployment graph
| cmd.append("--compilation_mode=dbg") | ||
| else: | ||
| cmd.append("--compilation_mode=opt") | ||
| #if develop: |
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.
Remove
| @torch.library.custom_op("tensorrt::enter_compute_stream",mutates_args=()) | ||
| defenter_compute_stream()->None: |
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.
IMO these should likely be dependent on the input tensors and pull their stream information from there. You arent necessarily guaranteed for the input tensors to be on the default stream. It also means you need storage for the streams.
Also how does this work in c++?
| partitioned_module:torch.fx.GraphModule, | ||
| )->torch.fx.GraphModule: | ||
| fornodeinpartitioned_module.graph.nodes: | ||
| ifnode.op=="placeholder": |
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.
what if there are multiple placeholders or outputs?
4615705 toc611e5fComparec611e5f tob72a538Compare7022b29 tod36494aCompared36494a to6446085Compare
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: