Skip to content

Plumb statistics object into ProgressThread for all engines - #23082

Merged
rapids-bot[bot] merged 2 commits into
rapidsai:mainfrom
wence-:wence/fix/comm-statistics
Jul 10, 2026
Merged

Plumb statistics object into ProgressThread for all engines#23082
rapids-bot[bot] merged 2 commits into
rapidsai:mainfrom
wence-:wence/fix/comm-statistics

Conversation

@wence-

@wence- wence- commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Description

We need this so that the communicators that we build have an active statistics object for keeping tack of communcation volume.

Previously we never noticed this because the only communication volume statistics we tracked were from the Shuffler that got its statistics from the buffer resource. However, it makes more sense to use the communicator since (for example) the allreduce implementation doesn't allocate and therefore doesn't have a buffer resource.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 2, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jul 2, 2026

Copy link
Copy Markdown

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 Jul 2, 2026
@wence-

wence- commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Needs rapidsai/rapidsmpf#1118

@GPUtester GPUtester moved this to In Progress in cuDF Python Jul 2, 2026
Comment thread python/cudf_polars/cudf_polars/engine/spmd.py Outdated
Comment thread python/cudf_polars/cudf_polars/engine/spmd.py Outdated
@wence-
wence- force-pushed the wence/fix/comm-statistics branch 5 times, most recently from 0400d76 to 216d366 Compare July 3, 2026 14:38

@pentschev pentschev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left another comment, and I think a cuDF-level test that performs a communicator-backed operation and observes its send/receive counter through gather_statistics() would be good to have, otherwise LGTM. Thanks Lawrence!

Comment thread python/cudf_polars/cudf_polars/engine/spmd.py
We need this so that the communicators that we build have an active
statistics object for keeping tack of communcation volume.

Previously we never noticed this because the only communication volume
statistics we tracked were from the Shuffler that got its statistics from
the buffer resource. However, it makes more sense to use the communicator
since (for example) the allreduce implementation doesn't allocate and
therefore doesn't have a buffer resource.
@wence-
wence- force-pushed the wence/fix/comm-statistics branch from 216d366 to 1ffbde5 Compare July 9, 2026 11:38
@wence-
wence- marked this pull request as ready for review July 9, 2026 11:39
@wence-
wence- requested a review from a team as a code owner July 9, 2026 11:39
@wence-
wence- requested a review from madsbk July 9, 2026 11:39
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 141bdca8-780f-48c6-8cb4-18776917196b

📥 Commits

Reviewing files that changed from the base of the PR and between 1ffbde5 and a098bf9.

📒 Files selected for processing (1)
  • python/cudf_polars/tests/streaming/test_statistics.py

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Improved statistics and progress tracking consistency across execution engines, now aligned with the active runtime configuration.
    • Statistics objects are reused during engine lifecycle events instead of being recreated.
  • Bug Fixes

    • Fixed cases where progress/statistics could be initialized or reset inconsistently across setup/reset paths.
    • Reset behavior now updates the enabled/disabled state without losing collected statistics state unexpectedly.
  • Tests

    • Updated streaming statistics test expectations after clearing statistics.
  • Documentation

    • Refreshed engine documentation to reflect the updated statistics behavior.

Walkthrough

Adds a reset_statistics_from_options helper in core.py and updates Dask, Ray, SPMD, and the default singleton engine to reuse existing Statistics objects while rebuilding or resetting statistics state.

Changes

Statistics reuse and reset

Layer / File(s) Summary
reset_statistics_from_options helper
python/cudf_polars/cudf_polars/engine/core.py
Adds a helper that enables or disables an existing Statistics instance based on Options, returning the mutated instance without clearing it.
Dask worker statistics wiring and reset
python/cudf_polars/cudf_polars/engine/dask.py, python/cudf_polars/tests/streaming/test_statistics.py
Stores per-worker Statistics on _WorkerContext, threads that object through setup, reset, and statistics collection, and updates the streaming statistics test to accept the retained "event-loop-total" entry after clear=True.
Ray RankActor statistics reuse
python/cudf_polars/cudf_polars/engine/ray.py
Wires self._rapidsmpf_statistics into ProgressThread during setup, adds a post-barrier _base_mr assertion, and replaces statistics recreation in reset with the reset helper followed by clear().
SPMDEngine statistics init and reset
python/cudf_polars/cudf_polars/engine/spmd.py
Updates the class docstring and __init__ to construct or reset Statistics depending on whether a communicator is provided, and updates _reset to use the helper plus clear().
Default singleton engine ProgressThread
python/cudf_polars/cudf_polars/engine/default_singleton_engine.py
Precomputes RapidsMPF options, derives Statistics from them, and passes a Statistics-backed ProgressThread and options into single_communicator.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: passing statistics objects into ProgressThread across engines.
Description check ✅ Passed The description is directly about giving communicators an active statistics object to track communication volume.
Docstring Coverage ✅ Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

We might still see an event-loop stat due to timing.
Comment thread python/cudf_polars/cudf_polars/engine/ray.py
@wence-

wence- commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 2f082a7 into rapidsai:main Jul 10, 2026
108 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python Jul 10, 2026
@wence-
wence- deleted the wence/fix/comm-statistics branch July 10, 2026 13:33
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 improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants