Skip to content

Guarantee insert_finished() on bulk AllGather#22516

Merged
rapids-bot[bot] merged 1 commit into
rapidsai:release/26.06from
Matt711:bug/polars/allgather-ensure-insert-finished
May 15, 2026
Merged

Guarantee insert_finished() on bulk AllGather#22516
rapids-bot[bot] merged 1 commit into
rapidsai:release/26.06from
Matt711:bug/polars/allgather-ensure-insert-finished

Conversation

@Matt711
Copy link
Copy Markdown
Contributor

@Matt711 Matt711 commented May 14, 2026

Description

Need to make sure insert_finished is called for collective operations.

Checklist

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

@Matt711 Matt711 added the bug Something isn't working label May 14, 2026
@Matt711 Matt711 requested a review from a team as a code owner May 14, 2026 21:38
@Matt711 Matt711 added the non-breaking Non-breaking change label May 14, 2026
@Matt711 Matt711 requested a review from rjzamora May 14, 2026 21:38
@github-actions github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels May 14, 2026
@GPUtester GPUtester moved this to In Progress in cuDF Python May 14, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

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: 4023b4c2-f460-4ef4-9095-a38d6637fdaf

📥 Commits

Reviewing files that changed from the base of the PR and between 882878f and a4b6e2f.

📒 Files selected for processing (1)
  • python/cudf_polars/cudf_polars/experimental/rapidsmpf/frontend/core.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • python/cudf_polars/cudf_polars/experimental/rapidsmpf/frontend/core.py

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced reliability of data gathering operations by ensuring cleanup procedures are properly executed even when errors occur during data processing.

Walkthrough

The all_gather_host_data function wraps buffer insertion operations in a try/finally block to guarantee that AllGather.insert_finished() is called even if data packing or insertion raises an exception. A TODO comment documents a future refactor to make AllGather itself a context manager.

Changes

AllGather Error Handling

Layer / File(s) Summary
AllGather insertion with guaranteed cleanup
python/cudf_polars/cudf_polars/experimental/rapidsmpf/frontend/core.py
all_gather_host_data wraps PackedData.from_host_bytes(...) and allgather.insert(...) in a try/finally block to ensure allgather.insert_finished() executes even if those calls raise, with a TODO noting that AllGather should become a context manager.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main change: ensuring insert_finished() is called on bulk AllGather operations, which aligns with the changeset's primary objective.
Description check ✅ Passed The description is related to the changeset, stating the need to ensure insert_finished is called for collective operations, which matches the primary change in the code.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% 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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Matt711 Matt711 changed the title Ensure insert_finished() is called after all gathering statistics Guarantee insert_finished() on bulk AllGather May 14, 2026
@Matt711
Copy link
Copy Markdown
Contributor Author

Matt711 commented May 14, 2026

/merge

Copy link
Copy Markdown
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Good catch!

@mroeschke
Copy link
Copy Markdown
Contributor

Probably good to retarget to release/26.06

@Matt711 Matt711 changed the base branch from main to release/26.06 May 15, 2026 18:09
@Matt711 Matt711 requested review from a team as code owners May 15, 2026 18:09
@Matt711 Matt711 force-pushed the bug/polars/allgather-ensure-insert-finished branch from 31ddf34 to a4b6e2f Compare May 15, 2026 18:14
@Matt711 Matt711 removed request for a team May 15, 2026 18:15
@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 15, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@Matt711
Copy link
Copy Markdown
Contributor Author

Matt711 commented May 15, 2026

/merge

@rapids-bot rapids-bot Bot merged commit 2a5d2f7 into rapidsai:release/26.06 May 15, 2026
87 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python May 15, 2026
@Matt711 Matt711 deleted the bug/polars/allgather-ensure-insert-finished branch May 15, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cudf-polars Issues specific to cudf-polars non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants