Skip to content

[JIT] Unified type annotation parsing for script frontends#10279

Closed
jamesr66a wants to merge 29 commits intopytorch:masterfrom
jamesr66a:parser_type_annotation2
Closed

[JIT] Unified type annotation parsing for script frontends#10279
jamesr66a wants to merge 29 commits intopytorch:masterfrom
jamesr66a:parser_type_annotation2

Conversation

@jamesr66a
Copy link
Collaborator

@jamesr66a jamesr66a commented Aug 6, 2018 •
edited
Loading

After this, all combinations of {String frontend, Python AST Frontend}{Python 3-style type annotations, MyPy-style type comments}{Script method, Script function} should properly accept type annotations.

Possible TODOs:

  • Clean up the functions marked HACK
  • Clean up the Subscript tree-view to better match the Python AST versions
  • Can we use this for Python functions? That's the only place annotations.get_signature() is still needed

@jamesr66a jamesr66a force-pushed the parser_type_annotation2 branch 4 times, most recently from ff0fd05 to e579efd Compare August 10, 2018 17:54
zdevito
zdevito reviewed Aug 10, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. I have some comments inline for simplifications.

torch/csrc/jit/function_schema.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/parser.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/jit/annotations.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/jit/frontend.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/init.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/parser.h Outdated

This comment was marked as off-topic.

Sign in to view

@jamesr66a jamesr66a force-pushed the parser_type_annotation2 branch from e579efd to 994a341 Compare August 13, 2018 15:45
@zou3519 zou3519 added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 14, 2018
@jamesr66a jamesr66a force-pushed the parser_type_annotation2 branch from ce90046 to 26318ac Compare August 14, 2018 18:28
facebook-github-bot
facebook-github-bot reviewed Aug 14, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito
zdevito approved these changes Aug 15, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Unified type annotation parsing for script frontends (pytorch#10279)
f4c8b3d 
Summary:
After this, all combinations of {String frontend, Python AST Frontend}{Python 3-style type annotations, MyPy-style type comments}{Script method, Script function} should properly accept type annotations.

Possible TODOs:
- Clean up the functions marked HACK
- Clean up the Subscript tree-view to better match the Python AST versions
- Can we use this for Python functions? That's the only place annotations.get_signature() is still needed
Pull Request resolved: pytorch#10279

Differential Revision: D9319726

Pulled By: jamesr66a

fbshipit-source-id: b13f7d4f066b0283d4fc1421a1abb9305c3b28fa
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zdevito zdevito zdevito approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

+1 more reviewer

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jamesr66a @zdevito @facebook-github-bot @ezyang @zou3519