Skip to content

Record tensor factory functions in trace#10935

Closed
zdevito wants to merge 9 commits intopytorch:masterfrom
zdevito:pr/trace_factories
Closed

Record tensor factory functions in trace#10935
zdevito wants to merge 9 commits intopytorch:masterfrom
zdevito:pr/trace_factories

Conversation

@zdevito
Copy link
Contributor

@zdevito zdevito commented Aug 28, 2018

Things like torch.zeros now appear in traces rather than constants.

To continue to support our current level of ONNX export, we run
constant prop to turn these back into constants where possible before
export.

@zdevito zdevito added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 28, 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 force-pushed the pr/trace_factories branch from da44256 to ba1a22a Compare August 28, 2018 06:03
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
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
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.

Mostly LGTM, but we should make sure we don't embed large short-lived tensors as constants into our traces.

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.

This comment was marked as off-topic.

This comment was marked as off-topic.

@zdevito zdevito force-pushed the pr/trace_factories branch from be1d4fc to 3bbb929 Compare August 28, 2018 18:50
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
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 force-pushed the pr/trace_factories branch from 0500c9a to 41cd61c Compare August 29, 2018 17:11
Things like torch.zeros now appear in traces rather than constants.

To continue to support our current level of ONNX export, we run
constant prop to turn these back into constants where possible before
export.
Constant prop needs to work on prim::ListConstruct even when the list
has 0 elements.
@zdevito zdevito force-pushed the pr/trace_factories branch from 41cd61c to e84aac2 Compare August 29, 2018 18:38
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.

make sure constant prop doesnt try to execute them
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 LGTM

@zdevito
Copy link
Contributor Author

zdevito commented Aug 29, 2018

@pytorchbot retest this please

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
Summary:
Things like torch.zeros now appear in traces rather than constants.

To continue to support our current level of ONNX export, we run
constant prop to turn these back into constants where possible before
export.
Pull Request resolved: pytorch#10935

Differential Revision: D9527427

Pulled By: zdevito

fbshipit-source-id: 552a8bcc01b911251dab7d7026faafdd7a3c758a
@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.

5 participants