[torch.fx] Pass placeholders through delegate too #43432
Conversation
There was a problem hiding this comment.
@dzhulgakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/fx/symbolic_trace.py
Outdated
There was a problem hiding this comment.
Would be nice to have these commented like the other methods (I see create_arg slipped through as well)
There was a problem hiding this comment.
I don't really know what much to say about them :) also why those are comments and not docstrings?
There was a problem hiding this comment.
You would describe when you would call these methods, the arguments and types, etc. They can be docstrings if you want
💊 CI failures summary and remediationsAs of commit 504d6ad (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 25 times. |
There was a problem hiding this comment.
@dzhulgakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@dzhulgakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@dzhulgakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/fx/graph.py
Outdated
There was a problem hiding this comment.
can we keep these helper functions? passing raw strings for op type doesn't sound great.
or should we always create node through delegate now?
There was a problem hiding this comment.
Well, it's then easy to mix up delegate vs regular creation. Also, why placeholder and get_param but not others like call_module?
There was a problem hiding this comment.
Well, it's then easy to mix up delegate vs regular creation.
so is delegate the suggested API for creating nodes?
Also, why placeholder and get_param but not others like call_module?
I was planning to add helper functions for other node types as well.
There was a problem hiding this comment.
Node creation should always go through a delegate and the delegate should be specifiable by the caller of the pass. Otherwise, we will lose composability, for example if I want to run a pipeline of passes on a Module but I want custom leaf modules
I might go so far as to say we should make create_node private, or move it into Delegate itself to make Graph just a dumb container for nodes
There was a problem hiding this comment.
always enforcing people to use delegate sounds good, are you planning to do it? I still have some prs on quantization, feel free to change all the other call sites of create_node first and I can have a PR to make create_node private and change quantization call sites.
Summary: It's useful if we add additional attributed to nodes in the graph - it's easier to set the attribute on all nodes, even if the value would happen to be None. Pull Request resolved: pytorch#43432 Reviewed By: jamesr66a Differential Revision: D23276433 Pulled By: dzhulgakov fbshipit-source-id: 11b3f452e7fe859c5d6c861ff42cd326dd3b66c0
|
This pull request was exported from Phabricator. Differential Revision: D23276433 |
Codecov Report
@@ Coverage Diff @@
## master #43432 +/- ##
===========================================
- Coverage 69.34% 50.32% -19.02%
===========================================
Files 378 378
Lines 46700 46700
===========================================
- Hits 32382 23502 -8880
- Misses 14318 23198 +8880
Continue to review full report at Codecov.
|
|
@dzhulgakov merged this pull request in 633d239. |
5 participants
It's useful if we add additional attributed to nodes in the graph - it's easier to set the attribute on all nodes, even if the value would happen to be None.