KEMBAR78
adding test_sparse_csr to run_test by walterddr · Pull Request #58666 · pytorch/pytorch · GitHub
Skip to content

adding test_sparse_csr to run_test #58666

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 1 commit into from
Closed

adding test_sparse_csr to run_test #58666

wants to merge 1 commit into from

Conversation

@walterddr
Copy link
Contributor

@walterddr walterddr commented May 20, 2021 •
edited
Loading

fixes #58632.

Added several skips that relates to test assert and MKL. Will address them in separate PR.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 20, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 52c5d87 (more details on the Dr. CI page):

  • 3/3 failures introduced in this PR

3 failures not recognized by patterns:

Job Step Action
CircleCI pytorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_test2 Run tests 🔁 rerun
CircleCI pytorch_linux_bionic_cuda10_2_cudnn7_py3_9_gcc7_build Build 🔁 rerun
CircleCI pytorch_linux_xenial_py3_6_gcc5_4_build (Optional) Merge target branch 🔁 rerun
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.

@walterddr walterddr force-pushed the test_sparse_csr branch 3 times, most recently from 77e5b7e to f1d336f Compare May 21, 2021 13:28
@walterddr walterddr marked this pull request as ready for review May 21, 2021 14:50
@walterddr walterddr requested review from aocsa and malfet May 21, 2021 14:50
@walterddr walterddr mentioned this pull request May 21, 2021
Closed
walterddr
walterddr commented May 21, 2021
View reviewed changes
test/expect/TestSparseCSRCPU.test_sparse_csr_print_cpu.expect Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

crow_indices are all zero from this point forward in the original expect file. not sure if there's a copy/paste issue. plz review.

aocsa
aocsa suggested changes May 21, 2021
View reviewed changes
Copy link
Contributor

@aocsa aocsa left a comment

Choose a reason for hiding this comment

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

Minor comments about the tests, just one skip test is needed test_mkl_matvec_warnings. Actually before solving this issue, this one #58757 , related to CSR matmul bug, should be solved.

test/test_sparse_csr.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this skip test is not needed here.

test/test_sparse_csr.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this skip test is the only one needed due to is the only one expecting and MKL related warning

test/test_sparse_csr.py Outdated
Comment on lines +84 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

This test was wrong from the beginning. It uses genSparseCSRTensor which generates a random csr tensor however this test compares with an expected and static string.
See :

pytorch/test/test_sparse_csr.py

Line 223 in f4320cc

def test_sparse_csr_print(self, device):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we remove/skip this for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to generate csr tensor in this way:

pytorch/test/test_sparse_csr.py

Line 242 in f4320cc

x = torch.sparse_csr_tensor(torch.tensor([0, 2, 4], dtype=index_dtype),

So when CUDA support is ready for merge it will not generate to much conflicts

walterddr
walterddr commented May 21, 2021
View reviewed changes
Copy link
Contributor Author

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

Thanks for the review @aocsa . For the skipped tests, errors on windows and mac are slightly different. Please see:
windows and mac test results.

Also I would prefer landing this asap so that we can at least have some test coverage gating changes for CSR sparse operation. what do you think?

test/test_sparse_csr.py Outdated
Comment on lines +84 to +89
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we remove/skip this for now?

@aocsa
Copy link
Contributor

aocsa commented May 21, 2021

Thanks for the review @aocsa . For the skipped tests, errors on windows and mac are slightly different. Please see:
windows and mac test results.

Also I would prefer landing this asap so that we can at least have some test coverage gating changes for CSR sparse operation. what do you think?

Agree with the idea to skip for now and comment with a TODO with the issue number attached here #58757.

@walterddr walterddr mentioned this pull request May 21, 2021
Closed
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@walterddr merged this pull request in a700204.

@walterddr walterddr mentioned this pull request May 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malfet malfet malfet approved these changes

+1 more reviewer

@aocsa aocsa aocsa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test/test_sparse_csr.py is not run by CI

4 participants

@walterddr @facebook-github-bot @aocsa @malfet