Capture command line on pds-h benchmark runner - #22508
Merged
Merged
Conversation
To make it easier to see how a particular benchmark was run, this PR captures `sys.argv` and includes it in the output.
github-actions
Bot
added
Python
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughRunConfig now records the full benchmark command line and can capture a configurable comma-separated list of environment variables into extra_info.environment; values come from CLI args and sys.argv and are serialized with the run metadata. ChangesBenchmark invocation metadata
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/benchmarks/utils_new_frontends.py`:
- Line 547: The code currently persists raw command-line arguments via
shlex.join(sys.argv) (and later serializes it) which can leak secrets; implement
a sanitizer function (e.g., sanitize_args(argv)) and call it wherever
shlex.join(sys.argv) or the argv serialization occurs to mask values of
sensitive flags (common patterns like --token, --password, --secret,
--access-key, -p, --api-key and any flags listed in an allowlist/denylist)
before joining/serializing; update the usages that reference
shlex.join(sys.argv) and the later serialization point to use
sanitize_args(sys.argv) so only redacted CLI strings are stored.
🪄 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: d63fbf69-0857-4150-b13b-adb6dc334df2
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
TomAugspurger
added
improvement
mroeschke
approved these changes
May 14, 2026
Matt711
approved these changes
May 14, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py (1)
553-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact sensitive CLI values before persisting run metadata.
Line 553 captures raw
sys.argv, and Line 579 writes it to output unchanged. This can leak tokens/passwords passed via CLI flags into benchmark artifacts.Suggested patch
+def _redact_argv(argv: list[str]) -> list[str]: + sensitive_flags = { + "--connect", + "--extra-info", + "--token", + "--password", + "--secret", + "--api-key", + "--access-key", + "-p", + } + redacted: list[str] = [] + redact_next = False + for arg in argv: + if redact_next: + redacted.append("[REDACTED]") + redact_next = False + continue + if "=" in arg: + key, _ = arg.split("=", 1) + if key in sensitive_flags: + redacted.append(f"{key}=[REDACTED]") + continue + if arg in sensitive_flags: + redacted.append(arg) + redact_next = True + continue + redacted.append(arg) + return redacted + ... - command_line=shlex.join(sys.argv), + command_line=shlex.join(_redact_argv(sys.argv)),Also applies to: 579-579
🤖 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/benchmarks/utils_new_frontends.py` around lines 553 - 554, The code currently stores raw CLI arguments via shlex.join(sys.argv) into the run metadata (the command_line field) and later writes it unchanged; replace that with a sanitized value by implementing and calling a redaction helper (e.g., redact_cli_args(argv)) that strips or masks values for sensitive flags (tokens, passwords, keys, secrets) before assigning to command_line and before any output write. Update the places using shlex.join(sys.argv) and the write path for run metadata to call this helper so sensitive flag values are masked consistently.
🤖 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 443-447: The code in __post_init__ assumes extra_info and
extra_info["environment"] are dicts and naively iterates capture_env_vars split
by comma; validate and coerce before writing: ensure self.extra_info is a dict
(replace non-dict inputs with {}), ensure self.extra_info["environment"] is a
dict (if present but not a dict, overwrite with {}), and iterate over [v.strip()
for v in self.capture_env_vars.split(",") if v.strip()] to avoid keys with
leading spaces or empty names; then assign os.environ.get(var) into
self.extra_info["environment"][var]. Use the existing symbols __post_init__,
extra_info, capture_env_vars, and extra_info.setdefault/environment to locate
where to apply these checks and trimming.
---
Duplicate comments:
In
`@python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py`:
- Around line 553-554: The code currently stores raw CLI arguments via
shlex.join(sys.argv) into the run metadata (the command_line field) and later
writes it unchanged; replace that with a sanitized value by implementing and
calling a redaction helper (e.g., redact_cli_args(argv)) that strips or masks
values for sensitive flags (tokens, passwords, keys, secrets) before assigning
to command_line and before any output write. Update the places using
shlex.join(sys.argv) and the write path for run metadata to call this helper so
sensitive flag values are masked consistently.
🪄 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: fadb1c04-8ab5-4246-b063-81960f0c5753
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
madsbk
approved these changes
May 15, 2026
Matt711
reviewed
May 15, 2026
…ew_frontends.py Co-authored-by: Mads R. B. Kristensen <madsbk@gmail.com>
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/benchmarks/utils_new_frontends.py`:
- Around line 445-447: The loop reassigns the loop variable `var` (in the block
iterating over `self.capture_env_vars.split(",")`), triggering Ruff PLW2901; fix
it by not rebinding the loop variable — iterate using a different name (e.g.,
`raw` or `env_var`) or use a generator that applies `.strip()` (e.g., `for
env_var in map(str.strip, self.capture_env_vars.split(","))`) and then use that
new name when assigning into `self.extra_info["environment"]`; update references
to use the new identifier instead of reassigning `var`.
🪄 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: fd227601-c72c-4604-bf5b-71ce07774de0
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py (2)
444-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
extra_info.environmentwrites against non-object JSON input.
__post_init__still assumesextra_infois a dict with dict-compatibleenvironment. Inputs like--extra-info '[]'or--extra-info '{"environment":"x"}'will crash or misbehave; also skip empty env var names after stripping.🤖 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/benchmarks/utils_new_frontends.py` around lines 444 - 448, The __post_init__ handling of extra_info assumes it's a dict and that extra_info["environment"] is a dict; update __post_init__ to validate and coerce types: if self.extra_info is not a dict, replace it with {} (or parse JSON safely and fall back to {}), ensure self.extra_info["environment"] is a dict before writing (e.g., if it's missing or not a dict, set it to {}), and when iterating self.capture_env_vars skip any empty names after strip() (ignore "" entries). Refer to the __post_init__ method, the extra_info field, and capture_env_vars to locate where to add these guards.
555-555:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact sensitive CLI arguments before persisting
command_line.Capturing and serializing raw
sys.argvcan leak secrets (tokens/passwords/keys) into benchmark artifacts. Please sanitize/redact known sensitive flags before joining.Also applies to: 581-581
🤖 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/benchmarks/utils_new_frontends.py` at line 555, Replace the direct serialization of sys.argv (command_line=shlex.join(sys.argv)) with a sanitized version: implement or call a helper (e.g., sanitize_argv or redact_sensitive_args) that walks the sys.argv list and redacts values for known sensitive flags (examples: --token, --password, --secret, --key and short forms like -p, -k) and flags whose values look like secrets, then use shlex.join(sanitized_argv) for command_line; update both occurrences (the command_line assignment at the shown spot and the similar one around line 581) and ensure the helper is deterministic and preserves non-sensitive args/order.
🤖 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 434-435: RunConfig's dataclass now requires command_line and
capture_env_vars which breaks callers; make these fields optional with safe
defaults (e.g., empty string) so the RunConfig constructor remains compatible.
Update the RunConfig dataclass declaration (the fields named command_line and
capture_env_vars) to provide default values (or mark them Optional with
defaults) so existing direct RunConfig(...) invocations continue to work without
changes.
---
Duplicate comments:
In
`@python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py`:
- Around line 444-448: The __post_init__ handling of extra_info assumes it's a
dict and that extra_info["environment"] is a dict; update __post_init__ to
validate and coerce types: if self.extra_info is not a dict, replace it with {}
(or parse JSON safely and fall back to {}), ensure
self.extra_info["environment"] is a dict before writing (e.g., if it's missing
or not a dict, set it to {}), and when iterating self.capture_env_vars skip any
empty names after strip() (ignore "" entries). Refer to the __post_init__
method, the extra_info field, and capture_env_vars to locate where to add these
guards.
- Line 555: Replace the direct serialization of sys.argv
(command_line=shlex.join(sys.argv)) with a sanitized version: implement or call
a helper (e.g., sanitize_argv or redact_sensitive_args) that walks the sys.argv
list and redacts values for known sensitive flags (examples: --token,
--password, --secret, --key and short forms like -p, -k) and flags whose values
look like secrets, then use shlex.join(sanitized_argv) for command_line; update
both occurrences (the command_line assignment at the shown spot and the similar
one around line 581) and ensure the helper is deterministic and preserves
non-sensitive args/order.
🪄 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: 31a06c23-233a-405d-990c-bc0e878a1aed
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/experimental/benchmarks/utils_new_frontends.py
Comment on lines
+434
to
+435
| command_line: str | ||
| capture_env_vars: str |
There was a problem hiding this comment.
Avoid breaking RunConfig constructor compatibility.
Adding required command_line and capture_env_vars fields to an exported dataclass introduces a breaking API change for any direct RunConfig(...) callers. Please give these fields safe defaults (e.g., empty strings) or add a compatibility path.
As per coding guidelines: python/**/*.{py,pyx}: Detect and flag API breaking changes to public methods/attributes without deprecation warnings.
🤖 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/benchmarks/utils_new_frontends.py`
around lines 434 - 435, RunConfig's dataclass now requires command_line and
capture_env_vars which breaks callers; make these fields optional with safe
defaults (e.g., empty string) so the RunConfig constructor remains compatible.
Update the RunConfig dataclass declaration (the fields named command_line and
capture_env_vars) to provide default values (or mark them Optional with
defaults) so existing direct RunConfig(...) invocations continue to work without
changes.
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
To make it easier to see how a particular benchmark was run, this PR captures
sys.argvand includes it in the output.