NVIDIA / NVIDIA/cudf

[Story]: Trim cudf-polars's own test suite down to what upstream polars can't cover

Open
#23,963 2 comments 0 reactions 0 assignees View on GitHub
cudf-polars improvement
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

`cudf_polars/tests/` (plan node tests), `tests/expressions/`, and `tests/streaming/` together run roughly 2,600 tests, and a large share of them duplicate coverage that already exists in the upstream polars suite (run against the GPU engine in CI). A test that builds a `LazyFrame`, applies one ordinary polars operation on unremarkable data, and checks GPU output against CPU is already covered upstream, engine-agnostically. What's left after filtering those out is a smaller, genuinely local set: cudf-polars's own unsupported-op/fallback surface, GPU-kernel numerics edge cases (NaN/inf/overflow, decimal precision, dtype boundaries), and our own config/engine/optimizer internals (`GPUEngine` config, `StreamingOptions`, join-filter-pushdown, partitioning-request propagation, engine backends). None of this is testable upstream, since polars has no notion of any of it.

A chunk of the streaming-specific tests are also introspective in a way that isn't justified. They assert on hand-built IR node fields or repr strings instead of observable behavior, when a public-API test would cover the same ground just as well.

Goal: prune the duplicated tests, keep (or rewrite to be behavioral) the ones testing something genuinely cudf-polars-specific, and where a test covers real public-API behavior upstream doesn't test and has no cudf-polars dependency, contribute it upstream instead of keeping it local. Running the upstream suite with small partition sizes (`--inject-gpu-engine-blocksize=small`) already exists as a mechanism and can absorb most multi-partition-correctness tests directly. The exception is scan/IO tests, which are explicitly skipped there for being too slow, a gap that needs its own fix before those tests can be removed. Also worth resolving along the way: the upstream polars job currently runs with `raise_on_fail=False`, so it doesn't catch CPU-fallback regressions the way local tests do. Deleting local fallback tests without adding fallback tracking to the upstream run would be a net loss of coverage, not a cleanup.

Before deleting anything based on judgment calls about "this looks like standard polars behavior," we should back it with actual coverage data. `pytest-cov` supports [dynamic contexts](https://coverage.readthedocs.io/en/7.15.4/contexts.html#dynamic-contexts) (`--cov-context=test`), which records which test covers which line. As a first pass, even a plain line-coverage diff would help: run the polars test job and the cudf-polars test job separately, combine the two coverage reports, and look for cudf-polars lines that only the local job hits. Those are the parts of cudf-polars the local suite is actually covering better, and they're the strongest signal for what should stay local versus what's safe to prune.

Contributor guide

Open the contributing guide

Research direction

Start with cudf_polars/tests/, tests/expressions/, and tests/streaming/; run the upstream and cudf-polars jobs separately with --inject-gpu-engine-blocksize=small and collect coverage with pytest-cov dynamic contexts. Compare the reports to identify local-only coverage, then review fallback, scan/IO, configuration, optimizer, and behavioral tests. Done means duplicated tests are removed or moved upstream without losing fallback or scan coverage, and introspective tests are behavioral where appropriate.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.