-
Notifications
You must be signed in to change notification settings - Fork 376
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 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?
Graph break overhead #3946
Conversation
8c9284a to
46829e6
Compare
7541aca to
10662c0
Compare
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.
There are some changes that do not conform to Python style guidelines:
--- /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/_compiler.py 2025-12-12 00:21:37.519598+00:00
+++ /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/_compiler.py 2025-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 to
4615705
Compare
core/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?
| for attr in dir(gm): | ||
| if attr.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?
4615705 to
c611e5f
Compare
c611e5f to
b72a538
Compare
7022b29 to
d36494a
Compare
dd33ac3 to
aa645fd
Compare
aa645fd to
8a08bd5
Compare
8a08bd5 to
61f33b1
Compare
| setup_input_tensors(inputs, compiled_engine, cudagraphs_enabled, shape_changed); | ||
| // Check if input shapes can be inferred. | ||
| int32_t const io_size{compiled_engine->cuda_engine->getNbIOTensors()}; | ||
| int32_t const io_size{compiled_engine->io_size}; |
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.
Are things like this actually faster? Like have we looked at the implementation to see its not cached at the TRT level?
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.
Yes they do have small difference. I am trying to minize repeating code and this repeatly call this outbound function in every iteration
core/runtime/TRTEngine.h
Outdated
| std::string shape_key = "None"; | ||
| bool use_pre_allocated_outputs = false; | ||
| std::vector<at::Tensor> pre_allocated_outputs; | ||
| std::vector<at::Tensor> allocated_outputs; |
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 there a way to reuse pre_allocated_outputs?
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.
@cehongwang we need to figure out how to merge these two implementations, we cant just keep adding new code paths for every situation. Either the output allocator needs to change to leverage the new system or the new system needs to leverage what the output allocator uses
f393b43 to
a87a05c
Compare
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.
It seems like you are just renaming variables here? Why do we need to make that change?
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 pre_allocated_outputs -> allocated_outputs change
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 also changed self.output_tensors = self.create_output_tensors() to self.allocated_outputs = self.create_output_tensors(). Basically merged two variable together. Isn't that what you wanted me to do?
| ): | ||
| if can_use_pre_allocated_outputs: | ||
| outputs = self.pre_allocated_outputs | ||
| outputs = self.allocated_outputs |
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.
e.g. ^
a87a05c to
d1e786e
Compare
d1e786e to
154c621
Compare
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: