[JIT] Add comments for adding shape function and linting #73570
Conversation
[ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit f049367 (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).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
facebook-github-bot
added
the
oncall: jit
eellison
changed the title
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
cc silvasean [ghstack-poisoned]
| void checkInputReturnedAsOutput( | ||
| const FunctionSchema* schema, | ||
| const std::shared_ptr<Graph>& graph) { | ||
| // Could use alias db here as well but would have to warn because it's |
There was a problem hiding this comment.
Nice. I think a few of my handwritten shape functions are breaking this invariant! Will try to upstream them and get this great linting :)
There was a problem hiding this comment.
The main thing I'm trying to preserve is that you can compose shape functions together... Technically it's fine if an aliased list is return so long as no shape function mutates its' input, however I thought it would be a little more defensive/safe to return a new unaliased list..
There was a problem hiding this comment.
Ah, I get it. I actually have a shape function (topk) that mutates its input (it was easier to write that way, but could easily insert a copy if needed).
| in `test_symbolic_shape_analysis.py` such as `test_adaptive_avg_pool2d`. | ||
|
|
||
| Operators which take in a list of tensors, such as concat, are not yet | ||
| supported. Concat has been special cased and could be generalized as needed. |
There was a problem hiding this comment.
I am adding support for aten::stack which takes in a list of tensors. Added you as the reviewer for PRs in that stream.
| toGraphFunction(shape_compute_function).graph(); | ||
|
|
||
| transformShapeFunction(schema_string, graph); | ||
| checkShapeFunction(schema_string, graph); |
There was a problem hiding this comment.
Are all shape compute graphs hard coded? If so, can we move the checks to unit tests instead of doing it every time we load the module? Or perhaps I understand it incorrectly.
cc silvasean add lints that the types of shape functions are as expected, and for few other patterns [ghstack-poisoned]
cc silvasean add lints that the types of shape functions are as expected, and for few other patterns [ghstack-poisoned]
cc silvasean add lints that the types of shape functions are as expected, and for few other patterns [ghstack-poisoned]
|
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@pytorchbot merge this please |
|
Merge failed due to PR 73570 does not match merge rules |
| @@ -498,3 +499,37 @@ def test_shape_function_includes(self): | |||
| m2_shape = [20, 10] | |||
| res = torch.jit._shapes.matmul(m1_shape, m2_shape) | |||
| self.assertEqual(res, [10, 10]) | |||
|
|
|||
| def test_register_function_error_checking(self): | |||
There was a problem hiding this comment.
Nice python exposure. We actually get to patch it hot on our scripts!
|
@pytorchmergebot merge this please |
|
Merge failed due to Command Raised by https://github.com/pytorch/pytorch/actions/runs/2060916491 |
cc silvasean add lints that the types of shape functions are as expected, and for few other patterns Differential Revision: [D35192688](https://our.internmc.facebook.com/intern/diff/D35192688) [ghstack-poisoned]
|
@pytorchmergebot merge this please |
|
Hey @eellison. |
Summary: Pull Request resolved: #73570 Approved by: https://github.com/huiguoo Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/6d36bbde7eb2eb0aed448f694338cb49c2ae47f3 Reviewed By: pbelevich Differential Revision: D35192688 Pulled By: atalman fbshipit-source-id: b12b80e6a6dd1adaa57a8facb6bb077989faa543
Summary: Pull Request resolved: #73570 Approved by: https://github.com/huiguoo Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/6d36bbde7eb2eb0aed448f694338cb49c2ae47f3 Reviewed By: pbelevich Differential Revision: D35192688 Pulled By: atalman fbshipit-source-id: b12b80e6a6dd1adaa57a8facb6bb077989faa543 (cherry picked from commit e50478c)
Stack from ghstack:
cc @silvasean
add lints that the types of shape functions are as expected, and for few other patterns
Differential Revision: D35192688