-
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
core/runtime/execute_engine.cpp
Outdated
| at::Tensor contig_input = inputs[i].view(shape).contiguous(); | ||
| formatted_inputs.emplace_back(std::move(contig_input)); | ||
|
|
||
| bool need_cudagraphs_record = cudagraphs_enabled and |
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.
Just use &&, ||, !
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 or compiled_engine->allocated_outputs.size() == 0) { |
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 the allocated_outputs size test? Just that we havent allocated the outputs yet?
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 we fold some of these conditions ahead of time?
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.
So it's like for the first time you run the model or when the input shape get changed, we need to (re)set the input shape right? I don't think we can fold it
8a08bd5 to
61f33b1
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: