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

Temporary fix for determinant bug on CPU #35136

Closed
wants to merge 5 commits into from

Conversation

vishwakftw
Copy link
Contributor

Changelog:

  • Make diagonal contiguous

Temporarily Fixes #34061

Changelog:
- Make diagonal contiguous
@dr-ci
Copy link

dr-ci bot commented Mar 21, 2020

💊 CI failures summary and remediations

As of commit 0321b5c (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 11 times.

@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Mar 24, 2020
@vishwakftw
Copy link
Contributor Author

@ezyang @gchanan could you please review this PR?

@ezyang
Copy link
Contributor

ezyang commented Jul 7, 2020

Sure. But can we get a test?

@vishwakftw
Copy link
Contributor Author

Sure, I'll add it, and ping you.

@ezyang
Copy link
Contributor

ezyang commented Jul 10, 2020

rebase on master to fix the docker problems

@ezyang ezyang closed this Jul 10, 2020
@ezyang ezyang reopened this Jul 10, 2020
@vishwakftw
Copy link
Contributor Author

ROCM test failure is unrelated. cc: @ezyang

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in fcd6d91.

@mruberry
Copy link
Collaborator

Unlanding. This appears to have broken the slow test build. You can likely reproduce this failure locally by setting PYTORCH_TEST_WITH_SLOW=1 before running test_torch.py. Failing log:

Jul 15 06:19:32 ======================================================================
Jul 15 06:19:32 ERROR [0.018s]: test_det_logdet_slogdet_cpu_float64 (main.TestTorchDeviceTypeCPU)
Jul 15 06:19:32 ----------------------------------------------------------------------
Jul 15 06:19:32 Traceback (most recent call last):
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 241, in instantiated_test
Jul 15 06:19:32 result = test(self, device_arg, dtype)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 463, in wrapper
Jul 15 06:19:32 fn(*args, **kwargs)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 411, in dep_fn
Jul 15 06:19:32 return fn(slf, device, *args, **kwargs)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 411, in dep_fn
Jul 15 06:19:32 return fn(slf, device, *args, **kwargs)
Jul 15 06:19:32 File "test_torch.py", line 7570, in test_det_logdet_slogdet
Jul 15 06:19:32 test_single_det(q, ref_det, ref_logabsdet, 'orthogonal')
Jul 15 06:19:32 TypeError: test_single_det() takes 3 positional arguments but 4 were given
Jul 15 06:19:32
Jul 15 06:19:32 ======================================================================
Jul 15 06:19:32 ERROR [0.081s]: test_det_logdet_slogdet_cuda_float64 (main.TestTorchDeviceTypeCUDA)
Jul 15 06:19:32 ----------------------------------------------------------------------
Jul 15 06:19:32 Traceback (most recent call last):
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 777, in wrapper
Jul 15 06:19:32 method(*args, **kwargs)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 241, in instantiated_test
Jul 15 06:19:32 result = test(self, device_arg, dtype)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 463, in wrapper
Jul 15 06:19:32 fn(*args, **kwargs)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 411, in dep_fn
Jul 15 06:19:32 return fn(slf, device, *args, **kwargs)
Jul 15 06:19:32 File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 411, in dep_fn
Jul 15 06:19:32 return fn(slf, device, *args, **kwargs)
Jul 15 06:19:32 File "test_torch.py", line 7570, in test_det_logdet_slogdet
Jul 15 06:19:32 test_single_det(q, ref_det, ref_logabsdet, 'orthogonal')
Jul 15 06:19:32 TypeError: test_single_det() takes 3 positional arguments but 4 were given
Jul 15 06:19:32
Jul 15 06:19:33 ----------------------------------------------------------------------

@vishwakftw
Copy link
Contributor Author

Oh sorry about that, I know where the bug is.

@vishwakftw vishwakftw reopened this Jul 15, 2020
@vishwakftw
Copy link
Contributor Author

@mruberry is there some way to run the slow tests for a PR?

@vishwakftw vishwakftw requested a review from mruberry July 15, 2020 17:07
@mruberry
Copy link
Collaborator

@mruberry is there some way to run the slow tests for a PR?

In the CI you mean? Technically yes but it's laborious. You'd have to add the slow test build to the CI metadata with your PR. This would create a new CircleCI build on the PR. Then you'd have to remove it and resubmit the PR since we wouldn't want to actually commit that CI change. Better to just run it locally, I think.

@vishwakftw
Copy link
Contributor Author

@mruberry this is my test log

PYTORCH_TEST_WITH_SLOW=1 pytest test/test_torch.py -k det_logdet_slogdet -v
================================================ test session starts ================================================
platform linux -- Python 3.7.3, pytest-5.0.1, py-1.8.1, pluggy-0.13.1 -- /home/vishwak/miniconda3/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/media/vishwak/Official-1/pytorch/.hypothesis/examples')
rootdir: /media/vishwak/Official-1/pytorch
plugins: hypothesis-4.23.6
collected 5973 items / 5969 deselected / 4 selected                                                                 

test/test_torch.py::TestTorchDeviceTypeCPU::test_det_logdet_slogdet_batched_cpu_float64 PASSED                [ 25%]
test/test_torch.py::TestTorchDeviceTypeCPU::test_det_logdet_slogdet_cpu_float64 PASSED                        [ 50%]
test/test_torch.py::TestTorchDeviceTypeCUDA::test_det_logdet_slogdet_batched_cuda_float64 PASSED              [ 75%]
test/test_torch.py::TestTorchDeviceTypeCUDA::test_det_logdet_slogdet_cuda_float64 PASSED                      [100%]

@mruberry
Copy link
Collaborator

@ezyang Do you want to reimport or for me to commandeer?

@ezyang
Copy link
Contributor

ezyang commented Jul 16, 2020

you are always welcome to commandeer oss, no need to ask (but now that I am here, I have reimported)

@vishwakftw
Copy link
Contributor Author

@ezyang could you please reland this PR? Thank you.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@vishwakftw vishwakftw deleted the fix-det-bug-cpu branch July 24, 2020 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect det value on CPU
6 participants