[export] simplify automatic dynamic shapes processing #136591

pianpwk · 2024-09-25T00:40:16Z

Removing _transform_shapes_for_default_dynamic and assume_static_by_default=False as added in #133620.

This reverts back to assume_static_by_default=True with the use of dynamo decorators (e.g. maybe_mark_dynamic, mark_static, instead) for handling Dim.AUTO & Dim.STATIC instead. This is easier to maintain, as it doesn't requiring reasoning about "inverting" the dynamic_shapes specs, and also opens up usage of other decorators (mark_dynamic, mark_unbacked).

On the user side this change has no effect, but internally this means dynamic behavior is determined only by the dynamic_shapes specs (ignoring user-side input decorators following #135536), but transferring this information for _DimHints via decorators, for Dynamo/non-strict to create symbolic_contexts accordingly, e.g.

pytorch/torch/_dynamo/variables/builder.py

Lines 2646 to 2666 in 7c6d543

    
           if marked_unbacked: 
        
               dynamic_size = DimDynamic.SIZE_LIKE_UNBACKED 
        
           elif ( 
        
               constraint_size is not None 
        
               or marked_dynamic 
        
               or marked_weak_dynamic 
        
               or is_nested_int(e.size()[i]) 
        
           ): 
        
               # NB: We could assert static_shapes is False here, but it 
        
               # seems better to allow the user to override symbolic_context in this 
        
               # case 
        
               dynamic_size = DimDynamic.DYNAMIC 
        
           elif static_shapes or config.assume_static_by_default or marked_static: 
        
               dynamic_size = DimDynamic.STATIC 
        
           else: 
        
               dynamic_size = DimDynamic.DUCK 
        
           if constraint_stride is not None: 
        
               dynamic_stride = DimDynamic.DYNAMIC 
        
           else: 
        
               dynamic_stride = DimDynamic.INFER_STRIDE

One caveat is we don't raise errors for dynamic decorators on the user side, since we don't know if they're from user markings, or from re-exporting with inputs we've previously marked.

Differential Revision: D63358628

pytorch-bot · 2024-09-25T00:40:19Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/136591

📄 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 the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures

As of commit db71c3d with merge base c878ea2 ():

NEW FAILURES - The following jobs have failed:

inductor / cuda12.1-py3.10-gcc9-sm86 / test (aot_inductor_torchbench, 2, 2, lf.linux.g5.4xlarge.nvidia.gpu) (gh)
sam_fast
inductor-periodic / cuda12.1-py3.10-gcc9-sm80 / build (gh)
Lint / lintrunner-noclang / linux-job (gh)
>>> Lint for RELEASE.md:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2024-09-25T00:40:27Z

This pull request was exported from Phabricator. Differential Revision: D63358628

Summary: Pull Request resolved: #136591 Differential Revision: D63358628

facebook-github-bot · 2024-09-25T00:45:30Z

This pull request was exported from Phabricator. Differential Revision: D63358628

facebook-github-bot · 2024-09-25T16:57:04Z

This pull request was exported from Phabricator. Differential Revision: D63358628

avikchaudhuri

Looks fine as long as tests pass, but I'm not entirely sure what the effect of the change is in this PR, could you write a summary please?

What I'd like is the some kind of invariant to hold around STATIC, constant dims, DYNAMIC, AUTO, and None here. Looks like we're not transforming specs any more, but using a marking scheme to get what we want. Who consumes these markings, is it just dynamo, or is it make_fx as well?

avikchaudhuri · 2024-09-25T17:07:36Z

torch/_export/non_strict_utils.py

OK, trying to understand...so you start off with mostly STATIC here.

Yeah, for now everything here can be static except those marked with Dim.AUTO. This doesn't interfere with the old Dim() specs.

avikchaudhuri · 2024-09-25T17:07:51Z

torch/_export/non_strict_utils.py

Where does AUTO come in?

DimHints now get handled in _process_dynamic_shapes

avikchaudhuri · 2024-09-25T17:09:20Z

torch/export/_trace.py

What does this buy?

Nothing functionality wise, we just don't have to maintain this specs inverting which is already pretty confusing to me today

avikchaudhuri · 2024-09-25T17:11:28Z

torch/export/dynamic_shapes.py

Interesting, so we'll only allow specs to dictate which ones are marked? Should we warn if any of these were set, asking to use specs instead?

Ah added a comment in the last PR description line

avikchaudhuri · 2024-09-25T17:12:31Z

torch/export/dynamic_shapes.py

else what? Maybe handle that explicitly with a pass and then assert / give a useful error if anything unexpected happens?

pianpwk · 2024-09-25T17:20:58Z

Looks fine as long as tests pass, but I'm not entirely sure what the effect of the change is in this PR, could you write a summary please?

What I'd like is the some kind of invariant to hold around STATIC, constant dims, DYNAMIC, AUTO, and None here. Looks like we're not transforming specs any more, but using a marking scheme to get what we want. Who consumes these markings, is it just dynamo, or is it make_fx as well?

Ah sorry, just realized the diff description didn't update the PR description too

facebook-github-bot · 2024-09-25T17:38:04Z

@pianpwk has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-09-25T20:43:14Z

This pull request was exported from Phabricator. Differential Revision: D63358628

facebook-github-bot · 2024-09-25T21:34:46Z

This pull request was exported from Phabricator. Differential Revision: D63358628

facebook-github-bot · 2024-09-25T21:41:15Z

This pull request was exported from Phabricator. Differential Revision: D63358628

Summary: Removing `_transform_shapes_for_default_dynamic` and `assume_static_by_default=False` as added in #133620. This reverts back to `assume_static_by_default=True` with the use of dynamo decorators (e.g. `maybe_mark_dynamic, mark_static`, instead) for handling Dim.AUTO & Dim.STATIC instead. This is easier to maintain, as it doesn't requiring reasoning about "inverting" the dynamic_shapes specs, and also opens up usage of other decorators (`mark_dynamic, mark_unbacked`). On the user side this change has no effect, but internally this means dynamic behavior is determined only by the `dynamic_shapes` specs (ignoring user-side input decorators following #135536), but transferring this information for _DimHints via decorators, for Dynamo/non-strict to create symbolic_contexts accordingly, e.g. https://github.com/pytorch/pytorch/blob/7c6d543a5b3d65d8f49420e60cda150faaa5b8a0/torch/_dynamo/variables/builder.py#L2646-L2666 One caveat is we don't raise errors for dynamic decorators on the user side, since we don't know if they're from user markings, or from re-exporting with inputs we've previously marked. Pull Request resolved: #136591 Differential Revision: D63358628 Pulled By: pianpwk

Summary: Removing `_transform_shapes_for_default_dynamic` and `assume_static_by_default=False` as added in #133620. This reverts back to `assume_static_by_default=True` with the use of dynamo decorators (e.g. `maybe_mark_dynamic, mark_static`, instead) for handling Dim.AUTO & Dim.STATIC instead. This is easier to maintain, as it doesn't requiring reasoning about "inverting" the dynamic_shapes specs, and also opens up usage of other decorators (`mark_dynamic, mark_unbacked`). On the user side this change has no effect, but internally this means dynamic behavior is determined only by the `dynamic_shapes` specs (ignoring user-side input decorators following #135536), but transferring this information for _DimHints via decorators, for Dynamo/non-strict to create symbolic_contexts accordingly, e.g. https://github.com/pytorch/pytorch/blob/7c6d543a5b3d65d8f49420e60cda150faaa5b8a0/torch/_dynamo/variables/builder.py#L2646-L2666 One caveat is we don't raise errors for dynamic decorators on the user side, since we don't know if they're from user markings, or from re-exporting with inputs we've previously marked. Differential Revision: D63358628 Pulled By: pianpwk

facebook-github-bot · 2024-09-26T19:01:07Z

This pull request was exported from Phabricator. Differential Revision: D63358628

facebook-github-bot · 2024-09-27T17:07:59Z

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2024-09-27T17:09:46Z

Merge started

Your 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

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2024-09-27T17:09:53Z

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Lint / lintrunner-noclang / linux-job

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

pianpwk · 2024-09-27T18:25:52Z

@pytorchbot merge -f "unrelated lint"

pytorchmergebot · 2024-09-27T18:28:42Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Summary: sam_fast changes from timeout to fail_to_run after #136591, which "regressed" in a good way. Update the expected result file and continue investigating. [ghstack-poisoned]

Summary: sam_fast changes from timeout to fail_to_run after #136591, which "regressed" in a good way. Update the expected result file and continue investigating. ghstack-source-id: b627ce5 Pull Request resolved: #136996

Summary: sam_fast changes from timeout to fail_to_run after #136591, which "regressed" in a good way. Update the expected result file and continue investigating. Pull Request resolved: #136996 Approved by: https://github.com/ezyang

desertfire · 2024-10-01T18:30:13Z

@pytorchbot merge -f "unrelated lint"

@pianpwk , as a retrospect, the sam_fast failure was really and shouldn't been force merged.

Summary: sam_fast changes from timeout to fail_to_run after pytorch#136591, which "regressed" in a good way. Update the expected result file and continue investigating. Pull Request resolved: pytorch#136996 Approved by: https://github.com/ezyang

pianpwk · 2024-10-02T20:42:24Z

@pytorchbot merge -f "unrelated lint"

@pianpwk , as a retrospect, the sam_fast failure was really and shouldn't been force merged.

@desertfire Ah sorry. I should've realized the test no longer timing out was related

pianpwk requested review from angelayi, avikchaudhuri, tugsbayasgalan, ydwu4 and zhxchen17 as code owners September 25, 2024 00:40

pytorch-bot bot added the ciflow/inductor label Sep 25, 2024

facebook-github-bot added the fb-exported label Sep 25, 2024

facebook-github-bot pushed a commit that referenced this pull request Sep 25, 2024

[export] simplify automatic dynamic shapes processing (#136591)

9201fda

Summary: Pull Request resolved: #136591 Differential Revision: D63358628

facebook-github-bot force-pushed the export-D63358628 branch from 4016546 to 9201fda Compare September 25, 2024 00:45

pianpwk added the release notes: export label Sep 25, 2024

facebook-github-bot force-pushed the export-D63358628 branch from 9201fda to e4bbb73 Compare September 25, 2024 16:56

avikchaudhuri approved these changes Sep 25, 2024

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 25, 2024

facebook-github-bot force-pushed the export-D63358628 branch from 22a8917 to 430edf7 Compare September 25, 2024 20:42

facebook-github-bot force-pushed the export-D63358628 branch from 430edf7 to f5a6ac6 Compare September 25, 2024 21:34

pianpwk force-pushed the export-D63358628 branch from f5a6ac6 to 362cb31 Compare September 25, 2024 21:41

facebook-github-bot force-pushed the export-D63358628 branch from 362cb31 to db71c3d Compare September 26, 2024 19:00

pytorchmergebot added the merging label Sep 27, 2024

pytorchmergebot removed the merging label Sep 27, 2024

pytorchmergebot added the merging label Sep 27, 2024

pytorchmergebot added the Merged label Sep 27, 2024

pytorchmergebot closed this in 6075f56 Sep 27, 2024

pytorchmergebot removed the merging label Sep 27, 2024

desertfire mentioned this pull request Sep 30, 2024

[AOTI] Update sam_fast from timeout to fail_to_run #136996

Closed

github-actions bot deleted the export-D63358628 branch November 3, 2024 02:11

	if marked_unbacked:
	dynamic_size = DimDynamic.SIZE_LIKE_UNBACKED
	elif (
	constraint_size is not None
	or marked_dynamic
	or marked_weak_dynamic
	or is_nested_int(e.size()[i])
	):
	# NB: We could assert static_shapes is False here, but it
	# seems better to allow the user to override symbolic_context in this
	# case
	dynamic_size = DimDynamic.DYNAMIC
	elif static_shapes or config.assume_static_by_default or marked_static:
	dynamic_size = DimDynamic.STATIC
	else:
	dynamic_size = DimDynamic.DUCK

	if constraint_stride is not None:
	dynamic_stride = DimDynamic.DYNAMIC
	else:
	dynamic_stride = DimDynamic.INFER_STRIDE

[export] simplify automatic dynamic shapes processing #136591

[export] simplify automatic dynamic shapes processing #136591

Uh oh!

Conversation

pianpwk commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/136591

❌ 3 New Failures

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

avikchaudhuri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pianpwk commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 25, 2024

Uh oh!

facebook-github-bot commented Sep 26, 2024

Uh oh!

facebook-github-bot commented Sep 27, 2024

Uh oh!

pytorchmergebot commented Sep 27, 2024

Merge started

Uh oh!

pytorchmergebot commented Sep 27, 2024

Merge failed

Uh oh!

pianpwk commented Sep 27, 2024

Uh oh!

pytorchmergebot commented Sep 27, 2024

Merge started

Uh oh!

desertfire commented Oct 1, 2024

Uh oh!

pianpwk commented Oct 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pianpwk commented Sep 25, 2024 •

edited

Loading

pytorch-bot bot commented Sep 25, 2024 •

edited

Loading