Run only one pytest parametrization when generating optest by ezyang · Pull Request #108936 · pytorch/pytorch

ezyang · 2023-09-09T03:37:59Z

Stack from ghstack (oldest at bottom):

-> Run only one pytest parametrization when generating optest #108936

Richard, I'm curious to see what you think of this. I'm trying to use optest on the torchvision test suite, and after hacking up pytest support in #108929 I noticed that this was 5x'ing the test time... for no good reason.

torchvision nms tests before optests: 60 passed, 4 skipped, 1206 deselected in 11.47s
after optests: 300 passed, 20 skipped, 1206 deselected in 49.85s

It's no good reason because torchvision has parametrized the tests to get a spread of various random generation, but for checking schema or fake tensor, we don't actually need to test for different values.

This PR hacks up the codegen to replace pytest parametrize markers so that, instead of sampling many values, we sample only one value if you mark it with opcheck_only_one. There's a carveout for device parametrization, where we always run all those variants.

With this PR:

reduced optests: 88 passed, 4 skipped, 1206 deselected in 13.89s

Companion torchvision PR which uses this at pytorch/vision#7961

Signed-off-by: Edward Z. Yang ezyang@meta.com

Signed-off-by: Edward Z. Yang <ezyang@meta.com> [ghstack-poisoned]

pytorch-bot · 2023-09-09T03:38:02Z

🔗 Helpful Links

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

📄 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.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 045f8c9 with merge base 54dd65f ():

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

linux-focal-cuda12.1-py3.10-gcc9-sm86 / test (default, 3, 5, linux.g5.4xlarge.nvidia.gpu, unstable) (gh)

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

…optest" Richard, I'm curious to see what you think of this. I'm trying to use optest on the torchvision test suite, and after hacking up pytest support in #108929 I noticed that this was 5x'ing the test time... for no good reason. * torchvision nms tests before optests: 60 passed, 4 skipped, 1206 deselected in 11.47s * after optests: 300 passed, 20 skipped, 1206 deselected in 49.85s It's no good reason because torchvision has parametrized the tests to get a spread of various random generation, but for checking schema or fake tensor, we don't actually need to test for different values. This PR hacks up the codegen to replace pytest parametrize markers so that, instead of sampling many values, by default we only sample the first value. There are more bells and whistles we could add; for example, I could add an extra custom pytest marker to let you say "no no, please run each parametrization in the optest". I could also not do this. With this PR: * reduced optests: 88 passed, 4 skipped, 1206 deselected in 13.89s Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]

Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 97b2a48 Pull Request resolved: #108936

zou3519 · 2023-09-11T14:45:16Z

torch/testing/_internal/optests/generate_tests.py

+        if pytestmark := new_method.__dict__.get("pytestmark"):
+            import pytest
+
+            new_pytestmark = []
+            for mark in pytestmark:
+                if isinstance(mark, pytest.Mark) and mark.name == "parametrize":
+                    argnames, argvalues = mark.args
+                    assert not mark.kwargs, "NYI"
+                    new_pytestmark.append(
+                        pytest.mark.parametrize(argnames, (next(iter(argvalues)),))
+                    )
+                else:
+                    new_pytestmark.append(mark)
+            new_method.__dict__["pytestmark"] = new_pytestmark
+


I've thought about this before, especially because the internal tests also use some parametrization that make the tests go by slower. It kind of depends on how the parametrization is used:

if it is just to generate different random values, then yes, one parametrization is sufficient.

if it is to generate different options in your operator (e.g. BatchNorm with training=True/False), then it is important that we test all of them (BatchNorm mutates in one path but not the other)

Since generate_opcheck_tests is meant to be a "slow but runs everything check, because the user didn't want to write OpInfos (and we don't have an OpInfo V2 API)", I think I'd prefer the default to be "we run all parameterizations", but if the user understands what their test is doing and how it exercises their operator, they should be able to control the behavior to run just a single parametrization, perhaps by adding a decorator on the original test. Though, this does run the risk that the user copy-pastes tests with this decorator when writing new tests. WDYT?

This is fine by me. I'll need to figure out a mechanism to add extra annotations to parametrize though.

…optest" Richard, I'm curious to see what you think of this. I'm trying to use optest on the torchvision test suite, and after hacking up pytest support in #108929 I noticed that this was 5x'ing the test time... for no good reason. * torchvision nms tests before optests: 60 passed, 4 skipped, 1206 deselected in 11.47s * after optests: 300 passed, 20 skipped, 1206 deselected in 49.85s It's no good reason because torchvision has parametrized the tests to get a spread of various random generation, but for checking schema or fake tensor, we don't actually need to test for different values. This PR hacks up the codegen to replace pytest parametrize markers so that, instead of sampling many values, by default we only sample the first value. There are more bells and whistles we could add; for example, I could add an extra custom pytest marker to let you say "no no, please run each parametrization in the optest". I could also not do this. With this PR: * reduced optests: 88 passed, 4 skipped, 1206 deselected in 13.89s Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]

Richard, I'm curious to see what you think of this. I'm trying to use optest on the torchvision test suite, and after hacking up pytest support in #108929 I noticed that this was 5x'ing the test time... for no good reason. * torchvision nms tests before optests: 60 passed, 4 skipped, 1206 deselected in 11.47s * after optests: 300 passed, 20 skipped, 1206 deselected in 49.85s It's no good reason because torchvision has parametrized the tests to get a spread of various random generation, but for checking schema or fake tensor, we don't actually need to test for different values. This PR hacks up the codegen to replace pytest parametrize markers so that, instead of sampling many values, by default we only sample the first value. There are more bells and whistles we could add; for example, I could add an extra custom pytest marker to let you say "no no, please run each parametrization in the optest". I could also not do this. With this PR: * reduced optests: 88 passed, 4 skipped, 1206 deselected in 13.89s Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]

Richard, I'm curious to see what you think of this. I'm trying to use optest on the torchvision test suite, and after hacking up pytest support in #108929 I noticed that this was 5x'ing the test time... for no good reason. * torchvision nms tests before optests: 60 passed, 4 skipped, 1206 deselected in 11.47s * after optests: 300 passed, 20 skipped, 1206 deselected in 49.85s It's no good reason because torchvision has parametrized the tests to get a spread of various random generation, but for checking schema or fake tensor, we don't actually need to test for different values. This PR hacks up the codegen to replace pytest parametrize markers so that, instead of sampling many values, we sample only one value if you mark it with `opcheck_only_one`. There's a carveout for device parametrization, where we always run all those variants. With this PR: * reduced optests: 88 passed, 4 skipped, 1206 deselected in 13.89s Companion torchvision PR which uses this at pytorch/vision#7961 Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]

Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 2e89383 Pull Request resolved: #108936

zou3519 · 2023-09-14T13:50:15Z

torch/testing/_internal/optests/generate_tests.py

+        if pytestmark := new_method.__dict__.get("pytestmark"):
+            import pytest


Can you include a comment somewhere on how to actually mark a test with opcheck_only_one, for those who are unfamiliar with pytest?

ezyang · 2023-09-14T14:53:13Z

@pytorchbot merge

pytorchmergebot · 2023-09-14T14:54:52Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

[RFC] Run only one pytest parametrization when generating optest

a1f5ca8

Signed-off-by: Edward Z. Yang <ezyang@meta.com> [ghstack-poisoned]

github-actions bot requested review from SherlockNoMad, albanD, antoniojkim, bdhirsh, miladm, voznesenskym and wconstab September 9, 2023 03:38

ezyang requested a review from zou3519 September 9, 2023 03:41

ezyang added a commit that referenced this pull request Sep 10, 2023

[RFC] Run only one pytest parametrization when generating optest

57f5082

Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 97b2a48 Pull Request resolved: #108936

ezyang mentioned this pull request Sep 10, 2023

Meta implementation for nms pytorch/vision#7944

Merged

zou3519 reviewed Sep 11, 2023

View reviewed changes

ezyang changed the title ~~[RFC] Run only one pytest parametrization when generating optest~~ Run only one pytest parametrization when generating optest Sep 13, 2023

ezyang requested a review from zou3519 September 13, 2023 21:38

ezyang mentioned this pull request Sep 13, 2023

Add opcheck testing for nms pytorch/vision#7961

Merged

albanD removed their request for review September 13, 2023 22:05

ezyang added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Sep 14, 2023

ezyang added a commit that referenced this pull request Sep 14, 2023

[RFC] Run only one pytest parametrization when generating optest

343c6a6

Signed-off-by: Edward Z. Yang <ezyangmeta.com> ghstack-source-id: 2e89383 Pull Request resolved: #108936

zou3519 reviewed Sep 14, 2023

View reviewed changes

zou3519 approved these changes Sep 14, 2023

View reviewed changes

pytorchmergebot added the merging label Sep 14, 2023

pytorchmergebot added Merged and removed merging labels Sep 14, 2023

pytorchmergebot closed this in 7f1f5af Sep 14, 2023

facebook-github-bot deleted the gh/ezyang/2335/head branch September 18, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run only one pytest parametrization when generating optest#108936

Run only one pytest parametrization when generating optest#108936
ezyang wants to merge 5 commits intogh/ezyang/2335/basefrom
gh/ezyang/2335/head

ezyang commented Sep 9, 2023 •

edited

Loading

Uh oh!

pytorch-bot bot commented Sep 9, 2023 •

edited

Loading

Uh oh!

zou3519 Sep 11, 2023 •

edited

Loading

Uh oh!

ezyang Sep 11, 2023

Uh oh!

zou3519 Sep 14, 2023 •

edited

Loading

Uh oh!

ezyang commented Sep 14, 2023

Uh oh!

pytorchmergebot commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if pytestmark := new_method.__dict__.get("pytestmark"):
		import pytest

Conversation

ezyang commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 9, 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/108936

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

zou3519 Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

zou3519 Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Sep 14, 2023

Uh oh!

pytorchmergebot commented Sep 14, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ezyang commented Sep 9, 2023 •

edited

Loading

pytorch-bot bot commented Sep 9, 2023 •

edited

Loading

zou3519 Sep 11, 2023 •

edited

Loading

zou3519 Sep 14, 2023 •

edited

Loading