Make string serialization of C++ FunctionSchema consistent with torchgen.model.FunctionSchema by shunting314 · Pull Request #77926 · pytorch/pytorch

shunting314 · 2022-05-20T01:26:03Z

Stack from ghstack (oldest at bottom):

-> Make string serialization of C++ FunctionSchema consistent with torchgen.model.FunctionSchema #77926

There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema.
The latter will not add parenthesis around the returned types if that a single item,
but the C++ FunctionSchema always add the parenthesis.

Make them consistent so we can convert one type to the other via its string representation and parse method.

Differential Revision: D36535924

…gen.model.FunctionSchema There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

facebook-github-bot · 2022-05-20T01:26:07Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/77926
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit b7ef875 (more details on the Dr. CI page):

Expand to see more

💚 💚 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.

Click here to manually regenerate this comment.

…gen.model.FunctionSchema There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) ghstack-source-id: 156868252 Pull Request resolved: #77926

bdhirsh · 2022-05-20T13:16:57Z

aten/src/ATen/core/function_schema_inl.h


  const auto& returns = schema.returns();
-  out << "(";
+  bool need_paren = returns.size() != 1 && !schema.is_varret();


What does the is_varret() check do? (Can you paste a before/after?)

I actually don't know. From the name and the string representation (...), I think it means variable number of return values. Also I don't see any occurrence of ... in native_functions.yaml. The Return.parse method in torchgen also does not handle ..., so I think FunctionSchema::is_varret is something specific to the C++ FunctionSchema.

Not sure what does pasting 'a before/after' means here since I don't change FunctionSchema::is_varret itself. But now the behavior is, if there is only a single return value and FunctionSchema.is_varret flag is not on, we skip the parenthesis around the return values. This will be consistent with torchgen function schema parser.

I only find one example of using ... in return values here for prim ops: https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/runtime/register_prim_ops.cpp#L192 .

oh thanks, that seems reasonable (not putting parens around ..., and also the fact that none of our native_functions.yaml ops care about it)

bdhirsh

lgtm!

shunting314 · 2022-05-20T18:53:49Z

CI failure are not related but I'll rebase

… with torchgen.model.FunctionSchema" There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

…gen.model.FunctionSchema Pull Request resolved: #77926 There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. ghstack-source-id: 157089530 Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/)

… with torchgen.model.FunctionSchema" There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

shunting314 · 2022-05-24T00:12:28Z

Fix unit test failure by updating .expect files to reflect the change.

… with torchgen.model.FunctionSchema" There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

…gen.model.FunctionSchema Pull Request resolved: #77926 There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. ghstack-source-id: 157127467 Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/)

… with torchgen.model.FunctionSchema" There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

…gen.model.FunctionSchema Pull Request resolved: #77926 There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. ghstack-source-id: 157147958 Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/)

… with torchgen.model.FunctionSchema" There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/) [ghstack-poisoned]

…gen.model.FunctionSchema Pull Request resolved: #77926 There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. ghstack-source-id: 157148340 Differential Revision: [D36535924](https://our.internmc.facebook.com/intern/diff/D36535924/)

shunting314 · 2022-05-24T18:23:34Z

@bdhirsh I changed the code a bit to make existing unit tests happy. Do you want to talk another look?

Now we skip parenthesis around the return type if

the return type is a single item and not varrret: e.g. -> Tensor
or the return type is nothing but varret. e.g. -> ...

According to the above rule some thing like -> (Tensor, ...) will still be enclosed with a pair of parenthesis.

There is one exception. If the return type is a single item but starts with left parenthesis, we still need add the extra prenthesis.

There are 2 cases

something like 'aten::items.str(Dict(str, t) self) -> ((str, t)[])'. Without the extra parenthesis, the c++ schema parser will mistakenly treat '(str,t)[]' as returning a tuple since the lexer at most look ahead one token. That cause schema parsing fail.
something like '-> ((str, str))'. Need extra parenthesis so the return type is a single tuple rather than two strings. PR (Support variadic returns in Schema's operator<< #23204) has more context about this. test_serialize_and_deserialize (https://github.com/pytorch/pytorch/blob/master/test/test_function_schema.py#L15) also covers this case.

shunting314 · 2022-05-24T19:34:10Z

@pytorchbot land this please

shunting314 · 2022-05-24T19:36:53Z

@pytorchbot merge this please

github-actions · 2022-05-24T20:05:04Z

Hey @shunting314.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

shunting314 · 2022-05-24T20:46:31Z

cc @eellison for the change in case it affects TorchScript.

…gen.model.FunctionSchema (#77926) Summary: Pull Request resolved: #77926 There is a discrepency between the string representation of C++ FunctionSchema and torchgen.model.FunctionSchema. The latter will not add parenthesis around the returned types if that a single item, but the C++ FunctionSchema always add the parenthesis. Make them consistent so we can convert one type to the other via its string representation and parse method. Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/26d9386f6751b7c7b1cbc2861db9fcd99f64b8fa Reviewed By: bdhirsh Differential Revision: D36535924 Pulled By: shunting314 fbshipit-source-id: 1711a366ebcf44179a17a16fc5140458a1bd2780

facebook-github-bot added the cla signed label May 20, 2022

bdhirsh reviewed May 20, 2022

View reviewed changes

shunting314 requested a review from bdhirsh May 20, 2022 18:18

bdhirsh approved these changes May 20, 2022

View reviewed changes

pytorchmergebot added the Merged label May 24, 2022

pytorchmergebot closed this in 26d9386 May 24, 2022

facebook-github-bot deleted the gh/shunting314/20/head branch May 28, 2022 14:17

Conversation

shunting314 commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

bdhirsh May 20, 2022

Choose a reason for hiding this comment

Uh oh!

shunting314 May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shunting314 May 20, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh May 20, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh left a comment

Choose a reason for hiding this comment

Uh oh!

shunting314 commented May 20, 2022

Uh oh!

shunting314 commented May 24, 2022

Uh oh!

shunting314 commented May 24, 2022

Uh oh!

shunting314 commented May 24, 2022

Uh oh!

shunting314 commented May 24, 2022

Uh oh!

github-actions bot commented May 24, 2022

Uh oh!

shunting314 commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shunting314 commented May 20, 2022 •

edited

Loading

facebook-github-bot commented May 20, 2022 •

edited

Loading

shunting314 May 20, 2022 •

edited

Loading