8000 Added {logical_not, trace} refs, moved logical ops to use method overloads by Chillee · Pull Request #79000 · pytorch/pytorch · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added {logical_not, trace} refs, moved logical ops to use method overloads #79000

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
wants to merge 16 commits into from
Closed

Added {logical_not, trace} refs, moved logical ops to use method overloads #79000

wants to merge 16 commits into from

Conversation

Chillee
Copy link
Collaborator
@Chillee Chillee commented Jun 7, 2022 •
edited
Loading

8000

@Chillee Chillee requested review from mruberry and ngimel as code owners June 7, 2022 05:32
This was referenced Jun 7, 2022
Closed
Closed
Closed
@facebook-github-bot
Copy link
Contributor
facebook-github-bot commented Jun 7, 2022 •
edited
Loading

🔗 Helpful links

❌ 1 New Failures

As of commit 003d457 (more details on the Dr. CI page):

Expand to see more
  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages

See GitHub Actions build pull / linux-xenial-cuda11.3-py3.7-gcc7 / test (default, 1, 4, linux.4xlarge.nvidia.gpu) (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

2022-06-09T08:00:52.8118035Z RuntimeError: test_ops failed! 
2022-06-09T08:00:49.1600273Z Generating XML reports...
2022-06-09T08:00:50.9467518Z Generated XML report: test-reports/python-unittest/test_ops/TEST-TestCommonCUDA-20220609053643.xml
2022-06-09T08:00:51.1290874Z Generated XML report: test-reports/python-unittest/test_ops/TEST-TestCompositeComplianceCUDA-20220609053643.xml
2022-06-09T08:00:51.6090988Z Generated XML report: test-reports/python-unittest/test_ops/TEST-TestMathBitsCUDA-20220609053643.xml
2022-06-09T08:00:51.6895774Z Generated XML report: test-reports/python-unittest/test_ops/TEST-TestFakeTensorNonErroringCUDA-20220609053643.xml
2022-06-09T08:00:52.8110157Z Traceback (most recent call last):
2022-06-09T08:00:52.8110566Z   File "test/run_test.py", line 1077, in <module>
2022-06-09T08:00:52.8114232Z     main()
2022-06-09T08:00:52.8114637Z   File "test/run_test.py", line 1055, in main
2022-06-09T08:00:52.8117559Z     raise RuntimeError(err_message)
2022-06-09T08:00:52.8118035Z RuntimeError: test_ops failed!
2022-06-09T08:00:53.5945817Z + cleanup
2022-06-09T08:00:53.5946224Z + retcode=1
2022-06-09T08:00:53.5946445Z + set +x
2022-06-09T08:00:53.5992379Z ##[error]Process completed with exit code 1.
2022-06-09T08:00:53.6038730Z ##[group]Run pytorch/pytorch/.github/actions/get-workflow-job-id@master
2022-06-09T08:00:53.6050750Z with:
2022-06-09T08:00:53.6051398Z   github-token: ***
2022-06-09T08:00:53.6051629Z env:
2022-06-09T08:00:53.6051847Z   IN_CI: 1
2022-06-09T08:00:53.6052065Z   IS_GHA: 1
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@Chillee
Copy link
Collaborator Author
Chillee commented Jun 7, 2022

One thing I did in this PR that's questionable is porting trace from a decomp to a ref. Currently we don't have the diag ref implemented yet (so I need to xfail most of the ref tests), but I feel like it might be worth it in order to unify?

@Chillee Chillee mentioned this pull request Jun 7, 2022
Closed
@Chillee Chillee mentioned this pull request Jun 7, 2022
Closed
@Chillee Chillee requested a review from ezyang June 7, 2022 17:40
mruberry
mruberry reviewed Jun 7, 2022
View reviewed changes
mruberry
mruberry reviewed Jun 7, 2022
View reviewed changes
torch/_refs/__init__.py Outdated
# populate the decomp table
@register_decomposition(torch.ops.aten.trace)
def trace(self: TensorLikeType) -> TensorLikeType:
return torch.sum(torch.diag(self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect because diag accepts vectors and matrices but trace only accepts matrices

Copy link
Collaborator

Choose a reason for hiding this comment

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

(what about adding an error input for it?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an error input for it (although it's unfortunately not tested now for ref implementations since diag isn't there).

mruberry
mruberry reviewed Jun 7, 2022
View reviewed changes
@mruberry
Copy link
Collaborator
mruberry commented Jun 7, 2022

Hey @Chillee! Looks good overall -- a few small comments for your review inline

@mruberry
Copy link
Collaborator
mruberry commented Jun 7, 2022

One thing I did in this PR that's questionable is porting trace from a decomp to a ref. Currently we don't have the diag ref implemented yet (so I need to xfail most of the ref tests), but I feel like it might be worth it in order to unify?

Yeah don't worry about it

@mruberry
Copy link
Collaborator
mruberry commented Jun 8, 2022

Unfortunately test failures look related

@mruberry
Copy link
Collaborator
mruberry commented Jun 9, 2022

fyi @zasdfgbnm

@malfet
Copy link
Contributor
malfet commented Jun 9, 2022

@pytorchbot revert -m "Introduces test failure, see https://hud.pytorch.org/pr/79000" -c ignoredsignal

@pytorchmergebot
Copy link
Collaborator

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

pytorchmergebot added a commit that referenced this pull request Jun 9, 2022
@pytorchmergebot
Revert "Added {logical_not, trace} refs, moved logical ops to use met…
d67309a 
…hod overloads"

This reverts commit 64b6bd8.

Reverted #79000 on behalf of https://github.com/malfet due to Introduces test failure, see https://hud.pytorch.org/pr/79000
facebook-github-bot pushed a commit that referenced this pull request Jun 10, 2022
@Chillee @facebook-github-bot
Added {logical_not, trace} refs, moved logical ops to use method over…
d572bd0 
…loads (#79000)

Summary:
Pull Request resolved: #79000

Approved by: https://github.com/ezyang

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/64b6bd8c1e73abb039c111bb7a9ca37a5baff71e

Reviewed By: osalpekar

Differential Revision: D37058880

Pulled By: osalpekar

fbshipit-source-id: 3b8e1d7343815422f08b17ba79727fac0592d709
facebook-github-bot pushed a commit that referenced this pull request Jun 10, 2022
@pytorchmergebot @facebook-github-bot
Revert "Added {logical_not, trace} refs, moved logical ops to use met…
bc68702 
…hod overloads"

Summary:
This reverts commit 64b6bd8.

Reverted #79000 on behalf of https://github.com/malfet due to Introduces test failure, see https://hud.pytorch.org/pr/79000

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/d67309aefb435930c77d45935181b3238cf5772e

Reviewed By: osalpekar

Differential Revision: D37059062

Pulled By: osalpekar

fbshipit-source-id: 0b6d659b1d8601ee65ffd93381559ca7c6e2ba83
@facebook-github-bot facebook-github-bot deleted the gh/chillee/52/head branch June 12, 2022 14:19
@janeyx99 janeyx99 mentioned this pull request Jun 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mruberry mruberry mruberry left review comments

@ezyang ezyang ezyang approved these changes

@ngimel ngimel Awaiting requested review from ngimel

Assignees
No one assigned
Labels
cla signed Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Chillee @facebook-github-bot @mruberry @malfet @pytorchmergebot @ezyang
0