CI Use environment variable to turn warnings into errors in tests and doc build #28372

lesteve · 2024-02-06T14:43:29Z

Follow-up of #28348 (comment) to make it easier to run locally with the same "warnings as errors" setup as in the CI:

SKLEARN_WARNINGS_AS_ERRORS=1 pytest sklearn

Warning filters have grown a bit and will grow a bit more once there is a Python 3.12 build in the CI (dateutil, joblib latest releases create warnings with Python 3.12). There is now a single place where warning filters is defined instead of having setup.cfg, test_script.sh (CI), and sklearn/conftest.py.

While I was at it I use the same function for "warnings as errors" in the doc build.

For now there is a hack with sys.warnoptions, see comments in sklearn/conftest.py, I looked for some time, but I haven't found anything better ...

cc @adrinjalali @ogrisel who put a ❤️ on my original comment.

github-actions · 2024-02-06T14:44:46Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: ca0e111. Link to the linter CI: here}

… renaming" This reverts commit be8b69c.

adrinjalali

Tested locally and it works 👍🏼

Other than not being sure about the hack, lgtm.

adrinjalali · 2024-02-06T17:40:02Z

sklearn/conftest.py

+        # https://github.com/pytest-dev/pytest/blob/404d31a942975c04d4a7d48e258260584930819f/src/_pytest/warnings.py#L45-L48
+        # This would overrides our warning filters modifications so we set
+        # sys.warnoptions to a non-empty value
+        sys.warnoptions = "hack-to-avoid-pytest-overriding-warning-filters"


🤦🏼

I would have hoped for having things in pyproject.toml or something instead of needing this hack.

I think one of the challenge is to be able to dynamically set the warning filters based on an environment variable, so you need something else on top of defining the warning filters inside a configuration file. Also I am not a big fan of warnings as errors by default, I feel there may be too many warnings that contributors bump into depending on their environment, and chasing these warnings is not worth it, maybe I am wrong about this ...

If there was not the issue that pandas Pyarrow DeprecationWarning starts with a newline and thus can not be ignored with -W, the hack would not be needed. With the right Pytest hook you can add -W arguments before the command line is parsed for example with pytest_load_initial_conftests https://docs.pytest.org/en/7.1.x/reference/reference.html?highlight=pytest_collection_modifyitems#std-hook-pytest_load_initial_conftests

An alternative would be to have:

pytest_load_initial_conftests to add -W arguments based on environment variable

special case for pandas Pyarrow DeprecationWarning using the right Pytest hook

I just found pytest-dev/pytest#3311 with a use case somewhat similar to ours, change pytest config value programatically but there does not seem to be a clean solution either.

Thinking about it a bit more, it is kind of nice that in this PR the warning filters are used both for the tests and the doc build, so a Pytest-specific thing may not be the best solution.

Only commandline python -W doesn't work with \n at the beginning of the message though. They work if loaded from config files: https://github.com/skops-dev/skops/pull/409/files

So we could have multiple config files and load from them based on some input arg or env variable.

They work if loaded from config files

Yes I know 😉 filterwarnings in config files accept regexes, -W does not.

So we could have multiple config files and load from them based on some input arg or env variable.

Indeed, something similar is mentioned in pytest-dev/pytest#3311 (comment). A bit hacky but maybe less hacky than alternatives.

also I am not a big fan of warnings as errors by default, I feel there may be too many warnings that contributors bump into depending on their environment, and chasing these warnings is not worth it

I agree when building and running test locally, but I think it's good to have it configured as errors on the CI.

adrinjalali · 2024-02-06T17:43:25Z

doc/computing/parallelism.rst

+- tests, for example by running `SKLEARN_WARNINGS_AS_ERRORS=1 pytest sklearn`.
+- documentation build, where it is turned on by default when you do `make
+  html`. You can do `SKLEARN_WARNINGS_AS_ERRORS=0 make html` if you want to
+  ignore warnings. Note that this checks that running examples don't produce
+  any warnings which is not the same as `SHPINXOPTS="-W"` that checks syntax
+  issues in the rst files.


we should be more clear what the default is.

Based on the current diff it's set to 1 when building the doc but 0 when running the tests.

I think that for the sake of consistency, it should be 0 everywhere but we should set the SKLEARN_WARNINGS_AS_ERRORS=1 explicitly when building the doc on the CI.

WDYT @adrinjalali @lesteve?

+1 for having SKLEARN_WARNINGS_AS_ERRORS=0 by default in the doc build as well. I guess one reason to have it set to 1/true by default in the doc build was that people have the same settings as the CI by default.

As I mentioned above, my main reason to have SKLEARN_WARNINGS_AS_ERRORS=0 by default is to avoid getting reports from contributors with warnings in some environments that are not worth fixing.

I don't mind. And I can have the env present in my session always, so my personal workflow would be fine anyway.

2 weeks ago, this error by default on local build was the source of confusion for an experience scikit-learn contributor because of a deprecation warning on Python 3.12 only.

Also use the same logic in doc/conf.py and tweak doc

lesteve · 2024-02-07T15:13:18Z

So my last non-empty commit, I have an alternative approach with a slightly different hack which was mentioned in pytest-dev/pytest#3311 (comment): this uses config._inicache["filterwarnings"] to programatically change the value of the pytest config filterwarnings.

The fact that it still works almost 6 years after the comment maybe is a sign that this is a hack that is not too brittle ...

This is maybe a bit less hacky than the previous attempt based on warnings.filterwarnings since at least this uses pytest functionality.

Full-disclosure:

I did not implement support for module and lineno in warning filters, since we don't need it right now and we will likely never need it in the foreseeable future.
I originally thought there was an easy way to generate a config file with modififed filterwarnings and programmatically modify the pytest args to add ["--config-file", my_new_config_file] but it seems way too complicated. You can only use pytest_load_initial_conftests for this, which is only possible for pytest setuptools plugins, for example see https://stackoverflow.com/a/73803971

lesteve · 2024-02-07T15:36:11Z

setup.cfg

@@ -21,9 +21,6 @@ addopts =
    # source folder.
    -p sklearn.tests.random_seed

-filterwarnings =


I don't think there was a reason for this anymore ... it seems like the CI agrees.

lesteve · 2024-02-07T16:06:04Z

The full doc build was passing in fb7fd6f, see doc build. I pushed too early to wait for the upload artifacts to pass ...

lesteve · 2024-02-07T20:49:00Z

The coverage drop is due to turn_warnings_into_errors which is used only in doc build. It could be actually tested if we insist.

thomasjpfan · 2024-02-08T04:56:53Z

sklearn/conftest.py

+        # This seems like the only way to programmatically change the config
+        # filterwarnings. This was suggested in
+        # https://github.com/pytest-dev/pytest/issues/3311#issuecomment-373177592
+        config._inicache["filterwarnings"] += get_pytest_filterwarning_str()


I think we can use public API:

Suggested change

# This seems like the only way to programmatically change the config

# filterwarnings. This was suggested in

# https://github.com/pytest-dev/pytest/issues/3311#issuecomment-373177592

config._inicache["filterwarnings"] += get_pytest_filterwarning_str()

for line in get_pytest_filterwarning_str():

config.addinivalue_line("filterwarnings", line)

Good point although it is kind of not encouraged in pytest-dev/pytest#3311 (comment)

while addinivalue_line exists, it feels it was added solely to expand the list of builtin markers, which seems like it could have been done in some other way.

OK I used addinivalue_line, at least it is public ...

lesteve · 2024-02-08T09:12:58Z

This is ready for a second round of review:

use config.addinivalue_line which is public and thus maybe less brittle than previous attempts
add tests
tweak documentation

The full doc build passes in b5dad1c

ogrisel

LGTM, thank you very much @lesteve. Centralizing this is a quality of life improvement :)

ogrisel · 2024-02-08T10:48:26Z

sklearn/utils/_testing.py

+        WarningInfo("error", category=DeprecationWarning),
+        WarningInfo("error", category=FutureWarning),
+        WarningInfo("error", category=VisibleDeprecationWarning),
+        WarningInfo(


Can you please add comments (ideally with links to some issue tracker) to motivate the fact that we ignore those particular cases?

That might require some git blame archeology.

I actually add back a comment I removed by mistake, this is about pyamg that uses pkg_resources in its latest release but not in main ...

I don't understand what you mean by "in its latest release but not in main".

pyamg 5.0.1 (latest release) will create a DeprecationWarning about using pkg_resources. pyamg in main has removed pkg_resources usage.

When pyamg > 5.0.1 is released and our minimum pyamg supported version is strictly greater than 5.0.1, we can remove the pkg_resource ignore warning filters.

ogrisel · 2024-02-08T10:50:41Z

doc/computing/parallelism.rst

+Note that for documentation build, `SKLEARN_WARNING_AS_ERRORS=1` is checking
+that the documentation build, in particular running examples, does not produce
+any warnings. This is different from the `-W` `sphinx-build` argument that
+catches syntax warnings in the rst files.


We should move the whole "Environment variables" section of the doc to its own environment_variables.rst file instead of parallelism.rst but ok for not doing it as part of this PR.

Yeah I agree ...

sklearn/utils/_testing.py

sklearn/conftest.py

.circleci/config.yml

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

… doc build (scikit-learn#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

… doc build (#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

lesteve added 6 commits February 6, 2024 14:05

wip

a494222

Remove Python 3.12 warnings

600246d

Add common function to use in tests and doc build

ac4e4aa

Remove pytest_addoption

1adc9e1

tweak

bd995fc

[scipy-dev]

2fefea7

lesteve changed the title ~~CI Use environment variable to turn warnings into errors~~ CI Use environment variable to turn warnings into errors in tests and doc build Feb 6, 2024

github-actions bot added module:utils Build / CI labels Feb 6, 2024

lesteve added 6 commits February 6, 2024 16:12

Adapt environment variables + tweaks

416678d

[scipy-dev]

d80ab81

[scipy-dev] fix for scipy dev attr renaming

be8b69c

[scipy-dev] [doc build] make sure full doc build works

5db0b99

[scipy-dev] [doc build] make sure full doc build works take 2

0e76426

[scipy-dev] [doc build] make sure full doc build works take 2

213e686

lesteve added the No Changelog Needed label Feb 6, 2024

lesteve mentioned this pull request Feb 6, 2024

CI Fix following scipy dev coo_matrix attr renaming #28374

Merged

[azure parallel] [doc build] Revert "scipy-dev fix for scipy dev attr…

35dac82

… renaming" This reverts commit be8b69c.

lesteve force-pushed the simpler-warnings-as-errors-setup branch from b5f9d87 to 35dac82 Compare February 6, 2024 16:26

adrinjalali approved these changes Feb 6, 2024

View reviewed changes

lesteve added 3 commits February 7, 2024 16:06

Use _inicache approach to change pytest config filterwarnings.

8d27ed7

Also use the same logic in doc/conf.py and tweak doc

[doc build]

20ebe68

[azure parallel] [doc build]

fb7fd6f

lesteve commented Feb 7, 2024

View reviewed changes

[scipy-dev] remove debug code

226c619

thomasjpfan reviewed Feb 8, 2024

View reviewed changes

lesteve added 4 commits February 8, 2024 06:07

Add test

0a702f4

Fix

1f47260

Use addinivalue_line

cefa1ce

[azure parallel] [doc build]

b5dad1c

lesteve mentioned this pull request Feb 8, 2024

CI Use Python 3.12 in scipy-dev #28383

Merged

lesteve added 2 commits February 8, 2024 10:11

Add back comment about difference with sphinx-build -W

1cbc675

[azure parallel]

656683f

ogrisel approved these changes Feb 8, 2024

View reviewed changes

lesteve and others added 3 commits February 8, 2024 13:38

Apply suggestions from code review

6502915

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Add back comment about pyamg

11b8a8f

Tweak comment

ca0e111

lesteve added No Changelog Needed and removed No Changelog Needed labels Feb 8, 2024

glemaitre approved these changes Feb 8, 2024

View reviewed changes

glemaitre merged commit ad3addd into scikit-learn:main Feb 8, 2024

lesteve deleted the simpler-warnings-as-errors-setup branch February 8, 2024 17:56

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024

CI Use environment variable to turn warnings into errors in tests and…

2999c59

… doc build (scikit-learn#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

LeoGrin pushed a commit to LeoGrin/scikit-learn that referenced this pull request Feb 12, 2024

CI Use environment variable to turn warnings into errors in tests and…

fdde820

… doc build (scikit-learn#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Feb 13, 2024

CI Use environment variable to turn warnings into errors in tests and…

44c7699

… doc build (scikit-learn#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre pushed a commit that referenced this pull request Feb 13, 2024

CI Use environment variable to turn warnings into errors in tests and…

ed604ce

… doc build (#28372) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI Use environment variable to turn warnings into errors in tests and doc build #28372

CI Use environment variable to turn warnings into errors in tests and doc build #28372

lesteve commented Feb 6, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading

adrinjalali left a comment

adrinjalali Feb 6, 2024

lesteve Feb 7, 2024

lesteve Feb 7, 2024

adrinjalali Feb 7, 2024

lesteve Feb 7, 2024

ogrisel Feb 7, 2024

adrinjalali Feb 6, 2024

ogrisel Feb 7, 2024

lesteve Feb 7, 2024 •

edited

Loading

adrinjalali Feb 7, 2024

ogrisel Feb 8, 2024

lesteve commented Feb 7, 2024 •

edited

Loading

lesteve Feb 7, 2024 •

edited

Loading

lesteve commented Feb 7, 2024

lesteve commented Feb 7, 2024

thomasjpfan Feb 8, 2024

lesteve Feb 8, 2024

lesteve Feb 8, 2024

lesteve commented Feb 8, 2024

ogrisel left a comment

ogrisel Feb 8, 2024

lesteve Feb 8, 2024

ogrisel Feb 8, 2024

lesteve Feb 8, 2024

ogrisel Feb 8, 2024 •

edited

Loading

lesteve Feb 8, 2024

CI Use environment variable to turn warnings into errors in tests and doc build #28372

CI Use environment variable to turn warnings into errors in tests and doc build #28372

Conversation

lesteve commented Feb 6, 2024 • edited Loading

github-actions bot commented Feb 6, 2024 • edited Loading

✔️ Linting Passed

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Feb 7, 2024 • edited Loading

lesteve Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

lesteve commented Feb 7, 2024

lesteve commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Feb 8, 2024

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogrisel Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Feb 6, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading

lesteve Feb 7, 2024 •

edited

Loading

lesteve commented Feb 7, 2024 •

edited

Loading

lesteve Feb 7, 2024 •

edited

Loading

ogrisel Feb 8, 2024 •

edited

Loading