-
Notifications
You must be signed in to change notification settings - Fork 370
Cpu memory graph break #3886
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?
Cpu memory graph break #3886
Conversation
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-11-04 20:05:23.825034+00:00
+++ /home/runner/work/TensorRT/TensorRT/py/torch_tensorrt/dynamo/_compiler.py 2025-11-04 20:05:55.253944+00:00
@@ -876,15 +876,14 @@
# This is done to release CPU memory.
for attr in dir(gm):
if attr.startswith("_frozen_param"):
delattr(gm, attr)
-
-
from torch_tensorrt.dynamo.conversion._ConverterRegistry import DYNAMO_CONVERTERS
+
DYNAMO_CONVERTERS.disallowed_targets = set()
-
+
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):
continue
narendasan
left a comment
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.
Do you have a test case or something to demonstrate this feature?
py/torch_tensorrt/dynamo/partitioning/_adjacency_partitioner.py
Outdated
Show resolved
Hide resolved
py/torch_tensorrt/dynamo/partitioning/_adjacency_partitioner.py
Outdated
Show resolved
Hide resolved
py/torch_tensorrt/dynamo/partitioning/_adjacency_partitioner.py
Outdated
Show resolved
Hide resolved
7f0e504 to
18ccadf
Compare
|
|
Improve usability by automating nn.Module -> atomic fx graph |
18ccadf to
f03ab2c
Compare
py/torch_tensorrt/dynamo/partitioning/_adjacency_partitioner.py
Outdated
Show resolved
Hide resolved
c2c7fce to
6f580b6
Compare
6f580b6 to
b9fe0c1
Compare
e7cad5b to
99581e2
Compare
narendasan
left a comment
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.
Its looking good, just add a quick example in the examples folder and list it under contributor documentation for now
3b02ef9 to
290cc39
Compare
aba69ef to
756f827
Compare
cb8480d to
2d6053e
Compare
| subgraphs = [Subgraph(is_acc=True, nodes=nodes)] | ||
| self.fusion_patterns = get_node_in_fusion_pattern(self.module.graph) | ||
|
|
||
| assert self.check_topological_order( |
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.
When will this fail? Do we need to check this or can FX just guarantee this for us?
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.
This should never fail. I put this here just in case something changes in torch and we don't know
| require_full_compilation=settings.require_full_compilation, | ||
| ) | ||
|
|
||
| partitioned_module = resource_partition( |
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.
Shoudnt this conditionally run based on if the user provided a budget at all?
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.
/ should we make this opt in while testing?
| == 2 | ||
| ), "The graph should have 2 non-accelerated subgraphs" | ||
|
|
||
| def test_resource_partitioning_with_global_capability_partitioning(self): |
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.
We should also have a test with no fall back as well as a test which tests for making sure that the atomic subgraph system works
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 one that tests registering a new atomic subgraph and then verifying that it has the desired effect
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 one that tests registering a new atomic subgraph and then verifying that it has the desired effect
Can this prove the atomic subgraph system works?
a061180 to
a67d4a6
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: