Repository navigation
Run cudf_polars unit tests with RapidsMPF #21807
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
Changes from 72 commits
c65a0a4
b7e9cce
96fe32c
8fb8b75
7452bd8
4b9c5e7
e46518f
8623f22
fea7ee9
5556a7e
6816244
e6a7f80
f10b5f5
e5223f5
a7a9c81
30294dd
8e9cbd2
2e30436
b45dbd9
acaef66
2cd190b
8b08f62
f635088
eeef8e8
aa8c67b
3c8eb10
63ecdae
c02cb36
87ba76a
9318457
1704866
202cca5
415d5ea
ab95752
17f84a4
151be46
a401aa3
35e2d15
70791cd
1cfcb2e
5da9dac
916b164
0990377
c6d598e
0b54d50
c7b6d5d
2056a38
54fe379
b29fc5e
4997229
26f0fe1
384f878
01fa311
d2095ae
9b28ea7
d74ba8b
61bd956
1144a2a
609cf99
d19b49f
7bb6520
804cf3b
8f642f3
35f91da
28d0c2c
adef645
5cff7f7
af3cc97
6c5d2b8
8acf8e9
2d5019f
64e35bb
12a154f
0f4ec9e
f1eff5f
f754afc
2252824
c36c39b
545602c
eb816aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,10 +14,11 @@ cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cudf_polars/ | |
|
|
||
| # Run experimental tests with the "single" cluster mode and the "rapidsmpf" runtime | ||
| rapids-logger "Running experimental tests with the 'rapidsmpf' runtime and a 'single' cluster" | ||
| timeout 10m python -m pytest --cache-clear "$@" "tests/experimental" \ | ||
| CUDF_POLARS__PARQUET_OPTIONS__USE_RAPIDSMPF_NATIVE=1 CUDF_POLARS__EXECUTOR__SHUFFLE_METHOD=rapidsmpf timeout 10m python -m pytest --cache-clear "$@" tests \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you say why setting In [8]: import polars as pl
...: from cudf_polars.utils.config import ConfigOptions
...:
...:
...: engine = pl.GPUEngine(executor="streaming", executor_options={"runtime": "rapidsmpf"})
...: cfg = ConfigOptions.from_polars_engine(engine)
...: cfg.executor.shuffle_method
...:
Out[8]: <ShuffleMethod._RAPIDSMPF_SINGLE: 'rapidsmpf-single'>
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah fair point, I suppose I was trying to guarantee we would always use the rapidsmpf shuffler here (since there's logic for the distributed cluster falling back to the task shuffler), but I'll remove as it's not really necessary. |
||
| --executor streaming \ | ||
| --cluster single \ | ||
| --runtime rapidsmpf | ||
| --runtime rapidsmpf \ | ||
| --blocksize-mode small | ||
|
|
||
| # Run experimental tests with the "distributed" cluster mode and the "rapidsmpf" runtime | ||
| rapids-logger "Running experimental tests with the 'rapidsmpf' runtime and a 'distributed' cluster" | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -76,8 +76,14 @@ def test_booleanfunction_all_any_kleene(expr, ignore_nulls): | |||||
| ) | ||||||
| @pytest.mark.parametrize("has_nans", [False, True], ids=["no_nans", "nans"]) | ||||||
| def test_boolean_function_unary( | ||||||
| expr: Callable[[pl.Expr], pl.Expr], *, has_nans: bool, has_nulls: bool | ||||||
| expr: Callable[[pl.Expr], pl.Expr], | ||||||
| *, | ||||||
| has_nans: bool, | ||||||
| has_nulls: bool, | ||||||
| using_rapidsmpf: bool, | ||||||
| ) -> None: | ||||||
| if using_rapidsmpf: | ||||||
| pytest.skip("Can segault on 12.9 builds with rapidsmpf runtime.") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Do we have an issue tracking this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, updated the pytest.skips to fix the type and include the github link |
||||||
| values: list[float | None] = [1, 2, 3, 4, 5] | ||||||
| if has_nans: | ||||||
| values[3] = float("nan") | ||||||
|
|
@@ -160,7 +166,9 @@ def test_boolean_isbetween(closed, bounds): | |||||
| "expr", [pl.any_horizontal("*"), pl.all_horizontal("*")], ids=["any", "all"] | ||||||
| ) | ||||||
| @pytest.mark.parametrize("wide", [False, True], ids=["narrow", "wide"]) | ||||||
| def test_boolean_horizontal(expr, has_nulls, wide): | ||||||
| def test_boolean_horizontal(expr, has_nulls, wide, using_rapidsmpf): | ||||||
| if using_rapidsmpf: | ||||||
| pytest.skip("Can segault on 12.9 builds with rapidsmpf runtime.") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This typo occurs repeatedly.
Suggested change
|
||||||
| ldf = pl.LazyFrame( | ||||||
| { | ||||||
| "a": [False, False, False, False, False, True], | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import sys | ||
| from typing import Any, cast | ||
|
|
||
|
|
@@ -381,8 +382,12 @@ def test_scheduler_deprecated() -> None: | |
|
|
||
| def test_validate_shuffle_method_defaults( | ||
| *, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| rapidsmpf_distributed_available: bool, | ||
| ) -> None: | ||
| # TODO: Can remove once removed from ci/run_cudf_polars_pytests.sh | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, I think keeping this is fine. If possible, our tests should be robust to what's in the environment when the test process starts.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll just go ahead and revert this since I'll be removing the shuffler environment variable that affected the tests in this file. |
||
| # and the default shuffle method changes to "rapidsmpf" | ||
| monkeypatch.delenv("CUDF_POLARS__EXECUTOR__SHUFFLE_METHOD", raising=False) | ||
| config = ConfigOptions.from_polars_engine( | ||
| pl.GPUEngine( | ||
| executor="streaming", | ||
|
|
@@ -804,7 +809,12 @@ def test_cuda_stream_policy_from_env( | |
| elif runtime == "rapidsmpf": | ||
| with pytest.raises(ValueError, match="The rapidsmpf streaming engine"): | ||
| ConfigOptions.from_polars_engine(engine) | ||
| else: | ||
| elif ( | ||
| os.environ.get("CUDF_POLARS__EXECUTOR__SHUFFLE_METHOD", "") != "rapidsmpf" | ||
| and not rapidsmpf_single_available | ||
| ): | ||
| # CUDF_POLARS__EXECUTOR__SHUFFLE_METHOD set in ci/run_cudf_polars_pytests.sh | ||
| # Would raise since rapidsmpf shuffle requires rapidsmpf to be installed | ||
| config = ConfigOptions.from_polars_engine(engine) | ||
| assert config.cuda_stream_policy == env | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm kind of annoying that all the cudf and pandas tests will run because of this change. Perhaps we can figure out how to avoid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we either need to:
cidirectory to be project specific e.g.python/cudf_polars/ci/,python/cudf/cietc. (probably a larger ask)