Fix fft slow tests

[zasdfgbnm]

The failure is:

______________________________________________________________________________________________________ TestCommonCUDA.test_variant_consistency_jit_fft_rfft_cuda_float64 _______________________________________________________________________________________________________
../.local/lib/python3.9/site-packages/torch/testing/_internal/common_utils.py:889: in wrapper
    method(*args, **kwargs)
../.local/lib/python3.9/site-packages/torch/testing/_internal/common_utils.py:889: in wrapper
    method(*args, **kwargs)
../.local/lib/python3.9/site-packages/torch/testing/_internal/common_device_type.py:267: in instantiated_test
    if op is not None and op.should_skip(generic_cls.__name__, name,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <torch.testing._internal.common_methods_invocations.SpectralFuncInfo object at 0x7f7375f9b550>, cls_name = 'TestCommon', test_name = 'test_variant_consistency_jit', device_type = 'cuda', dtype = torch.float64

    def should_skip(self, cls_name, test_name, device_type, dtype):
>       for si in self.skips:
E       TypeError: 'NoneType' object is not iterable

../.local/lib/python3.9/site-packages/torch/testing/_internal/common_methods_invocations.py:186: TypeError

[facebook-github-bot]

💊 CI failures summary and remediations

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

---

• 1/1 failures possibly* introduced in this PR
  • 1/1 non-CircleCI failure(s)

---

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.

This comment has been revised 4 times.

[mruberry]

Would you elaborate on what's going on here, @zasdfgbnm?

cc @peterbell10

It seems like this line should be preventing skips from becoming None?

[zasdfgbnm]

When I run with PYTORCH_TEST_WITH_SLOW=1, the test_ops.py is failing with the above error for all these fft ops. It seems that nobody is running slow tests, so nobody found this failure. And this failure is because in the __init__ function, it should replace the skips=None with skips=[], regardless of the TEST_WITH_SLOW. But currently, it is only doing so if not TEST_WITH_SLOW, which is wrong.

To reproduce the failure, you can do

export PYTORCH_TEST_WITH_SLOW=1
python test/test_ops.py -v

[mruberry]

Nice fix, thanks @zasdfgbnm.

[facebook-github-bot]

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

[facebook-github-bot]

@mruberry merged this pull request in 4a3a378.

[malfet]

It seems that nobody is running slow tests, so nobody found this failure.

Hmm, slow test should be executed by slow_test config, see pytorch_linux_xenial_cuda10_2_cudnn7_py3_slow_test for example, though I wonder why failures is not reported back

[peterbell10]

Ah, I think I see this issue. The CI is running with PYTORCH_TEST_SKIP_FAST but the test was never marked as slow, so it gets skipped on every CI run. I think OpInfo needs a way to directly add the @slowTest decorator to a particular test.

@mruberry I'm thinking either extend the SkipInfo class to handle slow tests specifically. Or, introduce a mechanism to add a set of decorators to a particular test. That could either replace SkipInfo, or coexist with it. What do you think?

[mruberry]

I could see either approach work. If SkipInfo can be extended in a natural way, like there's just a active_if=no_slow_tests option, that sounds good. If there are other scenarios where we need to apply decorators to specific tests then I would create a separate mechanism, although I cannot think of any at the moment.

[peterbell10]

I'm leaning towards creating a DectorateInfo class that has a decorators argument, like the main OpInfo argument. The decorators get added while instantiating the test, just before where the test is currently wrapped in a functor that queries op.should_skip(...). Then optionally, SkipInfo can be reimplemented as a DecorateInfo that sets decorators = skipIf(true).

Adding the slowTest decorator requires this same logic anyway, so it seems reasonable to provide a generic interface in case it's needed elsewhere.

If that sounds alright, I'll create a PR.

[mruberry]

Sounds good.