Skip to content

[einsum] Fix opt_einsum defaults to be more reasonable #86985

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
Closed
janeyx99 wants to merge 1 commit into from
Closed

[einsum] Fix opt_einsum defaults to be more reasonable #86985

janeyx99 wants to merge 1 commit into from

Conversation

@janeyx99
Copy link
Contributor

@janeyx99 janeyx99 commented Oct 14, 2022

Fixes the confusing situation mentioned here #85224 (comment) by

  • setting better OG defaults
  • changing warnings to errors now that we have better defaults

Test plan:

  • Ran einsum tests locally + CI
  • Uninstalled opt-einsum and ran through setting
    • enabled to False (doesn't throw error)
    • strategy to anything that's not None (errors)
    • strategy to None (noops)
  • Installed opt-einsum and ran through setting
    • enabled to False (doesn't throw error)
    • enabled to True (doesn't throw error, no ops + defaults to 'auto')
    • strategy to random string (errors)
    • strategy to None (noops, still is 'auto')
    • strategy to 'greedy' (is set to 'greedy')

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 14, 2022 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures, 1 Pending

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

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

@janeyx99 janeyx99 mentioned this pull request Oct 14, 2022
Closed
@janeyx99 janeyx99 marked this pull request as ready for review October 14, 2022 19:28
@janeyx99 janeyx99 requested a review from soulitzer October 14, 2022 19:29
soulitzer
soulitzer approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@soulitzer soulitzer left a comment

Choose a reason for hiding this comment

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

OK!

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 14, 2022

This PR has been accepted with the accept2ship label. Attempting to merge now.

@pytorchbot merge -l

@pytorch-bot pytorch-bot bot added ciflow/trunk Trigger trunk jobs on your pull request and removed accept2ship labels Oct 14, 2022
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 14, 2022

Merge started

The -l land checks flag is deprecated and no longer needed. Instead we now automatically add the ciflow\trunk label to your PR once it's approved

Your change will be merged once all checks on your PR 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

pytorchmergebot commented Oct 14, 2022

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

@janeyx99
Copy link
Contributor Author

janeyx99 commented Oct 15, 2022

@pytorchbot merge -r

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 15, 2022

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 15, 2022

Successfully rebased modify-confusing-defaults onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout modify-confusing-defaults && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the modify-confusing-defaults branch from 0b4fef9 to c9943a0 Compare October 15, 2022 03:37
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 15, 2022

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 15, 2022

Hey @janeyx99.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

janeyx99 added a commit that referenced this pull request Oct 17, 2022
@janeyx99
[einsum] Fix opt_einsum defaults to be more reasonable (#86985)
7612969 
Fixes the confusing situation mentioned here #85224 (comment) by

- setting better OG defaults
- changing warnings to errors now that we have better defaults

Test plan:
- Ran einsum tests locally + CI
- Uninstalled opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `strategy` to anything that's not None (errors)
     - `strategy` to None (noops)
- Installed opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `enabled` to True (doesn't throw error, no ops + defaults to 'auto')
     - `strategy` to random string (errors)
     - `strategy` to None (noops, still is 'auto')
     - `strategy` to 'greedy' (is set to 'greedy')
Pull Request resolved: #86985
Approved by: https://github.com/soulitzer
This was referenced Oct 17, 2022
Merged
Closed
pytorchmergebot pushed a commit that referenced this pull request Oct 18, 2022
@janeyx99 @pytorchmergebot
[einsum] Fix opt_einsum defaults to be more reasonable (#86985)
ae8a5b1 
Fixes the confusing situation mentioned here #85224 (comment) by

- setting better OG defaults
- changing warnings to errors now that we have better defaults

Test plan:
- Ran einsum tests locally + CI
- Uninstalled opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `strategy` to anything that's not None (errors)
     - `strategy` to None (noops)
- Installed opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `enabled` to True (doesn't throw error, no ops + defaults to 'auto')
     - `strategy` to random string (errors)
     - `strategy` to None (noops, still is 'auto')
     - `strategy` to 'greedy' (is set to 'greedy')
Pull Request resolved: #86985
Approved by: https://github.com/soulitzer
atalman pushed a commit that referenced this pull request Oct 19, 2022
@janeyx99
[einsum] Fix opt_einsum defaults to be more reasonable (#86985) (#87144)
e6d8c13 
Fixes the confusing situation mentioned here #85224 (comment) by

- setting better OG defaults
- changing warnings to errors now that we have better defaults

Test plan:
- Ran einsum tests locally + CI
- Uninstalled opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `strategy` to anything that's not None (errors)
     - `strategy` to None (noops)
- Installed opt-einsum and ran through setting
     - `enabled` to False (doesn't throw error)
     - `enabled` to True (doesn't throw error, no ops + defaults to 'auto')
     - `strategy` to random string (errors)
     - `strategy` to None (noops, still is 'auto')
     - `strategy` to 'greedy' (is set to 'greedy')
Pull Request resolved: #86985
Approved by: https://github.com/soulitzer
@malfet malfet mentioned this pull request Nov 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soulitzer soulitzer soulitzer approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@janeyx99 @pytorchmergebot @soulitzer