Suppress Deprecation Warning

[krshrimali]

This PR is in draft mode. Here is what it does so far:

• Implements a decorator (suppress_depr_warnings) which captures and ignores DeprecationWarning/UserWarning.
• Slap the decorator (suppress_depr_warnings) on tests present in tests/integration/loss/* and tests/integration/ops/test_comparison.py files.
• Force pytest to fail on warnings (pytest -Werror)

Testing with: tox -e tests-integration on the local system:

Click to see skips/failures

================================================================================== short test summary info ===================================================================================
SKIPPED [1] tests/integration/test_optim.py:48: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/test_optim.py:64: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/test_optim.py:86: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/test_optim.py:102: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/test_optim.py:141: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/test_optim.py:166: Test errors on Python 3.8 only. This is most likely caused by the test itself rather than the code it should test.
SKIPPED [1] tests/integration/core/test_objects.py:223: CUDA is not available.
SKIPPED [1] tests/integration/enc/models/test_alexnet.py:18: Test is flaky and --run-flaky was not given.
SKIPPED [1] tests/integration/enc/models/test_vgg.py:75: Test is flaky and --run-flaky was not given.
FAILED tests/integration/core/test_math.py::TestCosineSimilarity::test_shape - UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress...
FAILED tests/integration/data/test_collections.py::TestDownloadableImage::test_generate_file - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/data/test_licenses.py::test_licenses_repr_smoke - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_alexnet.py::TestAlexNetMultiLayerEncoder::test_smoke - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_alexnet.py::TestAlexNetMultiLayerEncoder::test_state_dict_url - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_utils.py::TestModelMultiLayerEncoder::test_internal_preprocessing - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_utils.py::TestModelMultiLayerEncoder::test_repr - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_vgg.py::TestVGGMultiLayerEncoder::test_smoke - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/enc/models/test_vgg.py::TestVGGMultiLayerEncoder::test_state_dict_url - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/image/test_io.py::test_show_image_smoke - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/loss/test_functional.py::TestMRFLoss::test_main - UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress thi...
FAILED tests/integration/ops/test_container.py::TestOperatorContainer::test_main - UserWarning: The class pystiche.ops.OperatorContainer is deprecated since pystiche==1.0 and will be remo...
FAILED tests/integration/ops/test_container.py::TestOperatorContainer::test_get_image_or_guide - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/ops/test_container.py::TestOperatorContainer::test_set_image_or_guide - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/ops/test_container.py::TestOperatorContainer::test_call - UserWarning: The class pystiche.ops.OperatorContainer is deprecated since pystiche==1.0 and will be remo...
FAILED tests/integration/ops/test_container.py::TestMultiLayerEncodingOperator::test_main - UserWarning: The class pystiche.ops.MultiLayerEncodingOperator is deprecated since pystiche==1....
FAILED tests/integration/ops/test_container.py::TestMultiRegionOperator::test_set_regional_image_or_guide - pytest.PytestDeprecationWarning: A private pytest class or function was used.
FAILED tests/integration/ops/test_regularization.py::test_TotalVariationOperator_call - UserWarning: The class pystiche.ops.TotalVariationOperator is deprecated since pystiche==1.0 and wi...
FAILED tests/integration/ops/test_regularization.py::test_ValueRangeOperator_call - UserWarning: The class pystiche.ops.ValueRangeOperator is deprecated since pystiche==1.0 and will be re...
FAILED tests/integration/pyramid/test_pyramid.py::TestImagePyramid::test_iter_resize - pytest.PytestDeprecationWarning: A private pytest class or function was used.
========================================================================= 20 failed, 242 passed, 9 skipped in 24.58s =========================================================================

With tox -e tests-integration, there are 20 failures - 242 passes - 9 skips with this branch while on the upstream/main branch (after adding -Werror to tox.ini), there were 36 failures - 252 passes - 9 skips.

TODOs:

• Consider replacing the new decorator suppress_depr_warnings with existing decorator: suppress_warnings.
• Make sure only those warnings are ignored/suppressed which are coming from pystiche. (Since we explicitly use pystiche in the deprecation message, I'm thinking of checking that keyword in the message for this.)
• Is there a better way to slap this decorator on all the tests instead of adding for each test manually?

Note:

• This PR is in a heavy WIP mode. But it's also in heavy development mode, should be ready for review in a day.

See #511 for more context.

cc: @pmeier

[pmeier]

• Consider replacing the new decorator suppress_depr_warnings with existing decorator: suppress_warnings.

suppress_warnings is not a decorator, but rather a context manager. It is used for the same problem that we are trying to solve, but in our implementation rather than in the test suite. I agree, it would be good to marry the two. Maybe we can have the decorator approach only in tests.utils?

from pystiche.misc import (
    suppress_deprecation_warnings as _suppress_deprecation_warnings,
)


def suppress_deprecation_warning(test_fn):
    @functools.wraps(test_fn)
    def wrapper(*args, **kwargs):
        with _suppress_deprecation_warnings():
            return test_fn(*args, **kwargs)

    return wrapper

• Is there a better way to slap this decorator on all the tests instead of adding for each test manually?

Even if there is, I wouldn't go for that. I think it is ok to be verbose here. Especially since we only need to do that once if something is deprecated. Afterwards the test should not change or there shouldn't be any new tests that check deprecated stuff.

[krshrimali]

• Consider replacing the new decorator suppress_depr_warnings with existing decorator: suppress_warnings.

suppress_warnings is not a decorator, but rather a context manager. It is used for the same problem that we are trying to solve, but in our implementation rather than in the test suite. I agree, it would be good to marry the two. Maybe we can have the decorator approach only in tests.utils?

from pystiche.misc import (
    suppress_deprecation_warnings as _suppress_deprecation_warnings,
)


def suppress_deprecation_warning(test_fn):
    @functools.wraps(test_fn)
    def wrapper(*args, **kwargs):
        with _suppress_deprecation_warnings():
            return test_fn(*args, **kwargs)

    return wrapper

I opted with what you suggested since I don't have a strong opinion on this (both look good to me). This should be fixed in the recent commit. I took the freedom to remove suppress_warnings which isn't used anywhere in the code, please let me know if this should stay in case you plan to use it in the future.

• Is there a better way to slap this decorator on all the tests instead of adding for each test manually?

Even if there is, I wouldn't go for that. I think it is ok to be verbose here. Especially since we only need to do that once if something is deprecated. Afterwards the test should not change or there shouldn't be any new tests that check deprecated stuff.

Right, for now, I added the decorator on the tests where this warning was raised. Does that sound good? Or should it go for all the tests?

This PR should be ready for review, please take a look whenever you find the time. @pmeier

Thanks!

[pmeier]

Hey @krshrimali, thanks for the PR! I have a few comments inline. Apart from them two more general ones:

I took the freedom to remove suppress_warnings which isn't used anywhere in the code, please let me know if this should stay in case you plan to use it in the future.

As of pystiche==1.0.0 we have adopted semantic versioning. That means, we cannot introduce BC breaking changes on a non-major release (of course we can, but then we wouldn't follow the scheme anymore). The next release should be 1.1.0 so just a minor one. Thus, we cannot remove suppress_warnings here. Still we can mark it as deprecated, so it can be removed with the 2.0.0 release.

Right, for now, I added the decorator on the tests where this warning was raised. Does that sound good? Or should it go for all the tests?

In this PR we should cover all tests where we actually test deprecated stuff. If there are failures in other tests, let's ignore them for now and fix this in follow-up PRs. To do that, I suggest we add the filterwarnings option to the pytest.ini. Example

[krshrimali]

Minor update: Thanks @pmeier for the review. Just a little caught up with PyTorch during the weekdays, will get back on this one with updates by the weekend or earlier. Hope that's fine.

[pmeier]

@krshrimali No worries, I'm happy for every contribution!

[krshrimali]

As of pystiche==1.0.0 we have adopted semantic versioning. That means, we cannot introduce BC breaking changes on a non-major release (of course we can, but then we wouldn't follow the scheme anymore). The next release should be 1.1.0 so just a minor one. Thus, we cannot remove suppress_warnings here. Still we can mark it as deprecated, so it can be removed with the 2.0.0 release.

Sure, I've brought it back to life but will have to mark it deprecated. This should be done in the next commit (after your inputs on existing implementation).

In this PR we should cover all tests where we actually test deprecated stuff. If there are failures in other tests, let's ignore them for now and fix this in follow-up PRs. To do that, I suggest we add the filterwarnings option to the pytest.ini. Example

Unfortunately, using filterwarnings and -Werror together doesn't work. The warnings (as failures) are still raised. I tried with the following pytest.ini file:

[pytest]

markers =
  large_download
  slow
  slow_if_cuda_not_available
  flaky

addopts =
  -ra
  -Wignore::UserWarning
  -Wignore::pytest.PytestDeprecationWarning
  -Werror
  --tb=short

Also as:

[pytest]

markers =
  large_download
  slow
  slow_if_cuda_not_available
  flaky

filterwarnings=
  ignore::UserWarning
  ignore::pytest.PytestDeprecationWarning

addopts =
  -ra
  -Werror
  --tb=short

Do you have any suggestions on this?

Thanks for the patience, @pmeier!

[pmeier]

Hey @krshrimali, thanks a lot for the new commits.

Do you have any suggestions on this?

I meant what you suggested in #566 (comment) so we are almost good to go. Only minor suggestions inline.

Apart from them, I see one bigger issue: it seems our new decorator does not play nice with pytest-subtests. We might encounter errors that read E pytest.PytestDeprecationWarning: A private pytest class or function was used. I wanted to remove subtests for a while now and replace it with pytest.mark.parametrize. We probably need to do that before we merge this PR. Thoughts?

[krshrimali]

Hey @krshrimali, thanks a lot for the new commits.

Do you have any suggestions on this?

I meant what you suggested in #566 (comment) so we are almost good to go. Only minor suggestions inline.

Apart from them, I see one bigger issue: it seems our new decorator does not play nice with pytest-subtests. We might encounter errors that read E pytest.PytestDeprecationWarning: A private pytest class or function was used. I wanted to remove subtests for a while now and replace it with pytest.mark.parametrize. We probably need to do that before we merge this PR. Thoughts?

Really good suggestions and questions, @pmeier - acknowledging that I've seen these comments. The suggestion looks good to fix #571, looks like @ankitaS11 is interested so this shouldn't be too late. :) I'll also send in the fixes from your comments tomorrow.

Thank you for taking a look again!

[pmeier]

Hey @krshrimali, with #571 now fixed, could you update your branch? After that the error messages should be a lot more clear.

[krshrimali]

Hi, @pmeier - thanks for the update on #571. I've resolved the merge conflicts, and have updated my branch with the new changes. A few things to note:

1. From previous reviews, where it's possible and trivial to eliminate the deprecation warning, I've done that - for example: get_preprocessor was renamed to preprocessing, ops.<Class> was renamed to loss.<Class>.
2. There are a few failures for the default value of batched_input, please see below. I can try to fix this but wanted to confirm before I do. Should this be fixed in this PR or kept for later?
3. There are now 4 failures, 366 passes, 16 skips for integration tests.

Click to view failures

=================================================================================================== FAILURES ===================================================================================================
_______________________________________________________________________________________ TestCosineSimilarity.test_shape ________________________________________________________________________________________
tests/integration/core/test_math.py:87: in test_shape
    assert pystiche.cosine_similarity(input, target).size() == (2, 3, 3)
pystiche/core/_math.py:108: in cosine_similarity
    warnings.warn(msg, UserWarning)
E   UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress this warning, pass the wanted behavior explicitly.
____________________________________________________________________________________________ TestMRFLoss.test_call _____________________________________________________________________________________________
tests/integration/loss/test_comparison.py:60: in test_call
    desired = F.mrf_loss(
pystiche/loss/functional.py:44: in mrf_loss
    warnings.warn(msg, UserWarning)
E   UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress this warning, pass the wanted behavior explicitly.
_________________________________________________________________________________________ TestMRFLoss.test_call_guided _________________________________________________________________________________________
tests/integration/loss/test_comparison.py:88: in test_call_guided
    desired = F.mrf_loss(
pystiche/loss/functional.py:44: in mrf_loss
    warnings.warn(msg, UserWarning)
E   UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress this warning, pass the wanted behavior explicitly.
____________________________________________________________________________________________ TestMRFLoss.test_main _____________________________________________________________________________________________
tests/integration/loss/test_functional.py:19: in test_main
    actual = F.mrf_loss(input, target)
pystiche/loss/functional.py:44: in mrf_loss
    warnings.warn(msg, UserWarning)
E   UserWarning: The default value of batched_input has changed from False to True in version 1.0.0. To suppress this warning, pass the wanted behavior explicitly.

[codecov]

Codecov Report

Merging #566 (52b2683) into main (91afacb) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #566   +/-   ##
=====================================
  Coverage   90.8%   90.8%           
=====================================
  Files         48      48           
  Lines       2574    2574           
=====================================
  Hits        2339    2339           
  Misses       235     235           

Flag	Coverage Δ
cli	55.3% <ø> (ø)
integration	82.5% <ø> (+<0.1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91afacb...52b2683. Read the comment docs.

[pmeier]

@krshrimali For the remaining failures, we only needed to explicitly set batched_input. I've added this (and fixed one CLI test), and now the PR looks good. Thanks a lot!