-
Notifications
You must be signed in to change notification settings - Fork 685
Add create_mutable_buffer util #13813
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13813
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit c6bb918 with merge base 6c12956 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
dbbd892
to
311eba6
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.
Add backends/transforms in https://github.com/pytorch/executorch/blob/main/pytest.ini
backends/transforms/utils.py
Outdated
|
||
if not user_input_indices_or_consts: | ||
# If no user inputs or constants exist, insert at the end of all existing inputs | ||
node_index = len(exp_program.graph_signature.input_specs) |
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.
unused in this if block
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 is used outside the block
) | ||
return executorch_program_manager | ||
|
||
def _test_eager(self, model, example_inputs, num_lifted_args=1): |
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.
dont follow what this is doing for eager?
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.
eager
might be a bad name, but the idea is to compare the ExecuTorch graph from two different origins for a mutable buffer (1) written in torch.nn.Module by the end user as in eager
mode, and (2) inserted via this util + pass, and both ET graphs should be indistinguishable in every way.
|
||
self.compare(et_1, et_2, example_inputs) | ||
|
||
def test_basic_with_param(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.
are you testing when you dont have any inputs in the graph? there is a path for that in your pass.
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.
Not sure I follow. x
is the normal input, and I am adding a param (beyond the test_basic
module) to test if the signature update is correct.
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.
looks pretty good. left some comments
backends/transforms/utils.py
Outdated
) | ||
|
||
# Find indices of user inputs and constant tensors | ||
user_input_indices_or_consts = [ |
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 a loop would be a cleaner way of writing this through line 324.
Since you have validated the Spec.kind and the nodes earlier you could just loop over the input_specs and index into graph.nodes
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.
refactored but still not as clean as I like.
@angelayi @avikchaudhuri @tugsbayasgalan I think in general it would be a good value add from compiler if these sorts of primitive graph operations lived in core. Things like node.is_mutable_buffer(), ep.add_buffer() ep.add_mutable_buffer() etc |
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.
.
332edae
to
918b70b
Compare
@digantdesai see my previous comment #13813 (review) |
separate PR? |
if modified: | ||
graph_module = super().call(graph_module).graph_module | ||
graph_module.graph.lint() | ||
graph_module.graph.eliminate_dead_code() |
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 fn is technically unsafe to call if the graph is not functional.
|
||
example_node = nodes[0] | ||
if isinstance( | ||
example_node.meta["val"], (tuple, torch.fx.immutable_collections.immutable_list) |
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.
seems like a bit of a strange check. Is this robust?
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.
Hmm just refactored in this PR.. but what else it can be when number of tensors > 1?
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.
Thanks @JacobSzwejbka
|
||
example_node = nodes[0] | ||
if isinstance( | ||
example_node.meta["val"], (tuple, torch.fx.immutable_collections.immutable_list) |
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.
Hmm just refactored in this PR.. but what else it can be when number of tensors > 1?
Rationale: * We want to support stateful custom ops inserted after to_edge in a non delegated cases like op-lib-backends i.e. cortex-m::cmsis_nn::linear Details: * Allows a pass to add a mutable buffer. * Essentially redoes what export, to_edge does for a buffer registered in an nn.module. * Also plays nice with to_executorch passes like conversion to in-place, and write-back for mutated buffers. * Verifies above with tests, adds a HelperPass for someone looking to leverage this util as an example too. To Test: $ python -m unittest backends.transforms.test.test_create_mutable_buffer
7f74232
to
c6bb918
Compare
Rationale:
Details:
To Test: