Align pdsh benchmarks and library defaults#22399
Align pdsh benchmarks and library defaults#22399TomAugspurger wants to merge 3 commits intorapidsai:mainfrom
Conversation
This updates the defaults in our pdsh benchmarks to match the defaults from the library. This will make it easier to understand what values are being used based just on the command run.
|
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. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughCLI argument defaults and help text are updated across benchmark and streaming configuration files to reflect actual runtime behavior. The ChangesCLI Configuration Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/experimental/rapidsmpf/frontend/options.py`:
- Around line 644-646: The help text for the --num-py-executors option
incorrectly names the env var as CUDF_POLARS__NUM_PY_EXECUTORS; update that help
string to the actual env var used at runtime,
CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS. Locate the option definition for
--num-py-executors in options.py (the field wired to
CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS) and replace the incorrect env var
reference in its help/description so the documentation matches the configured
environment variable.
🪄 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: e862b61a-eafd-41bc-98f1-cedbe61d240b
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.pypython/cudf_polars/cudf_polars/experimental/rapidsmpf/frontend/options.py
| Max workers for the Python ThreadPoolExecutor inside RapidsMPF. | ||
| Env: CUDF_POLARS__NUM_PY_EXECUTORS. | ||
| Built-in default: 1."""), | ||
| Built-in default: 8."""), |
There was a problem hiding this comment.
Fix --num-py-executors env var name in help text.
Line 645 currently references CUDF_POLARS__NUM_PY_EXECUTORS, but the field is wired to CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS (Line 313). The help text should match runtime behavior.
Suggested patch
help=textwrap.dedent("""\
Max workers for the Python ThreadPoolExecutor inside RapidsMPF.
- Env: CUDF_POLARS__NUM_PY_EXECUTORS.
+ Env: CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS.
Built-in default: 8."""),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Max workers for the Python ThreadPoolExecutor inside RapidsMPF. | |
| Env: CUDF_POLARS__NUM_PY_EXECUTORS. | |
| Built-in default: 1."""), | |
| Built-in default: 8."""), | |
| Max workers for the Python ThreadPoolExecutor inside RapidsMPF. | |
| Env: CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS. | |
| Built-in default: 8."""), |
🤖 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_polars/cudf_polars/experimental/rapidsmpf/frontend/options.py`
around lines 644 - 646, The help text for the --num-py-executors option
incorrectly names the env var as CUDF_POLARS__NUM_PY_EXECUTORS; update that help
string to the actual env var used at runtime,
CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS. Locate the option definition for
--num-py-executors in options.py (the field wired to
CUDF_POLARS__EXECUTOR__NUM_PY_EXECUTORS) and replace the incorrect env var
reference in its help/description so the documentation matches the configured
environment variable.
Description
This updates the defaults in our pdsh benchmarks to match the defaults from the library. This will make it easier to understand what values are being used based just on the command run.