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.

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

[user errors] compulsory case names, allow multiple #110878

Closed

Conversation

avikchaudhuri
Copy link
Contributor

@avikchaudhuri avikchaudhuri commented Oct 9, 2023

Stack from ghstack (oldest at bottom):

We want to get to a point where most UserErrors link to exportdb examples. This PR makes passing case names non-optional to make this intent clearer and encourage developers who raise UserErrors to make or point to examples that make fixing such errors more obvious for users.

In addition, sometimes there are multiple examples that are relevant to an error. Thus this PR also enables passing multiple case names.

Retry of #110733 which was reverted due to a landrace.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng @gmagogsfm @zhxchen17 @tugsbayasgalan

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 9, 2023

🔗 Helpful Links

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

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

✅ You can merge normally! (2 Unrelated Failures)

As of commit 171ee55 with merge base ada6550 (image):

UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:

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

avikchaudhuri added a commit that referenced this pull request Oct 9, 2023
Differential Revision: [D50087148](https://our.internmc.facebook.com/intern/diff/D50087148/)

ghstack-source-id: 203396070
Pull Request resolved: #110878
We want to get to a point where most UserErrors link to exportdb examples. This PR makes passing case names non-optional to make this intent clearer and encourage developers who raise UserErrors to make or point to examples that make fixing such errors more obvious for users.

In addition, sometimes there are multiple examples that are relevant to an error. Thus this PR also enables passing multiple case names.

Retry of #110733 which was reverted due to a landrace.

Differential Revision: [D50087148](https://our.internmc.facebook.com/intern/diff/D50087148/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng gmagogsfm zhxchen17 tugsbayasgalan

[ghstack-poisoned]
avikchaudhuri added a commit that referenced this pull request Oct 9, 2023
Pull Request resolved: #110878


ghstack-source-id: 203415401
@exported-using-ghexport

Differential Revision: [D50087148](https://our.internmc.facebook.com/intern/diff/D50087148/)
@avikchaudhuri
Copy link
Contributor Author

@pytorchbot merge

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

Merge failed

Reason: This PR needs a release notes: 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

@avikchaudhuri avikchaudhuri added the release notes: fx release notes category label Oct 10, 2023
@avikchaudhuri
Copy link
Contributor Author

@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

@kit1980
Copy link
Member

kit1980 commented Oct 10, 2023

@pytorchbot revert -m "export/test_export.py::TestExport::test_multiple_definitions_same_name_dim - TypeError: UserError.init() missing 1 required positional argument: 'case_names'" -c landrace

@pytorch pytorch deleted a comment from pytorch-bot bot Oct 10, 2023
@kit1980
Copy link
Member

kit1980 commented Oct 10, 2023

@avikchaudhuri Looks like a landrace with your own PR #110638

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

Can't revert PR that was landed via phabricator as D50087148. Please revert by going to the internal diff and clicking Unland.

@kit1980
Copy link
Member

kit1980 commented Oct 10, 2023

I'm unlinking Unpublished diff D50087148 to revert this.

@kit1980
Copy link
Member

kit1980 commented Oct 10, 2023

@pytorchbot revert -m "export/test_export.py::TestExport::test_multiple_definitions_same_name_dim - TypeError: UserError.init() missing 1 required positional argument: 'case_names'" -c landrace

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@avikchaudhuri your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Oct 10, 2023
This reverts commit 2ae71c4.

Reverted #110878 on behalf of https://github.com/kit1980 due to export/test_export.py::TestExport::test_multiple_definitions_same_name_dim - TypeError: UserError.init() missing 1 required positional argument: 'case_names' ([comment](#110878 (comment)))
@avikchaudhuri
Copy link
Contributor Author

@kit1980 Just want to confirm whether everything is in sync.

  1. Initial attempt at this PR was [user errors] compulsory case names, allow multiple #110733.
  2. PR in (1) got reverted by Revert "[user errors] compulsory case names, allow multiple (#110733)" #110783. Yet PR (1) got imported / landed internally as D50090877. This might be because the revert was manual?
  3. The current PR was a retry of the original. Now this is reverted. So net effect is that GH doesn't have this change (neither from the retry or the original). So hopefully nothing is broken on GH.
  4. However, fbcode still shows the change because of (2). And it also has the change different bounds for same Dim name #110638, which you linked as the causing the land race, and which landed internally as D49965944. So I'm afraid the test is currently broken on fbcode.

This doesn't seem OK, does it?

@kit1980
Copy link
Member

kit1980 commented Oct 10, 2023

@avikchaudhuri #110783 will be imported and landed to fbcode, so everything will be fine.

We're a bit behind on fbcode sync since Friday.

@facebook-github-bot facebook-github-bot deleted the gh/avikchaudhuri/53/head branch October 13, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo release notes: fx release notes category Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants