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

use pytest markers instead of custom solution for prototype transforms functional tests #6653

Merged
merged 6 commits into from
Oct 5, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Sep 27, 2022

In #6626 we added support for skipping tests through the KernelInfo and DispatcherInfo. It was a custom solution that relied on some fixture magic

@pytest.fixture(autouse=True)
def maybe_skip(request):
# In case the test uses no parametrization or fixtures, the `callspec` attribute does not exist
try:
callspec = request.node.callspec
except AttributeError:
return

Plus, there were other downsides:

  1. We were only able to skip a test, but not xfail for example.
  2. The test to skip was only identified by its name, but multiple classes in the test module could have the same test function names, leading to clashes.

This PR changes this by replacing our custom skip logic with the ability to add pytest.mark's to a specific info / args_kwargs combination.

Comment on lines 23 to 30
test_marks: Sequence[TestMark] = dataclasses.field(default_factory=list)
_test_marks_map: Dict[str, List[TestMark]] = dataclasses.field(default=None, init=False)

def __post_init__(self):
self._skips_map = {skip.test_name: skip for skip in self.skips}
test_marks_map = defaultdict(list)
for test_mark in self.test_marks:
test_marks_map[test_mark.test_id].append(test_mark)
self._test_marks_map = dict(test_marks_map)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the same on KernelInfo and here on the DispatcherInfo. I'm going to factor this out into a common base class in a follow-up PR since this PR is already quite large.


import numpy as np
import pytest
import torch.testing
import torchvision.ops
import torchvision.prototype.transforms.functional as F

from _pytest.mark.structures import MarkDecorator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only needed for annotating the attributes of the data classes below. I'll send a follow-up PR removing them, since the upside of @dataclasses.dataclass is outweighed by the downside of importing from a private namespace.

@@ -17,11 +20,24 @@
__all__ = ["KernelInfo", "KERNEL_INFOS"]


TestID = Tuple[Optional[str], str]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above.

@pmeier pmeier requested a review from vfdev-5 September 27, 2022 14:32
@pmeier pmeier marked this pull request as ready for review September 27, 2022 14:32
pmeier added 3 commits October 5, 2022 15:20
Conflicts:
	test/prototype_transforms_dispatcher_infos.py
	test/prototype_transforms_kernel_infos.py
	test/test_prototype_transforms_functional.py
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @pmeier

@pmeier pmeier merged commit 46eae18 into pytorch:main Oct 5, 2022
@pmeier pmeier deleted the functional-test-marks branch October 5, 2022 14:46
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
…transforms functional tests (#6653)

Summary:
* use pytest markers instead of custom solution for prototype transforms functional tests

* cleanup

* cleanup

* trigger CI

Reviewed By: datumbox

Differential Revision: D40138744

fbshipit-source-id: b2a51d258eaacd348c3b2004591455ed88aac927
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants