Skip to content

Tuples/Lists can now be inputs/outputs to script and other simple fixes.#10812

Closed
zdevito wants to merge 7 commits intopytorch:masterfrom
zdevito:pr/jit_fixes
Closed

Tuples/Lists can now be inputs/outputs to script and other simple fixes.#10812
zdevito wants to merge 7 commits intopytorch:masterfrom
zdevito:pr/jit_fixes

Conversation

@zdevito
Copy link
Contributor

@zdevito zdevito commented Aug 23, 2018

  • Fix the necessary pathways so that tuples and lists can be inputs to the script.

  • prevent linear algebra functions from being run in shape prop because
    they frequently will error out for nonsense data.

  • favor schema-driven python input conversion where possible.
    remaining cases where we directly create Stacks without schema are
    only for debugging

  • Make the error messages when calling script/trace functions more pythonic

  • Simplify FlattenTuples -- now that tuples are supported we can choose to only flatten tuples when needed. This may have to be revisited pending onnx test results, but is necessary for making tuple io work.

@zdevito zdevito added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 23, 2018
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.

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

@zdevito zdevito changed the title Minor JIT Fixes Tuples/Lists can now be inputs/outputs to script and other simple fixes. Aug 24, 2018
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

LGTM, but there are a few suspicious things.

test/test_jit.py Outdated

This comment was marked as off-topic.

test/test_jit.py Outdated

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

test/test_jit.py Outdated

This comment was marked as off-topic.

This comment was marked as off-topic.

prevent linear algebra functions from being run in shape prop because
they frequently will error out for nonsense data.

favor schema-driven python input conversion where possible.
remaining cases where we directly create Stacks without schema are
only for debugging
* Since tuples are not supported we only lower TupleUnpack(TupleConstruct())
pairs. Other lowers are not always a performance win and are no longer necessary
for correctness.

* Clean up in the compiler removes a lot of unneeded isTensor checks
and other restrictions
* improve tests
* restore full tuple removal for onnx
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.

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

Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

ONNX changes look functionally equivalent so LGTM

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
…es. (pytorch#10812)

Summary:
* Fix the necessary pathways so that tuples and lists can be inputs to the script.

* prevent linear algebra functions from being run in shape prop because
they frequently will error out for nonsense data.

* favor schema-driven python input conversion where possible.
remaining cases where we directly create Stacks without schema are
only for debugging

* Make the error messages when calling script/trace functions more pythonic

* Simplify FlattenTuples -- now that tuples are supported we can choose to only flatten tuples when needed. This may have to be revisited pending onnx test results, but is necessary for making tuple io work.
Pull Request resolved: pytorch#10812

Differential Revision: D9477982

Pulled By: zdevito

fbshipit-source-id: ed06fc426e6ef6deb404602a26c435a7fc40ea0c
@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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants