Replace --executor with extended --frontend choices in cudf-polars benchmarks - #22504
Conversation
wence-
left a comment
There was a problem hiding this comment.
I would have expected also just migrating the contexts of utils_new_frontends.py into utils.py?
Keeping |
0f56980 to
b99d3f5
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSwitches benchmark selection from an executor field to a typed ChangesFrontend-based execution model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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: 3
🤖 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/benchmarks/utils_new_frontends.py`:
- Around line 1419-1424: Assertion arises because _consolidate_logs assumes a
1:1 positional match between grouped trace batches and run_records; instead
group traces by record.iteration and allow missing groups. Modify
_consolidate_logs to parse all trace Scope entries into groups keyed by
record.iteration (use the iteration field on records and any iteration metadata
on Scope logs), then when iterating run_records (including FailedRecord), look
up the corresponding trace group by iteration and tolerate a missing entry
rather than asserting counts; update any logic that relied on positional zipping
to use record.iteration-based lookups so that failed iterations with no traces
do not cause AssertionError.
- Around line 914-917: The polars-cpu validation uses the wrong execution path:
inside the match on args.baseline (the branch handling "polars-cpu") replace the
plain q.collect() call with the streaming invocation
q.collect(engine="streaming") so the baseline uses the same streaming engine as
the polars-cpu frontend; update the case handling for "polars-cpu" (the branch
referencing q.collect()) to call q.collect(engine="streaming") and keep the
existing assignment to expected.
- Around line 2022-2035: Add a validation that rejects --debug for CPU
frontends: if run_config.debug is true and run_config.frontend is in
_CPU_ENGINES, raise a ValueError with a clear message like "--debug is not
supported with --frontend {run_config.frontend}; debug mode only applies to GPU
frontends (in-memory, dask, ray, spmd)". Place this check alongside the existing
validations that reference run_config.collect_traces and
run_config.validation_method so execute_query() and run_duckdb() no longer
silently ignore the flag.
🪄 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: 1ac9f079-fd9c-463f-866d-8b8e22d31052
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
|
Thanks @wence-, I merged with |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/benchmarks/utils_new_frontends.py`:
- Around line 1074-1076: The current validation-summary gating uses
args.validate so validation-run metadata (like validation_failures) happens but
the human-readable summary is suppressed for --validate-directory; change the
condition on the validation-summary branch to check run_config.validation_method
is not None instead of args.validate (keep the existing frontend exclusion check
run_config.frontend not in _CPU_ENGINES), i.e., make the if that currently reads
"if args.validate and run_config.frontend not in _CPU_ENGINES:" use
"run_config.validation_method is not None" to ensure summaries are printed for
directory-based validation while preserving CPU-engine gating and referencing
args.summarize and run_config.summarize() as before.
- Around line 819-823: Add the runtime import for StreamingEngine to the
existing optional cudf-polars import try/except block (the block that currently
handles the optional cudf-polars/rapidsmpf frontend imports) so StreamingEngine
is available at module level, then remove the local function-level imports of
StreamingEngine (the ones inside the functions that check isinstance(engine,
StreamingEngine)); keep the isinstance(engine, StreamingEngine) check and the
call to engine.global_statistics(clear=True).to_dict() as-is but rely on the
module-level import to avoid PLC0415 violations.
🪄 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: 32bb0f48-9951-4831-baad-39bc85b171b2
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
…enchmark-frontend-argument
|
/merge |
…s benchmarks (NVIDIA#22504) Unify the engine/executor arguments into `--frontend`. ```bash python python/cudf_polars/cudf_polars/experimental/benchmarks/pdsh.py \ --frontend in-memory \ --path /datasets/datasets/tpch-rs/scale-10 \ --output pdsh-output.json \ --iterations 2 \ 1 ``` Swap `--frontend in-memory` for `polars-cpu`, `duckdb`, `dask`, `ray`, or `spmd` to switch backends; everything else stays the same. ## Breaking Changes ### CLI / argparse * `--engine` removed from `pdsh.py` / `pdsds.py` * `--executor` removed * `--frontend` values are now: * `duckdb` * `polars-cpu` * `spmd` * `ray` * `dask` * `in-memory` * `--baseline` value renames (and restricted choices) * `--baseline cpu` → `--baseline polars-cpu` * Default changed from `"duckdb"` to `"duckdb"` (same semantics, new spelling) * `--collect-traces` * `--collect-traces` now raises `ValueError` when combined with: * `--frontend polars-cpu` * `--frontend duckdb` * Previously, these combinations were silently accepted as a no-op ### Output JSON schema * `executor` * Removed from the serialized run config as a consequence of removing `--executor` * `--collect-traces --frontend in-memory` * Runs now populate `records[*].traces` * Previously, traces were empty or missing * This is a bug fix, but downstream consumers that assumed `"in-memory"` runs never produced traces will now observe new data Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Tom Augspurger (https://github.com/TomAugspurger) URL: NVIDIA#22504
Description
Unify the engine/executor arguments into
--frontend.python python/cudf_polars/cudf_polars/experimental/benchmarks/pdsh.py \ --frontend in-memory \ --path /datasets/datasets/tpch-rs/scale-10 \ --output pdsh-output.json \ --iterations 2 \ 1Swap
--frontend in-memoryforpolars-cpu,duckdb,dask,ray, orspmdto switch backends; everything else stays the same.Breaking Changes
CLI / argparse
--engineremoved frompdsh.py/pdsds.py--executorremoved--frontendvalues are now:duckdbpolars-cpuspmdraydaskin-memory--baselinevalue renames (and restricted choices)--baseline cpu→--baseline polars-cpu"duckdb"to"duckdb"(same semantics, new spelling)--collect-traces--collect-tracesnow raisesValueErrorwhen combined with:--frontend polars-cpu--frontend duckdbOutput JSON schema
executor--executor--collect-traces --frontend in-memoryrecords[*].traces"in-memory"runs never produced traces will now observe new data