Add FusedScan and nxtx annotations for FusedScan and SplitScan - #22838
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a FusedScan IR node, records per-split paths on SplitScan, changes expand_scan_for_rank to emit SplitScan or FusedScan per rank, wraps delegated evaluations in NVTX ranges, and updates typing/wiring and tests to use the new node types. ChangesFused Scan IR Extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 9db0749 |
| __slots__ = ( | ||
| "base_scan", | ||
| "parquet_options", | ||
| "paths", |
There was a problem hiding this comment.
I'm noticing that we don't pass in the paths (or path maybe?) when we construct a SplitScan, even though each node is only mapped to a single file. I think this is because we pass in a paths argument to do_evaluate anyway.
I wonder if it would be clearer to use the same pattern for both FusedScan and SplitScan? More specifically, maybe we should pass in the paths subset to both or we shouldn't pass it in to either?
There was a problem hiding this comment.
Thinking about this a bit more, I’d probably suggest the explicit-paths version for both wrappers: SplitScan(base_scan, paths=[path], ...) and FusedScan(base_scan, paths=local_paths, ...). Then base_scan consistently means “the original scan/options template,” and the wrapper’s paths consistently means “the files assigned to this task.”
There was a problem hiding this comment.
They now both take paths now
There was a problem hiding this comment.
Cool - Sorry for adding more work for you here, but we probably need SplitScan.get_hashable() now too :)
There was a problem hiding this comment.
Thanks, I'm doing some more profiling. I think I like the idea of including the absolute paths in the annotations? I'll clean this PR up in a bit
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_polars/cudf_polars/streaming/io.py`:
- Around line 330-332: The NVTX annotation currently embeds raw path strings in
the message (nvtx_annotate_cudf_polars call with message=f"SplitScan: {paths[0]}
[...]") which can leak filesystem or URL details; update both occurrences (the
SplitScan annotation and the similar annotation around line 431) to redact paths
by using a safe summary such as os.path.basename(paths[0]) or a compact form
like f"{len(paths)} files, first={os.path.basename(paths[0])}" (or "<redacted>"
when basename is not available), ensuring no raw absolute paths or query strings
are included in the NVTX message while preserving useful context (count and
first-file name).
🪄 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: 7530b718-81b4-4cb1-9a70-da4c80b2afcb
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/streaming/io.pypython/cudf_polars/tests/streaming/test_scan.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf_polars/tests/streaming/test_scan.py
|
/merge |
…apidsai#22838) Mainly a refactor: PR adds `FusedScan` (`explicit is better than implicit`) in addition to nvtx annotations for `FusedScan` and `SplitScan`. <img width="1689" height="733" alt="image" src="https://github.com/user-attachments/assets/f6e0ad74-62a0-43f3-b986-1800bfcd1f36" /> Interesting enough, we're likely doing a lot of `SplitScans` so that is likely the path to optimize to get the most bang for buck for local and remote I/O work. xref rapidsai#22662 Authors: - Matthew Murray (https://github.com/Matt711) Approvers: - Richard (Rick) Zamora (https://github.com/rjzamora) URL: rapidsai#22838
Description
Mainly a refactor: PR adds

FusedScan(explicit is better than implicit) in addition to nvtx annotations forFusedScanandSplitScan.Interesting enough, we're likely doing a lot of
SplitScansso that is likely the path to optimize to get the most bang for buck for local and remote I/O work.xref #22662
Checklist