-
Notifications
You must be signed in to change notification settings - Fork 22.2k
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
keep output type after calling SubgraphRewriter #65453
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit d53c4be (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 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 to the (internal) Dr. CI Users group. |
2d5f91e
to
80b9ebc
Compare
Codecov Report
@@ Coverage Diff @@
## master #65453 +/- ##
==========================================
- Coverage 66.38% 66.37% -0.01%
==========================================
Files 739 739
Lines 94295 94299 +4
==========================================
- Hits 62594 62592 -2
- Misses 31701 31707 +6 |
That change looks reasonable, thank you for making it! Do you mind adding a test for this scenario as well? You can put it here: https://github.com/pytorch/pytorch/blob/master/test/cpp/jit/test_subgraph_rewriter.cpp |
@ZolotukhinM, do you know how to check a graph node has shape info? I don't find a test case to check it. |
We can access type info from the pytorch/torch/csrc/jit/tensorexpr/kernel.cpp Lines 201 to 220 in fccaa4a
Alternatively, and it can be a better way for testing I think, we could simply print the IR after the rewrite and see if the new IR has the shape info. We could then use |
3601cf7
to
d53c4be
Compare
@ZolotukhinM, one test case is added. |
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.
Awesome, thanks! Do you need help with merging the PR?
@ZolotukhinM, yes, please help merge it, thanks! |
@ZolotukhinM has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@ZolotukhinM merged this pull request in 1682722. |
@malfet I see we marked this for inclusion in the |
@seemethere this PR adds a test for the regression |
For jit SubgraphRewriter, it doesn't keep output type after overwriting the old graph, for example, in profiling mode, the old graph has the old operator's shapes, but after replacing the old operator with a newer operator by applying SubgraphRewriter, the tensor shape info was eliminated.
The activation is that I want to replace pytorch convolution with a customer's convolution, I first register aten::_convolution as a profiler node that can reorder the input and output's shapes, and then using graph rewrite to replace it as aten::conv2d, which tensors' shapes info are eliminated. I hope using input size do some pre-progress before replacing aten::conv2d with the customer's convolution.
Before rewrite:
after rewrite by using aten::conv2d
expected result after replace aten::_convolution with aten::conv2d: