Add instructions on running PDS-H - #23025
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a PDS-H benchmark guide, updates cuDF Polars docs navigation, and changes benchmark-script thread defaults for non-CPU frontends. ChangescuDF Polars Benchmark Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/cudf/source/cudf_polars/index.md (1)
95-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPoint this link at the new docs page.
The benchmark entry already has a dedicated
benchmarkspage in the toctree, so linking readers to the GitHub source tree makes the navigation split between docs and source. A{doc}link keeps the entry point inside the rendered docs and matches the existing cross-link pattern incudf_polars.♻️ Proposed fix
-For more information on the benchmarks being run, see the PDS queries in the [cuDF GitHub repository](https://github.com/rapidsai/cudf/tree/main/python/cudf_polars/cudf_polars/streaming/benchmarks). +For more information on the benchmarks being run, see {doc}`benchmarks`.🤖 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 `@docs/cudf/source/cudf_polars/index.md` around lines 95 - 96, The benchmark reference in the docs still points to the GitHub source tree instead of the rendered documentation. Update the link in the cuDF Polars benchmark section to use the existing `benchmarks` doc page via the current cross-link pattern used in `cudf_polars`, so readers stay within the docs navigation.Source: Path instructions
🤖 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 `@docs/cudf/source/cudf_polars/index.md`:
- Around line 95-96: The benchmark reference in the docs still points to the
GitHub source tree instead of the rendered documentation. Update the link in the
cuDF Polars benchmark section to use the existing `benchmarks` doc page via the
current cross-link pattern used in `cudf_polars`, so readers stay within the
docs navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2574264d-0a27-4c92-8bf0-d2391bb6e3c0
📒 Files selected for processing (2)
docs/cudf/source/cudf_polars/benchmarks.mddocs/cudf/source/cudf_polars/index.md
|
|
||
| ### Run | ||
|
|
||
| Set these environment variables before running to match the configuration used for the published results: |
There was a problem hiding this comment.
This makes me nervous, since it'll inevitably go out of date. Maybe that's the right tradeoff, but could we at least link to those published benchmarks from here?
There was a problem hiding this comment.
I removed them from here. And added defaults for POLARS_MAX_THREADS=1 and OMP_NUM_THREADS=1 for the GPU benchmarks in the actual runner.
Also did the same for RAPIDSMPF_NUM_STREAMING_THREADS
github-actions
Bot
added
Python
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/benchmarks/pdsh.py`:
- Around line 45-47: The benchmark environment metadata is still looking for the
old RapidsMPF thread variable name, so it will miss the value now being set in
pdsh.py. Update the environment capture logic in utils.py to read the exact
RAPIDSMPF_NUM_STREAMING_THREADS name used by the benchmark setup, or include
both spellings if you need compatibility, so extra_info.environment records the
streaming-thread setting correctly.
🪄 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: a0cf1e03-9b3f-49ba-9f14-a49a48545342
📒 Files selected for processing (2)
docs/cudf/source/cudf_polars/benchmarks.mdpython/cudf_polars/cudf_polars/streaming/benchmarks/pdsh.py
✅ Files skipped from review due to trivial changes (1)
- docs/cudf/source/cudf_polars/benchmarks.md
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/benchmarks/pdsds.py`:
- Around line 41-44: The benchmark setup in pdsds.py is only mutating os.environ
in the driver, so connected dask/ray workers never receive the updated
RAPIDSMPF_NUM_STREAMING_THREADS, POLARS_MAX_THREADS, or OMP_NUM_THREADS values.
Update the startup flow around build_parser and the benchmark environment setup
to either propagate these settings to workers through the cluster/worker launch
path or reject --connect unless the remote workers are already configured, and
make sure extra_info.environment reflects the actual worker-side values rather
than only the driver’s overrides.
🪄 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: 6f8da132-9506-46d3-b538-cb54b127992c
📒 Files selected for processing (3)
python/cudf_polars/cudf_polars/streaming/benchmarks/pdsds.pypython/cudf_polars/cudf_polars/streaming/benchmarks/pdsh.pypython/cudf_polars/cudf_polars/streaming/benchmarks/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf_polars/cudf_polars/streaming/benchmarks/pdsh.py
|
@mroeschke In 9d31e22 and 90c2f79 I updated the instructions for running CPU benchmarks on CPU-only machines. |
|
I'm merging this as-is. But will follow-up with a PR to add pandas instructions and work out some broken assumptions in regards to running CPU benchmarks on CPU-only machines. The assumption is CUDA is installed (which of course it isn't). |
|
/merge |
4 participants
Description
Documentation for humans and agents to refer to for reproducing our benchmarks.
I punted on PDS-DS primarily because the data generation step is not as simple as PDS-H. I'll add instructions in a follow-up. First I want to close out #20587, so data generation really is just using duckdb's wrapper of
dsdgen, rather than having to convert to floats.#20587 should mostly be done, since the work we did for PDS-H with decimals should cover most of the cases we'll encounter for PDS-DS. FYI: I'm testing PDS-DS with decimals in #22848.
Contributes to #17640
Checklist