pytest warnings due to deprecation of pytest.warns(None)

[avm19]

Pytest issues warnings about using pytest.warns(None) context manager, where the None part is now being deprecated.

For example,
>>> pytest ./sklearn/tests/test_naive_bayes.py
results in 79 passed, 4 warnings. After commenting out --disable-pytest-warnings key in [tool:pytest] section of setup.cfg, I could see that some of the warnings are:

PytestRemovedIn8Warning: Passing None has been deprecated.
See https://docs.pytest.org/en/latest/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests for alternatives in common use cases.

Apparently, pytest.warns(None) is deprecated for being widely misunderstood. I think that while sklearn's tests do not suffer from this misunderstanding, they might be affected in the future if and when pytest raises an error instead of a deprecation warning. (I couldn't figure out if this is planned for pytest's future releases, but in any case, avoiding the deprecation and reducing the warnings count won't harm.)

I found this message , which is relevant to how pytest.warns(None) is used in scikit-learn tests, and how this could be changed. I opened this issue to share this information and quote the message, as it may save someone time in the future.

The tests were of the form:

with pytest.warns(None) as record:
    <do some stuff>
    <check record for warnings, and possibly raise an error>

The tests were not assuming that the pytest context code would raise an error if a warning occurred. Instead, these tests were doing their own error checking based on record. So they were really using pytest.warns(None) exactly like warnings.catch_warnings(record=True). (There is a pull request to make the change to catch_warnings.)

[thomasjpfan]

Thanks for opening the issue! Since pytest.warns(None) is deprecated, It makes sense to update our tests to use warnings.catch_warnings(record=True).

To be specific:

    with pytest.warns(None) as record:
        ...

can be directly replaced with:

    with warnings.catch_warnings(record=True) as record:
        ...

[cmarmo]

Here is a list of files containing pytest.warns(None).
For those willing to address this issue, please fix only one file per pull request.

• sklearn/cluster/tests/test_affinity_propagation.py: TST Convert warnings into errors in test_affinity_propgation #22824
• sklearn/cluster/tests/test_feature_agglomeration.py: TST Replaces pytest.warns(None) in test_feature_agglomeration #22871
• sklearn/cluster/tests/test_optics.py:
• sklearn/cross_decomposition/tests/test_pls.py:
• sklearn/decomposition/tests/test_dict_learning.py: TST Replaces pytest.warns(None) in test_dict_learning #22845
• sklearn/decomposition/tests/test_fastica.py: TST Replaces pytest.warns(None) in test_fastica #22846
• sklearn/decomposition/tests/test_kernel_pca.py: TST Replaces pytest.warns(None) in test_kernel_pca #22872
• sklearn/decomposition/tests/test_pca.py: TST Replaces pytest.warns(None) in test_pca #22873
• sklearn/ensemble/tests/test_iforest.py: TST Removes pytest.warns(None) in test_iforest #22874
• sklearn/ensemble/tests/test_voting.py: TST Replaces pytest.warns(None) in test_voting #22949
• sklearn/feature_extraction/tests/test_text.py: TST Replace pytest.warns(None) in feature_extraction/tests/test_text.py #22962
• sklearn/feature_selection/tests/test_feature_select.py:
• sklearn/feature_selection/tests/test_from_model.py:
• sklearn/gaussian_process/tests/test_gpc.py: TST Replace pytest.warns(None) in test_gpc.py #22960
• sklearn/gaussian_process/tests/test_gpr.py: TST Replace pytest.warns(None) in test_gpr.py #22959
• sklearn/impute/tests/test_impute.py:
• sklearn/inspection/_plot/tests/test_plot_partial_dependence.py:
• sklearn/linear_model/tests/test_base.py: TST replace pytest.warns(None) in linear_model test_base.py #22876
• sklearn/linear_model/tests/test_coordinate_descent.py: TST replace pytest.warns(None) in test_coordinate_descent.py #22878
• sklearn/linear_model/tests/test_least_angle.py: TST Replace pytest.warns(None) in test_least_angle.py #22889
• sklearn/linear_model/tests/test_logistic.py: TST remove pytest.warns(None) in test_logistic.py #22877
• sklearn/linear_model/tests/test_omp.py: TST Replace pytest.warns(None) in test_omp.py #22886
• sklearn/linear_model/tests/test_ridge.py: MNT Replace pytest.warns(None) in test_ridge.py #22917
• sklearn/manifold/tests/test_t_sne.py:
• sklearn/metrics/cluster/tests/test_supervised.py: TST replace pytest.warns(None) in metrics/cluster/test_supervised.py #22955
• sklearn/metrics/cluster/tests/test_unsupervised.py: TST replace pytest.warns(None) in metrics/cluster/test_unsupervised.py #22956
• sklearn/metrics/tests/test_classification.py: TST replace pytest.warns(None) in metrics/test_classification.py #22888
• sklearn/metrics/tests/test_pairwise.py: TST replace pytest.warns(None) in metrics/tests/test_pairwise.py #22957
• sklearn/preprocessing/tests/test_common.py: TST replace pytest.warns(None) in preprocessing/test_data.py #22938
• sklearn/preprocessing/tests/test_data.py:
• sklearn/preprocessing/tests/test_function_transformer.py:
• sklearn/semi_supervised/tests/test_label_propagation.py:
• sklearn/svm/tests/test_svm.py:
• sklearn/tests/test_base.py:
• sklearn/tests/test_common.py:
• sklearn/tests/test_discriminant_analysis.py: TST Replace pytest.warns(None) in test_discriminant_analysis.py #23057
• sklearn/tests/test_isotonic.py:
• sklearn/tests/test_naive_bayes.py:
• sklearn/tests/test_pipeline.py:
• sklearn/utils/tests/test_utils.py:
• sklearn/utils/tests/test_validation.py:

I guess solving this issue will also move forward #22396.

[ShanDeng123]

Hello - I'd like to help tackle this problem if that'd be alright? If someone else is already working on this please just let me know!

[avm19]

Hello @ShanDeng123
As far as I understand, all the items on cmarmo's list are independent and she asked to do 1 file per pull request.
I suppose, you can write here to reserve a bunch of them (e.g., sklearn/cluster/tests/*.*), then take the next bunch, and so on.
I am not working on this issue.

[avm19]

I am curious if the issue is more complex than Find-Replace... And if pytest.warns(None) in the existing code was always used and understood correctly.

[thomasjpfan]

For this specific case, I am okay with one PR that does a "Find and Replace" replacing all the with pytest.warns(None) as record: with with warnings.catch_warnings(record=True) as record:.

I think it's better to do one PR per file. See #22572 (comment) for details.

[ShanDeng123]

@avm19
Sounds good - I can look to do the first 10 in the list up until the last file under the ensemble folder, and can continue working on the others afterwards if nobody else hops on.

@thomasjpfan
I can look to merge updates for multiple files in a single PR as I go! I do want to make sure that I'm following all of the proper guidelines/precautions though as I am still a bit new to open source work. Would hate to cause any unnecessary issues! 😅

That being said - would any of you guys know if this would necessitate an update to the changelog document? And if so, would the note be a bullet under the main Changelog header?

[thomasjpfan]

It is not necessary to update the changelog, since it is not a public facing change.

[thomasjpfan]

Coming from #22822 (comment) I think it's better to do this one per file.

When we write the following to test for no warnings:

with pytest.warns(None) as record:
    ...
assert not [w.message for w in record]

I think the better solution is to replace that with:

with warnings.catch_warnings():
    warnings.simplefilter("error", EXPECTED_WARNING)
    ...

where EXPECTED_WARNING is the warning we expect to not happen. Since this is a non-trival change, I think it's better a one PR per file.

If we do not know the expected warning then we can write:

with warnings.catch_warnings():
    warnings.simplefilter("error")
    ...

(But I prefer to have the expected warning)

[ShanDeng123]

Will do - I'll go back over the files with 1 PR each, and look to add the extra detail when applicable 👍

[thomasjpfan]

I opened #22824 as a demonstration of what the idea looks like. Let's wait to see what other maintainers think before opening more.

[thomasjpfan]

@ShanDeng123 #22824 has been merged. You may open a PR per file to address the other ones following the idea in #22572 (comment) .

[danifernandes-hub]

Working on sklearn/metrics/tests/test_classification.py

[ShanDeng123]

@thomasjpfan
Is there an intuitive way to know which error to not expect? In some cases, the error is a bit easy to see due to the function name/surrounding statements, but I'm a bit unsure on most others.

Would it be fair to say that pytest.warns(None) generally checks for UserWarning unless the surrounding statements suggest otherwise?

For example, in test_feature_agglomeration, I'd imagine that the expected warning is a UserWarning for the code below
with pytest.warns(None) as record: agglo_mean.fit(X) assert not [w.message for w in record]

but I'd be lying if I said I had any confidence in that answer.

[thomasjpfan]

Is there an intuitive way to know which error to not expect? In some cases, the error is a bit easy to see due to the function name/surrounding statements, but I'm a bit unsure on most others.

The only way to check is through git blame. For example, test_feature_agglomeration is a DeprecationWarning: #11537

For that specific case, there is no deprecation warning anymore in the code, so I would remove the "no warnings" check all together. The PR should state the reason for removing the "no warning" check.

[nik1097]

Hey @thomasjpfan I'm a first-time contributor, could you please review my changes? Thank you so much.

[MegaGonz]

i can work on sklearn/tests/test_naive_bayes.py

[nik1097]

i can work on sklearn/tests/test_naive_bayes.py
I'm afraid #23066 already fixes this issue @adamgonzo

[cmarmo]

Reopening as some tests still need to be modified.

[nik1097]

@jeremiedbb this should be the last file having the outdated syntax, I've opened another PR to fix this issue. Could you please review it?

[cmarmo]

Also #22949 is still open

[jeremiedbb]

sklearn/inspection/_plot/tests/test_plot_partial_dependence.py and sklearn/tests/test_pipeline.py still have some instances of the deprecated pattern