KEMBAR78
Unifies OpInfo dtype tests by mruberry · Pull Request #60157 · pytorch/pytorch · GitHub
Skip to content

Unifies OpInfo dtype tests #60157

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 7 commits into from
Closed

Unifies OpInfo dtype tests #60157

wants to merge 7 commits into from

Conversation

@mruberry
Copy link
Collaborator

Simplifies the OpInfo dtype tests and produces nicer error messages, like:

AssertionError: Items in the first set but not the second:
torch.bfloat16
Items in the second set but not the first:
torch.int64 : Attempted to compare [set] types: Expected: {torch.float64, torch.float32, torch.float16, torch.bfloat16}; Actual: {torch.float64, torch.float32, torch.float16, torch.int64}.
The supported dtypes for logcumsumexp on cuda according to its OpInfo are
        {torch.float64, torch.float32, torch.float16, torch.int64}, but the detected supported dtypes are {torch.float64, torch.float32, torch.float16, torch.bfloat16}.
        The following dtypes should be added to the OpInfo: {torch.bfloat16}. The following dtypes should be removed from the OpInfo: {torch.int64}.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 17, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 34c21ae (more details on the Dr. CI page and at hud.pytorch.org/pr/60157):

✅ None of the CI failures appear to be your fault 💚

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

See CircleCI build pytorch_linux_xenial_cuda10_2_cudnn7_py3_multigpu_test (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun) ❄️

Jun 17 10:58:23 ConnectionResetError: [Errno 104] Connection reset by peer 
Jun 17 10:58:23   File "tools/download_mnist.py", line 42, in download
Jun 17 10:58:23     urlretrieve(url, destination_path, reporthook=hook)
Jun 17 10:58:23   File "/opt/conda/lib/python3.6/urllib/request.py", line 277, in urlretrieve
Jun 17 10:58:23     block = fp.read(bs)
Jun 17 10:58:23   File "/opt/conda/lib/python3.6/http/client.py", line 463, in read
Jun 17 10:58:23     n = self.readinto(b)
Jun 17 10:58:23   File "/opt/conda/lib/python3.6/http/client.py", line 507, in readinto
Jun 17 10:58:23     n = self.fp.readinto(b)
Jun 17 10:58:23   File "/opt/conda/lib/python3.6/socket.py", line 586, in readinto
Jun 17 10:58:23     return self._sock.recv_into(b)
Jun 17 10:58:23 ConnectionResetError: [Errno 104] Connection reset by peer
Jun 17 10:58:23 + cleanup
Jun 17 10:58:23 + retcode=1
Jun 17 10:58:23 + set +x
Jun 17 10:58:23 =================== sccache compilation log ===================
Jun 17 10:58:23 =========== If your build fails, please take a look at the log above for possible reasons ===========
Jun 17 10:58:23 Compile requests                      0
Jun 17 10:58:23 Compile requests executed             0
Jun 17 10:58:23 Cache hits                            0
Jun 17 10:58:23 Cache misses                          0
Jun 17 10:58:23 Cache timeouts                        0
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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@mruberry mruberry requested a review from ngimel June 17, 2021 05:17
@facebook-github-bot
Copy link
Contributor

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

ngimel
ngimel approved these changes Jun 17, 2021
View reviewed changes
test/test_ops.py
if sample.output_process_fn_grad is not None:
# Checks for backward support in the same dtype
try:
result = sample.output_process_fn_grad(result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't we decide that process_fn_grad is not needed here? It's needed for proper gradcheck, and then it was added for to_sparse but for the wrong reasons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Follow-up from offline discussion: it is needed for operations like slogdet that produce multiple tensors but with only support for backwarding through some (one) of them

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 59b1003.

@walterddr walterddr mentioned this pull request Jun 21, 2021
Closed
3 tasks
@kshitij12345 kshitij12345 mentioned this pull request Jun 21, 2021
Closed
@xwang233 xwang233 mentioned this pull request Jun 22, 2021
Closed
@mruberry mruberry deleted the op_info_docs_and_dtypes branch January 14, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ngimel ngimel ngimel approved these changes

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mruberry @facebook-github-bot @ngimel