Skip to content

Configurable blocksize mode for streaming executor in unit tests #19146

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

Draft
wants to merge 7 commits into
base: branch-25.08
Choose a base branch
from

Conversation

TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented Jun 12, 2025

This adds a new test option --blocksize-mode to the test runner, which lets us easily exercise the multi-partition code paths of the streaming executor.

When --blocksize-mode=small and --executor="streaming" the default behavior will be to create a GPUEngine with max_rows_per_partition=5 (which controls the partition size from in-memory dataframes) and target_partition_size=10 (which controls the partition size from parquet sources).

Towards #18928

This adds a new test option ``--blocksize-mode`` to the test runner, which
lets us easily exercise the multi-partition code paths of the streaming
executor.

When ``--blocksize-mode=small`` and ``--executor="streaming"`` the default
behavior will be to create a GPUEngine with ``max_rows_per_partition=5``
and ``target_partition_size=10``.
Copy link

copy-pr-bot bot commented Jun 12, 2025

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.

@github-actions github-actions bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels Jun 12, 2025
@GPUtester GPUtester moved this to In Progress in cuDF Python Jun 12, 2025
@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Jun 12, 2025

The bad news: 196 tests fail with this. I'm starting to work through those in the issues linked from #18928. Fixes for those will probably go in separate PRs.

But I wanted to put this up a little early to discuss the general approach. For now, I've added a separate run of the full cudf_polars/tests suite with --blocksize-mode="small". This is probably sufficient to get the coverage we want, but is a bit of a blunt tool.

In particular, I don't the interaction between that second test run and the tests that want specific control over the GPU engine / configuration. Right now those tests will be run twice with the exact same configuration (--blocksize-mode, like --scheduler only has an effect when the engine passed to assert_gpu_result_equal is None.

As an alternative, we could make engine a required argument of assert_gpu_result_equal and force the test to think a bit about what it wants. The majority of our tests don't care, and so can just use an engine fixture, which we can parametrize over the blocksize-modes to get good coverage. That's a bunch of busywork to add an engine fixture to each test that doesn't already have it and will result in a bigger diff, but is a bit cleaner. Anyone have thoughts on whether that's worth doing?

@TomAugspurger TomAugspurger added tests Unit testing for project non-breaking Non-breaking change labels Jun 13, 2025
@TomAugspurger TomAugspurger changed the title Configurable blocksize mode for streaming executor Configurable blocksize mode for streaming executor in unit tests Jun 13, 2025
@TomAugspurger
Copy link
Contributor Author

Quick status update here: We have two more PRs in the works that fix the last two correctness issues:

And then I'll push an update here that slightly modifies some of the tests (e.g. changing some assert parameters like check_order=False, skipping some tests) where the chunked streaming executor is expected to behave differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf-polars Issues specific to cudf-polars non-breaking Non-breaking change Python Affects Python cuDF API. tests Unit testing for project
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

1 participant