Skip to content

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

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

Merged
merged 26 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a494222
wip
lesteve Feb 5, 2024
600246d
Remove Python 3.12 warnings
lesteve Feb 6, 2024
ac4e4aa
Add common function to use in tests and doc build
lesteve Feb 6, 2024
1adc9e1
Remove pytest_addoption
lesteve Feb 6, 2024
bd995fc
tweak
lesteve Feb 6, 2024
2fefea7
[scipy-dev]
lesteve Feb 6, 2024
416678d
Adapt environment variables + tweaks
lesteve Feb 6, 2024
d80ab81
[scipy-dev]
lesteve Feb 6, 2024
be8b69c
[scipy-dev] fix for scipy dev attr renaming
lesteve Feb 6, 2024
5db0b99
[scipy-dev] [doc build] make sure full doc build works
lesteve Feb 6, 2024
0e76426
[scipy-dev] [doc build] make sure full doc build works take 2
lesteve Feb 6, 2024
213e686
[scipy-dev] [doc build] make sure full doc build works take 2
lesteve Feb 6, 2024
35dac82
[azure parallel] [doc build] Revert "scipy-dev fix for scipy dev attr…
lesteve Feb 6, 2024
8d27ed7
Use _inicache approach to change pytest config filterwarnings.
lesteve Feb 7, 2024
20ebe68
[doc build]
lesteve Feb 7, 2024
fb7fd6f
[azure parallel] [doc build]
lesteve Feb 7, 2024
226c619
[scipy-dev] remove debug code
lesteve Feb 7, 2024
0a702f4
Add test
lesteve Feb 8, 2024
1f47260
Fix
lesteve Feb 8, 2024
cefa1ce
Use addinivalue_line
lesteve Feb 8, 2024
b5dad1c
[azure parallel] [doc build]
lesteve Feb 8, 2024
1cbc675
Add back comment about difference with sphinx-build -W
lesteve Feb 8, 2024
656683f
[azure parallel]
lesteve Feb 8, 2024
6502915
Apply suggestions from code review
lesteve Feb 8, 2024
11b8a8f
Add back comment about pyamg
lesteve Feb 8, 2024
ca0e111
Tweak comment
lesteve Feb 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ jobs:
- OPENBLAS_NUM_THREADS: 2
- CONDA_ENV_NAME: testenv
- LOCK_FILE: build_tools/circle/doc_min_dependencies_linux-64_conda.lock
# Do not fail if the documentation build generates warnings
- SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS: 'false'
# Do not fail if the documentation build generates warnings with minimum
# dependencies as long as we can avoid raising warnings with more recent
# versions of the same dependencies.
- SKLEARN_WARNINGS_AS_ERRORS: '0'
steps:
- checkout
- run: ./build_tools/circle/checkout_merge_commit.sh
Expand Down Expand Up @@ -60,8 +62,9 @@ jobs:
- OPENBLAS_NUM_THREADS: 2
- CONDA_ENV_NAME: testenv
- LOCK_FILE: build_tools/circle/doc_linux-64_conda.lock
# Make sure that we fail if the documentation build generates warnings
- SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS: 'true'
# Make sure that we fail if the documentation build generates warnings with
# recent versions of the dependencies.
- SKLEARN_WARNINGS_AS_ERRORS: '1'
steps:
- checkout
- run: ./build_tools/circle/checkout_merge_commit.sh
Expand Down
8 changes: 4 additions & 4 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ jobs:
pylatest_pip_scipy_dev:
DISTRIB: 'conda-pip-scipy-dev'
LOCK_FILE: './build_tools/azure/pylatest_pip_scipy_dev_linux-64_conda.lock'
CHECK_WARNINGS: 'true'
SKLEARN_WARNINGS_AS_ERRORS: '1'
CHECK_PYTEST_SOFT_DEPENDENCY: 'true'
# Tests that require large downloads over the networks are skipped in CI.
# Here we make sure, that they are still run on a regular basis.
Expand Down Expand Up @@ -195,7 +195,7 @@ jobs:
pymin_conda_forge_openblas_ubuntu_2204:
DISTRIB: 'conda'
LOCK_FILE: './build_tools/azure/pymin_conda_forge_openblas_ubuntu_2204_linux-64_conda.lock'
CHECK_WARNINGS: 'true'
SKLEARN_WARNINGS_AS_ERRORS: '1'
COVERAGE: 'false'
SKLEARN_TESTS_GLOBAL_RANDOM_SEED: '0' # non-default seed

Expand Down Expand Up @@ -249,7 +249,7 @@ jobs:
DISTRIB: 'conda-pip-latest'
LOCK_FILE: './build_tools/azure/pylatest_pip_openblas_pandas_linux-64_conda.lock'
CHECK_PYTEST_SOFT_DEPENDENCY: 'true'
CHECK_WARNINGS: 'true'
SKLEARN_WARNINGS_AS_ERRORS: '1'
SKLEARN_TESTS_GLOBAL_RANDOM_SEED: '3' # non-default seed
# disable pytest-xdist to have 1 job where OpenMP and BLAS are not single
# threaded because by default the tests configuration (sklearn/conftest.py)
Expand Down Expand Up @@ -315,7 +315,7 @@ jobs:
pymin_conda_forge_mkl:
DISTRIB: 'conda'
LOCK_FILE: ./build_tools/azure/pymin_conda_forge_mkl_win-64_conda.lock
CHECK_WARNINGS: 'true'
SKLEARN_WARNINGS_AS_ERRORS: '1'
# The Azure Windows runner is typically much slower than other CI
# runners due to the lack of compiler cache. Running the tests with
# coverage enabled make them run extra slower. Since very few parts of
Expand Down
20 changes: 0 additions & 20 deletions build_tools/azure/test_script.sh
Original file line number Diff line number Diff line change
Expand Up @@ -48,26 +48,6 @@ if [[ "$COVERAGE" == "true" ]]; then
TEST_CMD="$TEST_CMD --cov-config='$COVERAGE_PROCESS_START' --cov sklearn --cov-report="
fi

if [[ -n "$CHECK_WARNINGS" ]]; then
TEST_CMD="$TEST_CMD -Werror::DeprecationWarning -Werror::FutureWarning -Werror::sklearn.utils.fixes.VisibleDeprecationWarning"

# Ignore pkg_resources deprecation warnings triggered by pyamg
TEST_CMD="$TEST_CMD -W 'ignore:pkg_resources is deprecated as an API:DeprecationWarning'"
TEST_CMD="$TEST_CMD -W 'ignore:Deprecated call to \`pkg_resources:DeprecationWarning'"

# pytest-cov issue https://github.com/pytest-dev/pytest-cov/issues/557 not
# fixed although it has been closed. https://github.com/pytest-dev/pytest-cov/pull/623
# would probably fix it.
TEST_CMD="$TEST_CMD -W 'ignore:The --rsyncdir command line argument and rsyncdirs config variable are deprecated.:DeprecationWarning'"

# In some case, exceptions are raised (by bug) in tests, and captured by pytest,
# but not raised again. This is for instance the case when Cython directives are
# activated: IndexErrors (which aren't fatal) are raised on out-of-bound accesses.
# In those cases, pytest instead raises pytest.PytestUnraisableExceptionWarnings,
# which we must treat as errors on the CI.
TEST_CMD="$TEST_CMD -Werror::pytest.PytestUnraisableExceptionWarning"
fi

if [[ "$PYTEST_XDIST_VERSION" != "none" ]]; then
XDIST_WORKERS=$(python -c "import joblib; print(joblib.cpu_count(only_physical_cores=True))")
TEST_CMD="$TEST_CMD -n$XDIST_WORKERS"
Expand Down
33 changes: 25 additions & 8 deletions doc/computing/parallelism.rst
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,28 @@ Users looking for the best performance might want to tune this variable using
powers of 2 so as to get the best parallelism behavior for their hardware,
especially with respect to their caches' sizes.

`SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This environment variable issue errors instead of warnings when building the
documentation. It ensures that we don't introduce new warnings in the example
gallery. By default, the warnings are treated as errors (e.g. `"true"`). This
is different from `SPHINXOPTS="-W"` that catch syntax warnings from the rst
generation.
`SKLEARN_WARNINGS_AS_ERRORS`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This environment variable is used to turn warnings into errors in tests and
documentation build.

Some CI (Continuous Integration) builds set `SKLEARN_WARNINGS_AS_ERRORS=1`, for
example to make sure that we catch deprecation warnings from our dependencies
and that we adapt our code.

To locally run with the same "warnings as errors" setting as in these CI builds
you can set `SKLEARN_WARNINGS_AS_ERRORS=1`.

By default, warnings are not turned into errors. This is the case if
`SKLEARN_WARNINGS_AS_ERRORS` is unset, or `SKLEARN_WARNINGS_AS_ERRORS=0`.

This environment variable use specific warning filters to ignore some warnings,
since sometimes warnings originate from third-party libraries and there is not
much we can do about it. You can see the warning filters in the
`_get_warnings_filters_info_list` function in `sklearn/utils/_testing.py`.

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.
Copy link
Member

@ogrisel ogrisel Feb 8, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree ...

25 changes: 3 additions & 22 deletions doc/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from pathlib import Path

from sklearn.externals._packaging.version import parse
from sklearn.utils._testing import turn_warnings_into_errors

# If extensions (or modules to document with autodoc) are in another
# directory, add these directories to sys.path here. If the directory
Expand Down Expand Up @@ -702,8 +703,6 @@ def setup(app):
),
)

from sklearn.utils.fixes import VisibleDeprecationWarning

warnings.filterwarnings(
"ignore",
category=UserWarning,
Expand All @@ -712,26 +711,8 @@ def setup(app):
" non-GUI backend, so cannot show the figure."
),
)
if os.environ.get("SKLEARN_DOC_BUILD_WARNINGS_AS_ERRORS", "true").lower() == "true":
# Raise warning as error in example to catch warnings when building the
# documentation Since we are using lock files to build the documentation, we should
# not have any warnings. Before updating the lock files, we need to fix them.
for warning_type in (FutureWarning, DeprecationWarning, VisibleDeprecationWarning):
warnings.filterwarnings("error", category=warning_type)
# TODO: remove when pyamg > 5.0.1
# Avoid a deprecation warning due pkg_resources deprecation in pyamg.
warnings.filterwarnings(
"ignore",
message="pkg_resources is deprecated as an API",
category=DeprecationWarning,
)
# XXX: Easiest way to ignore Pyarrow DeprecationWarning in the short-term.
# See https://github.com/pandas-dev/pandas/issues/54466 for more details.
warnings.filterwarnings(
"ignore",
message=r"\s*Pyarrow will become a required dependency of pandas.*",
category=DeprecationWarning,
)
if os.environ.get("SKLEARN_WARNINGS_AS_ERRORS", "0") != "0":
turn_warnings_into_errors()

# maps functions with a class name that is indistinguishable when case is
# ignore to another filename
Expand Down
3 changes: 0 additions & 3 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ addopts =
# source folder.
-p sklearn.tests.random_seed

filterwarnings =
Copy link
Member Author

@lesteve lesteve Feb 7, 2024

Choose a reason for hiding this comment

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

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

ignore:the matrix subclass:PendingDeprecationWarning

[mypy]
ignore_missing_imports = True
allow_redefinition = True
Expand Down
22 changes: 12 additions & 10 deletions sklearn/conftest.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import builtins
import platform
import sys
import warnings
from contextlib import suppress
from functools import wraps
from os import environ
Expand All @@ -27,7 +26,12 @@
)
from sklearn.tests import random_seed
from sklearn.utils import _IS_32BIT
from sklearn.utils.fixes import np_base_version, parse_version, sp_version
from sklearn.utils._testing import get_pytest_filterwarning_lines
from sklearn.utils.fixes import (
np_base_version,
parse_version,
sp_version,
)

if parse_version(pytest.__version__) < parse_version(PYTEST_MIN_VERSION):
raise ImportError(
Expand Down Expand Up @@ -276,14 +280,12 @@ def pytest_configure(config):
if not config.pluginmanager.hasplugin("sklearn.tests.random_seed"):
config.pluginmanager.register(random_seed)


def pytest_collectstart(collector):
# XXX: Easiest way to ignore pandas Pyarrow DeprecationWarning in the
# short-term. See https://github.com/pandas-dev/pandas/issues/54466 for
# more details.
warnings.filterwarnings(
"ignore", message=r"\s*Pyarrow", category=DeprecationWarning
)
if environ.get("SKLEARN_WARNINGS_AS_ERRORS", "0") != "0":
# 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
for line in get_pytest_filterwarning_lines():
config.addinivalue_line("filterwarnings", line)


@pytest.fixture
Expand Down
74 changes: 73 additions & 1 deletion sklearn/utils/_testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import unittest
import warnings
from collections.abc import Iterable
from dataclasses import dataclass
from functools import wraps
from inspect import signature
from subprocess import STDOUT, CalledProcessError, TimeoutExpired, check_output
Expand All @@ -49,7 +50,7 @@
_in_unstable_openblas_configuration,
)
from sklearn.utils._array_api import _check_array_api_dispatch
from sklearn.utils.fixes import parse_version, sp_version
from sklearn.utils.fixes import VisibleDeprecationWarning, parse_version, sp_version
from sklearn.utils.multiclass import check_classification_targets
from sklearn.utils.validation import (
check_array,
Expand Down Expand Up @@ -1095,3 +1096,74 @@ def _array_api_for_tests(array_namespace, device):
if cupy.cuda.runtime.getDeviceCount() == 0:
raise SkipTest("CuPy test requires cuda, which is not available")
return xp


def _get_warnings_filters_info_list():
@dataclass
class WarningInfo:
action: "warnings._ActionKind"
message: str = ""
category: type[Warning] = Warning

def to_filterwarning_str(self):
if self.category.__module__ == "builtins":
category = self.category.__name__
else:
category = f"{self.category.__module__}.{self.category.__name__}"

return f"{self.action}:{self.message}:{category}"

return [
WarningInfo("error", category=DeprecationWarning),
WarningInfo("error", category=FutureWarning),
WarningInfo("error", category=VisibleDeprecationWarning),
# TODO: remove when pyamg > 5.0.1
# Avoid a deprecation warning due pkg_resources usage in pyamg.
WarningInfo(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ...

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"ignore",
message="pkg_resources is deprecated as an API",
category=DeprecationWarning,
),
WarningInfo(
"ignore",
message="Deprecated call to `pkg_resources",
category=DeprecationWarning,
),
# pytest-cov issue https://github.com/pytest-dev/pytest-cov/issues/557 not
# fixed although it has been closed. https://github.com/pytest-dev/pytest-cov/pull/623
# would probably fix it.
WarningInfo(
"ignore",
message=(
"The --rsyncdir command line argument and rsyncdirs config variable are"
" deprecated"
),
category=DeprecationWarning,
),
# XXX: Easiest way to ignore pandas Pyarrow DeprecationWarning in the
# short-term. See https://github.com/pandas-dev/pandas/issues/54466 for
# more details.
WarningInfo(
"ignore",
message=r"\s*Pyarrow will become a required dependency",
category=DeprecationWarning,
),
]


def get_pytest_filterwarning_lines():
warning_filters_info_list = _get_warnings_filters_info_list()
return [
warning_info.to_filterwarning_str()
for warning_info in warning_filters_info_list
]


def turn_warnings_into_errors():
warnings_filters_info_list = _get_warnings_filters_info_list()
for warning_info in warnings_filters_info_list:
warnings.filterwarnings(
warning_info.action,
message=warning_info.message,
category=warning_info.category,
)
39 changes: 39 additions & 0 deletions sklearn/utils/tests/test_testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
TempMemmap,
_convert_container,
_delete_folder,
_get_warnings_filters_info_list,
assert_allclose,
assert_allclose_dense_sparse,
assert_no_warnings,
Expand All @@ -26,6 +27,7 @@
ignore_warnings,
raises,
set_random_state,
turn_warnings_into_errors,
)
from sklearn.utils.deprecation import deprecated
from sklearn.utils.fixes import (
Expand Down Expand Up @@ -882,3 +884,40 @@ def test_convert_container_sparse_to_sparse(constructor_name):
"""
X_sparse = sparse.random(10, 10, density=0.1, format="csr")
_convert_container(X_sparse, constructor_name)


def check_warnings_as_errors(warning_info, warnings_as_errors):
if warning_info.action == "error" and warnings_as_errors:
with pytest.raises(warning_info.category, match=warning_info.message):
warnings.warn(
message=warning_info.message,
category=warning_info.category,
)
if warning_info.action == "ignore":
with warnings.catch_warnings(record=True) as record:
message = warning_info.message
# Special treatment when regex is used
if "Pyarrow" in message:
message = "\nPyarrow will become a required dependency"

warnings.warn(
message=message,
category=warning_info.category,
)
assert len(record) == 0 if warnings_as_errors else 1
if record:
assert str(record[0].message) == message
assert record[0].category == warning_info.category


@pytest.mark.parametrize("warning_info", _get_warnings_filters_info_list())
def test_sklearn_warnings_as_errors(warning_info):
warnings_as_errors = os.environ.get("SKLEARN_WARNINGS_AS_ERRORS", "0") != "0"
check_warnings_as_errors(warning_info, warnings_as_errors=warnings_as_errors)


@pytest.mark.parametrize("warning_info", _get_warnings_filters_info_list())
def test_turn_warnings_into_errors(warning_info):
with warnings.catch_warnings():
turn_warnings_into_errors()
check_warnings_as_errors(warning_info, warnings_as_errors=True)