Use new rapidsmpf utilities to handle cancellation in bloom filter awaitables - #23293
Conversation
|
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. |
49557b5 to
5983b22
Compare
ea40732 to
a49d788
Compare
|
/ok to test a49d788 |
a49d788 to
20a480c
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesBloom filter updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py (1)
175-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd equivalent cancellation coverage for
BloomFilter.apply.The PR changes cancellation wiring for both async methods, but this regression test covers only
build. Add an apply test that blocks, cancels, and verifies its bloom-filter/input/output channels are closed.As per coding guidelines, “Add unit tests and unit benchmarks for code contributions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py` around lines 175 - 223, Add a cancellation regression test alongside test_bloom_filter_build_timeout_cancellation for BloomFilter.apply that blocks on its output, runs through run_actor_network until pytest-timeout cancellation, and verifies the apply bloom-filter, input, and output channels are closed by receiving None from each after cancellation. Reuse the existing Context, Communicator, channel, and message setup patterns while covering the apply-specific inputs and outputs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py`:
- Around line 175-223: Add a cancellation regression test alongside
test_bloom_filter_build_timeout_cancellation for BloomFilter.apply that blocks
on its output, runs through run_actor_network until pytest-timeout cancellation,
and verifies the apply bloom-filter, input, and output channels are closed by
receiving None from each after cancellation. Reuse the existing Context,
Communicator, channel, and message setup patterns while covering the
apply-specific inputs and outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 504c0e19-0c5b-4f3d-8ecd-8796594f861a
📒 Files selected for processing (7)
cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cudependencies.yamlpython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.pypython/cudf_streaming/pyproject.toml
TomAugspurger
left a comment
There was a problem hiding this comment.
The python changes look good, though I have one question about the test.
20a480c to
fba2372
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py`:
- Around line 204-220: Update the test around just_raise and bloom.build so the
RuntimeError is released only after a deterministic signal confirms the build
has reached its blocked ch_out-draining state, ensuring the test exercises
cancellation of that blocked worker. Rename the timeout-oriented comment to
describe exception-driven cancellation, and retain coverage for the resulting
exception group.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bfad6dab-76d5-47ec-a59b-6524a59a4e4e
📒 Files selected for processing (5)
cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cupython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cudf_streaming/cudf_streaming/bloom_filter.pyx
- cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
- cpp/libcudf_streaming/src/detail/device_bloom_filter.cu
fba2372 to
2892e1e
Compare
|
This needs new 26.08 packages to be released, but the publishing process appears broken |
2892e1e to
5b68103
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf_streaming/cudf_streaming/bloom_filter.pyx (1)
212-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument cancellation side effects in both public methods.
buildcancellation now shuts downch_in/ch_out, whileapplyalso shuts downbloom_filter; document these effects and theNonereturn value in the adjacent docstrings.As per coding guidelines, public API methods must document parameters, return values, and behavior.
Also applies to: 256-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_streaming/cudf_streaming/bloom_filter.pyx` around lines 212 - 216, Update the adjacent docstrings for both public methods, build and apply, to document their parameters, that cancellation returns None, and the cancellation side effects: build shuts down ch_in and ch_out, while apply additionally shuts down bloom_filter. Preserve the existing method behavior and describe these effects in the public API documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf_streaming/cudf_streaming/bloom_filter.pyx`:
- Around line 212-216: Update the adjacent docstrings for both public methods,
build and apply, to document their parameters, that cancellation returns None,
and the cancellation side effects: build shuts down ch_in and ch_out, while
apply additionally shuts down bloom_filter. Preserve the existing method
behavior and describe these effects in the public API documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73c8a4db-e422-4020-967a-e6f1d32512a8
📒 Files selected for processing (5)
cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hppcpp/libcudf_streaming/src/bloom_filter.cppcpp/libcudf_streaming/src/detail/device_bloom_filter.cupython/cudf_streaming/cudf_streaming/bloom_filter.pyxpython/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/libcudf_streaming/include/cudf_streaming/detail/device_bloom_filter.hpp
- python/cudf_streaming/cudf_streaming/tests/test_bloom_filter.py
- cpp/libcudf_streaming/src/bloom_filter.cpp
- cpp/libcudf_streaming/src/detail/device_bloom_filter.cu
ed3dee0 to
558135f
Compare
…cudf-streaming-cancellation
|
I merged |
…cudf-streaming-cancellation
|
/merge |
dbafeb3
into
NVIDIA:release/26.08
Description
To avoid cancellation in python leading to attempting to set a result on an already cancelled Future, use the new pattern introduced in rapidsai/rapidsmpf#1131
Checklist