Fix warnings in dask-cudf test suite #20951
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 7544eab |
TomAugspurger
added
improvement
|
/ok to test 2723bd7 |
|
/ok to test 15edec7 |
| return ngpus >= n | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
Like you helped identify with the cudf_polars tests, should this be made module scope to not recreate this per test?
There was a problem hiding this comment.
Done in c6bc789.
We previously used the loop fixture from distributed. I think we don't need that: we shouldn't really be sensitive to which event loop the test is running on these days.
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
I left one small non-blocking comment... do whatever you want with it. Otherwise looks good to me, approving from a ci-codeowners / packaging-codeowners perspective.
| # This warning comes from dask-expr, and is probably a consequence of | ||
| # cudf's NA handling differing from pandas' default handling, | ||
| # since we try to construct an int64 ndarray with NaNs. | ||
| @pytest.mark.filterwarnings("ignore::RuntimeWarning") |
There was a problem hiding this comment.
For my understanding... why are these being added as pytest markers instead of in the configuration in pyproject.toml alongside other warning filters?
I think it also may be helpful to limit this to only dask_expr instead of all RuntimeWarning, so you have a chance to see and react to other RuntimeWarnings besides the one the comment describes.
And example of that:
filterwarnings = [
...
"ignore::FutureWarning:sklearn",
"ignore::DeprecationWarning:sklearn",
...
]And docs on the syntax: https://docs.python.org/3/library/warnings.html#describing-warning-filters
There was a problem hiding this comment.
My reasoning was to make it local: maybe it's just a personal preference, but I like the idea of putting general rules in the pyproject.toml (error by default) but override that locally in specific modules / functions.
I'll adopt your suggestion to filter this to a module.
There was a problem hiding this comment.
Yes, add a module filter, otherwise this is fine to define here. We only need to use pyproject.toml for global (or far-reaching, or large-but-temporary) ignores.
There was a problem hiding this comment.
Sounds good, thanks for explaining that.
| # This warning comes from dask-expr, and is probably a consequence of | ||
| # cudf's NA handling differing from pandas' default handling, | ||
| # since we try to construct an int64 ndarray with NaNs. | ||
| @pytest.mark.filterwarnings("ignore::RuntimeWarning") |
There was a problem hiding this comment.
Yes, add a module filter, otherwise this is fine to define here. We only need to use pyproject.toml for global (or far-reaching, or large-but-temporary) ignores.
4 participants
Description
This fixes the warnings in the dask-cudf test suite, so that the tests run with
-Werror.Checklist