[ONNX] Update 'Functionalize' pass to support pre-decomp graph; Drop 'aten_graph' arg for 'DynamoExporter' #99667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
pytorch-bot
bot
added
release notes: onnx
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99667
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit 3fc55a6: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
BowenBao
added a commit
that referenced
this pull request
Apr 20, 2023
ghstack-source-id: b15d9612edec3ebd2917ad381e5c52de79416d1b Pull Request resolved: #99667
Summary - Previously this was required by `tracing_mode=symbolic` for `dynamic` tracing. That argument will be dropped by #99555. - Later decomposition pass will do graph lowering, so this step is duplicated. - Functionalization currently cannot work properly on aten level graph. So it must happen before lowering & decompositions. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 21, 2023
ghstack-source-id: 6d04ee3f2ee259a6c51cd527c290a357096846f2 Pull Request resolved: #99667
Summary - Previously this was required by `tracing_mode=symbolic` for `dynamic` tracing. That argument will be dropped by #99555. - Later decomposition pass will do graph lowering, so this step is duplicated. - Functionalization currently cannot work properly on aten level graph. So it must happen before lowering & decompositions. - Introduce `ReplaceInplacePostFunctionalization` pass to replace inplace variant ops with outplace version. These ops are created by aten graph lowering and decomposition post functionalization. They won't be doing any real mutation as it is expected to have been handled by functionalization. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 21, 2023
ghstack-source-id: aa29f45725b6c3f4a951223fa6fd56e06f36b982 Pull Request resolved: #99667
titaiwangms
approved these changes
Apr 21, 2023
Summary - Previously this was required by `tracing_mode=symbolic` for `dynamic` tracing. That argument will be dropped by #99555. - Later decomposition pass will do graph lowering, so this step is duplicated. - Functionalization currently cannot work properly on aten level graph. So it must happen before lowering & decompositions. - Introduce `ReplaceInplacePostFunctionalization` pass to replace inplace variant ops with outplace version. These ops are created by aten graph lowering and decomposition post functionalization. They won't be doing any real mutation as it is expected to have been handled by functionalization. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 21, 2023
ghstack-source-id: e25861a8ec73d8f4010237dc95205fddf0faebee Pull Request resolved: #99667
BowenBao
added
module: onnx
…] Drop 'aten_graph' arg for 'DynamoExporter'" Summary - Previously this was required by `tracing_mode=symbolic` for `dynamic` tracing. That argument will be dropped by #99555. - Later decomposition pass will do graph lowering, so this step is duplicated. - Functionalization currently cannot work properly on aten level graph. So it must happen before lowering & decompositions. - Introduce `ReplaceInplacePostFunctionalization` pass to replace inplace variant ops with outplace version. These ops are created by aten graph lowering and decomposition post functionalization. They won't be doing any real mutation as it is expected to have been handled by functionalization. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: 077fd3a1d13582ceb7a135f7b07ece93ff0b159d Pull Request resolved: #99667
…h' arg for 'DynamoExporter'" Summary - Previously this was required by `tracing_mode=symbolic` for `dynamic` tracing. That argument will be dropped by #99555. - Later decomposition pass will do graph lowering, so this step is duplicated. - Functionalization currently cannot work properly on aten level graph. So it must happen before lowering & decompositions. - Introduce `ReplaceInplacePostFunctionalization` pass to replace inplace variant ops with outplace version. These ops are created by aten graph lowering and decomposition post functionalization. They won't be doing any real mutation as it is expected to have been handled by functionalization. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 28, 2023
ghstack-source-id: b2ef6bc7f45661f8b0098ea405496a3dc9584e8c Pull Request resolved: #99667
BowenBao
changed the title
…orkaround in 'Functionalize' pass" Summary - Previously this was required by and entangled with `tracing_mode=symbolic` for `dynamic` tracing. That is resolved by #99555 and its follow ups. - Later decomposition pass will do graph lowering, so this step is duplicated. - Updated `Functionalization` to workaround #99774 (comment) Todo - Training vs eval in dynamo_export So we are effectively exporting all models in traning mode by default. But for the sake of this export we are only interested in eval mode. The question is, should we call `model.eval()` in `dynamo_export`? Tests with model containing batch norm fails 'functionalization' in training mode. We are explicitly calling `model.eval()` for these model for now. - Merge decomp and functionalize pass. Both calls into `make_fx`. Merging potentially increases performance. However it is unclear if it will result in different behavior. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
Apr 29, 2023
ghstack-source-id: 78e3950a12d32e764987857a43faf3935004ae7f Pull Request resolved: #99667
…orkaround in 'Functionalize' pass" Summary - Previously this was required by and entangled with `tracing_mode=symbolic` for `dynamic` tracing. That is resolved by #99555 and its follow ups. - Later decomposition pass will do graph lowering, so this step is duplicated. - Updated `Functionalization` to workaround #99774 (comment) Todo - Training vs eval in dynamo_export So we are effectively exporting all models in traning mode by default. But for the sake of this export we are only interested in eval mode. The question is, should we call `model.eval()` in `dynamo_export`? Tests with model containing batch norm fails 'functionalization' in training mode. We are explicitly calling `model.eval()` for these model for now. - Merge decomp and functionalize pass. Both calls into `make_fx`. Merging potentially increases performance. However it is unclear if it will result in different behavior. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
May 1, 2023
ghstack-source-id: 594171b60ed8a3f48090b90a24d24b9ccba8a1a3 Pull Request resolved: #99667
…orkaround in 'Functionalize' pass" Summary - Previously this was required by and entangled with `tracing_mode=symbolic` for `dynamic` tracing. That is resolved by #99555 and its follow ups. - Later decomposition pass will do graph lowering, so this step is duplicated. - Updated `Functionalization` to workaround #99774 (comment) Todo - Training vs eval in dynamo_export So we are effectively exporting all models in traning mode by default. But for the sake of this export we are only interested in eval mode. The question is, should we call `model.eval()` in `dynamo_export`? Tests with model containing batch norm fails 'functionalization' in training mode. We are explicitly calling `model.eval()` for these model for now. - Merge decomp and functionalize pass. Both calls into `make_fx`. Merging potentially increases performance. However it is unclear if it will result in different behavior. Workaround to unblock #99662. [ghstack-poisoned]
BowenBao
added a commit
that referenced
this pull request
May 3, 2023
ghstack-source-id: 278c37b40d0995e8b9ebf3d3cbe26dd015552c2c Pull Request resolved: #99667
|
Seems getting complicated. You might consider move Functionalization ahead of dropping aten_graph in the title. |
BowenBao
changed the title
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
jon-chuang
reviewed
Oct 12, 2023
| for inpt, input_functional in zip(flat_inputs, flat_inputs_functional): | ||
| if isinstance(input_functional, torch.Tensor): | ||
| torch._sync(input_functional) | ||
| inpt_new = torch._from_functional_tensor(input_functional) |
There was a problem hiding this comment.
Why is this inpt_new assigned?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Summary
tracing_mode=symbolicfordynamictracing.That is resolved by Delete tracing_mode argument to export #99555 and its follow ups.
Functionalizationto workaround RuntimeError: Cannot call sizes() on tensor with symbolic sizes/strides w/dynamo.export,make_fxandfunctionalize#99774 (comment)Todo
So we are effectively exporting all models in traning mode by
default. But for the sake of this export we are only interested in eval mode.
The question is, should we call
model.eval()indynamo_export?Tests with model containing batch norm fails 'functionalization' in training mode.
We are explicitly calling
model.eval()for these model for now.make_fx.Merging potentially increases performance. However it is unclear
if it will result in different behavior.
Fixes #99662. (For the functionalization issue. Still need missing op support.)