Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add mark_static #98427

Closed
wants to merge 2 commits into from
Closed

Add mark_static #98427

wants to merge 2 commits into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Apr 5, 2023 •
edited by pytorch-bot bot

@ezyang
Add mark_static
764f5f6 
Signed-off-by: Edward Z. Yang <ezyang@meta.com>

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Apr 5, 2023 •
edited

🔗 Helpful Links

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

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

✅ No Failures

As of commit 88a1d3b:
💚 Looks good so far! There are no failures yet. 💚

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

ezyang added a commit that referenced this pull request Apr 5, 2023
@ezyang
Add mark_static
e17f779 
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

ghstack-source-id: ca2c80ca237d8bbf945f8bb9abdbfa03089ce0a5
Pull Request resolved: #98427
@github-actions github-actions bot requested a review from wconstab April 5, 2023 17:52
voznesenskym
voznesenskym reviewed Apr 5, 2023
View reviewed changes
torch/_dynamo/variables/builder.py
Comment on lines +1171 to +1172
marked_dynamic = i in getattr(e, "_dynamo_dynamic_indices", set())
marked_static = i in getattr(e, "_dynamo_static_indices", set())
Copy link
Contributor

Choose a reason for hiding this comment

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

If it has both, should we just raise? Feels like user error, and it would be irritating to think you are marking something static and not seeing anything change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm slightly in favor of defining a precedence instead, mostly because I had some experiences with the mark_dynamic having too aggressive error checking that I needed to relax.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 5, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 5, 2023
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@ezyang
Copy link
Contributor Author

ezyang commented Apr 5, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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
Copy link
Collaborator

Merge failed

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

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@albanD albanD removed their request for review April 5, 2023 18:22
@ezyang
Update on "Add mark_static"
88a1d3b 
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire

[ghstack-poisoned]
@ezyang
Copy link
Contributor Author

ezyang commented Apr 6, 2023

@pytorchbot merge -f "lint fix only"

ezyang added a commit that referenced this pull request Apr 6, 2023
@ezyang
Add mark_static
4060f45 
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

ghstack-source-id: 87817811ac9fc6f27210e45999e959d41dadefeb
Pull Request resolved: #98427
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

ZainRizvi pushed a commit that referenced this pull request Apr 19, 2023
@ezyang @ZainRizvi
Add mark_static (#98427)
ddd6879 
Signed-off-by: Edward Z. Yang <ezyang@meta.com>

Pull Request resolved: #98427
Approved by: https://github.com/voznesenskym
@facebook-github-bot facebook-github-bot deleted the gh/ezyang/1966/head branch June 8, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voznesenskym voznesenskym voznesenskym approved these changes

@antoniojkim antoniojkim Awaiting requested review from antoniojkim

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

@jbschlosser jbschlosser Awaiting requested review from jbschlosser

@miladm miladm Awaiting requested review from miladm

@SherlockNoMad SherlockNoMad Awaiting requested review from SherlockNoMad

@wconstab wconstab Awaiting requested review from wconstab

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo release notes: dynamo topic: new features topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ezyang @pytorchmergebot @voznesenskym