sparse gradcheck: retire gradcheck_semantics by nikitaved · Pull Request #99043 · pytorch/pytorch

nikitaved · 2023-04-13T15:47:11Z

Once the tests are parametrized to properly reflect domains/co-domains of sparse functions, success with masked=False implies success with masked=True, so there is no need in running gradcheck twice.

This PR only affects the tests, it does not touch any gradcheck code.

Stack from ghstack (oldest at bottom):

cc @alexsamardzic @pearu @cpuhrsch @amjames @bhosmer @ezyang @albanD @zou3519 @gqchen @soulitzer @lezcano @Varal7

[ghstack-poisoned]

pytorch-bot · 2023-04-13T15:47:16Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❌ 1 New Failure

As of commit 6f1678b:

NEW FAILURE - The following job has failed:

lintrunner / linux-job (gh)

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

ghstack-source-id: 80c43b5 Pull Request resolved: #99043

pearu · 2023-04-14T06:51:47Z

@nikitaved I think you have missed the point of these tests that this PR removes and the previous PR modifies. These tests cover the functionality of gradcheck for masked being True or False. When removing masked=True checks, you essentially leave the corresponding paths in gradcheck untested.

Also, notice that if a test passes with masked=False, this does not always imply that the test will pass with masked=True. This implication is true only when the sparse inputs are full tensors (_nnz() == numel() when non-hybrid, for instance) because the densification (that is applied when masked=False) will be a no-op. Otherwise, when input tensors have unspecified elements, the gradcheck with masked=True has subtleties that are not covered with masked=False.

In fact, I think the general implication is: if a test passes with masked=True then it will pass with masked=False. So, I find this and the previous PR unacceptable from a point of view of reducing the test coverage of gradcheck.
From the operations point of view, indeed, checking with masked=False will be sufficient in the domain of non-masked semantics but we'll still need to test the masked semantics as this is what ops in torch.sparse namespace implement and a few operations in torch namespace support.

Once the tests are parametrized to properly reflect domains/co-domains of sparse functions, success with `masked=False` implies success with `masked=True`, so there is no need in running gradcheck twice. This PR only affects the tests, it does not touch any gradcheck code. cc alexsamardzic pearu cpuhrsch amjames bhosmer ezyang albanD zou3519 gqchen soulitzer Lezcano Varal7 [ghstack-poisoned]

nikitaved · 2023-04-17T10:36:10Z

After an offline discussion with @pearu we did reach consensus. Namely, it would be great to have masked=True/False to test logic in gradcheck. It looks, like, @pearu , these cases are covered in test/test_autograd.py, so we should be safe here...

Once the tests are parametrized to properly reflect domains/co-domains of sparse functions, success with `masked=False` implies success with `masked=True`, so there is no need in running gradcheck twice. This PR only affects the tests, it does not touch any gradcheck code. cc alexsamardzic pearu cpuhrsch amjames bhosmer ezyang albanD zou3519 gqchen soulitzer Lezcano Varal7 [ghstack-poisoned]

pearu · 2023-04-17T12:57:00Z

It looks, like, @pearu , these cases are covered in test/test_autograd.py, so we should be safe here...

Hmm, can you be more specific? AFAIK, test_autograd.py does not cover all the combinations of to_dense(masked_grad=...) and gradcheck(..., masked=..., fast_mode=...) usages (although ciflow/periodic will cover the fast_mode variations).

nikitaved · 2023-04-17T13:02:07Z

@pearu, for example, have a look at

pytorch/test/test_autograd.py

Line 4721 in 0157b2d

def test_gradcheck_validates_inputs(self):

.

pearu · 2023-04-17T13:43:36Z

@nikitaved

0157b2d

this covers only the COO layout but there are no tests with sparse compressed layouts.

nikitaved · 2023-04-17T13:46:30Z

@pearu , none of the tests I modified in test_sparse.py concern any compressed formats IIRC...

nikitaved · 2023-04-17T13:54:04Z

test/test_sparse.py

-            r = gradcheck(lambda x: torch.Tensor.to_dense(x, masked_grad=gradcheck.masked), t)
+            r = gradcheck(lambda x: torch.Tensor.to_dense(x, masked_grad=False), t)


Ah, ok, this one touches compressed formats. Will update it, @pearu .

Once the tests are parametrized to properly reflect domains/co-domains of sparse functions, success with `masked=False` implies success with `masked=True`, so there is no need in running gradcheck twice. This PR only affects the tests, it does not touch any gradcheck code. cc alexsamardzic pearu cpuhrsch amjames bhosmer ezyang albanD zou3519 gqchen soulitzer Lezcano Varal7 [ghstack-poisoned]

github-actions · 2023-09-01T14:33:44Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

sparse gradcheck: retire gradcheck_semantics

aa87f5c

[ghstack-poisoned]

This was referenced Apr 13, 2023

sparse.mm backward: performance improvements #94991

Closed

relu/threshold backward for sparse: enable 0-nnz grads #98935

Closed

pytorch-bot bot added the topic: not user facing topic category label Apr 13, 2023

This was referenced Apr 13, 2023

sparse.sum backward: short circuit on zero/empty grad #98838

Closed

sparse_mask: backward support for sparse lhs #95165

Closed

sparse gradcheck: reparametrize some tests to remove masked=True #98490

Closed

nikitaved added a commit that referenced this pull request Apr 13, 2023

sparse gradcheck: retire gradcheck_semantics

be6aa10

ghstack-source-id: 80c43b5 Pull Request resolved: #99043

nikitaved requested review from albanD, amjames, cpuhrsch and pearu April 13, 2023 15:57

pytorchbot added the open source label Apr 13, 2023

nikitaved added module: sparse Related to torch.sparse module: autograd Related to torch.autograd, and the autograd engine in general labels Apr 13, 2023

nikitaved mentioned this pull request Apr 17, 2023

sparse.sum backward: fix gradient projection #99298

Closed

nikitaved commented Apr 17, 2023

View reviewed changes

nikitaved added 4 commits April 17, 2023 14:06

nikitaved added 12 commits April 26, 2023 10:30

nikitaved mentioned this pull request Jun 28, 2023

sparse_mask: backward support for sparse lhs (take 2) #104341

Closed

nikitaved added 13 commits June 28, 2023 12:46

github-actions bot added the Stale label Sep 1, 2023

github-actions bot closed this Oct 1, 2023

facebook-github-bot deleted the gh/nikitaved/38/head branch November 1, 2023 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sparse gradcheck: retire gradcheck_semantics#99043

sparse gradcheck: retire gradcheck_semantics#99043
nikitaved wants to merge 39 commits intogh/nikitaved/38/basefrom
gh/nikitaved/38/head

nikitaved commented Apr 13, 2023 •

edited

Loading

Uh oh!

pytorch-bot bot commented Apr 13, 2023 •

edited

Loading

Uh oh!

pearu commented Apr 14, 2023

Uh oh!

nikitaved commented Apr 17, 2023 •

edited

Loading

Uh oh!

pearu commented Apr 17, 2023

Uh oh!

nikitaved commented Apr 17, 2023

Uh oh!

pearu commented Apr 17, 2023

Uh oh!

nikitaved commented Apr 17, 2023 •

edited

Loading

Uh oh!

nikitaved Apr 17, 2023

Uh oh!

github-actions bot commented Sep 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		r = gradcheck(lambda x: torch.Tensor.to_dense(x, masked_grad=gradcheck.masked), t)
		r = gradcheck(lambda x: torch.Tensor.to_dense(x, masked_grad=False), t)

Conversation

nikitaved commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 New Failure

Uh oh!

pearu commented Apr 14, 2023

Uh oh!

nikitaved commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pearu commented Apr 17, 2023

Uh oh!

nikitaved commented Apr 17, 2023

Uh oh!

pearu commented Apr 17, 2023

Uh oh!

nikitaved commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikitaved Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nikitaved commented Apr 13, 2023 •

edited

Loading

pytorch-bot bot commented Apr 13, 2023 •

edited

Loading

nikitaved commented Apr 17, 2023 •

edited

Loading

nikitaved commented Apr 17, 2023 •

edited

Loading