Skill to compare performance of a branch or PR with main - #22725
Conversation
|
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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
mhaseeb123
added
2 - In Progress
|
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:
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a documented NVBench workflow for comparing a cuDF target branch against ChangescuDF perf-compare skill
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (2)
.agents/skills/perf-compare-cudf/scripts/compare.py (2)
120-122: ⚡ Quick winConsider adding error handling for malformed CSV files.
If a CSV file is malformed,
csv.DictReadermay raise exceptions that would crash the script with an unclear error message. Adding a try/except block would improve user experience.🛡️ Proposed fix to handle CSV errors gracefully
def read_rows(path: Path) -> list[dict]: - with open(path) as f: - return list(csv.DictReader(f)) + try: + with open(path) as f: + return list(csv.DictReader(f)) + except (OSError, csv.Error) as e: + raise SystemExit(f"Failed to read {path}: {e}")🤖 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 @.agents/skills/perf-compare-cudf/scripts/compare.py around lines 120 - 122, The read_rows function uses csv.DictReader without handling malformed CSVs; wrap the file read and csv.DictReader invocation in a try/except inside read_rows to catch csv.Error (and optionally UnicodeDecodeError/IOError), log or raise a clearer, contextual error mentioning the path, and return an empty list or rethrow a custom exception depending on caller expectations; update references to read_rows to handle the new return/error behavior if needed.
305-328: ⚡ Quick winConsider validating input directories in argument parsing.
The script doesn't validate that
--prand--mainpoint to existing directories. If they don't exist or are files, the error messages later will be unclear (e.g., "no CSVs found" when the directory doesn't exist).✅ Proposed fix to validate directories early
Add a custom type for directory validation:
+def existing_dir(path_str: str) -> Path: + path = Path(path_str) + if not path.is_dir(): + raise argparse.ArgumentTypeError(f"{path} is not a directory") + return path + def parse_args() -> argparse.Namespace: parser = argparse.ArgumentParser( description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter, ) parser.add_argument( - "--pr", required=True, type=Path, help="dir with PR-branch CSVs" + "--pr", required=True, type=existing_dir, help="dir with PR-branch CSVs" ) parser.add_argument( - "--main", required=True, type=Path, help="dir with main-branch CSVs" + "--main", required=True, type=existing_dir, help="dir with main-branch CSVs" )🤖 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 @.agents/skills/perf-compare-cudf/scripts/compare.py around lines 305 - 328, The parse_args function currently accepts --pr and --main as Path but doesn't validate they exist or are directories; add early validation so users get clear errors: either implement a custom argparse type/validator (used for the --pr and --main arguments) that checks path.exists() and path.is_dir() and raises argparse.ArgumentTypeError on failure, or after parser.parse_args() check args.pr and args.main and raise argparse.ArgumentTypeError (or call parser.error) if they are missing or not directories; update parse_args to reference the new validator so bad inputs fail fast and with a clear message.
🤖 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 @.agents/skills/perf-compare-cudf/scripts/compare.py:
- Around line 120-122: The read_rows function uses csv.DictReader without
handling malformed CSVs; wrap the file read and csv.DictReader invocation in a
try/except inside read_rows to catch csv.Error (and optionally
UnicodeDecodeError/IOError), log or raise a clearer, contextual error mentioning
the path, and return an empty list or rethrow a custom exception depending on
caller expectations; update references to read_rows to handle the new
return/error behavior if needed.
- Around line 305-328: The parse_args function currently accepts --pr and --main
as Path but doesn't validate they exist or are directories; add early validation
so users get clear errors: either implement a custom argparse type/validator
(used for the --pr and --main arguments) that checks path.exists() and
path.is_dir() and raises argparse.ArgumentTypeError on failure, or after
parser.parse_args() check args.pr and args.main and raise
argparse.ArgumentTypeError (or call parser.error) if they are missing or not
directories; update parse_args to reference the new validator so bad inputs fail
fast and with a clear message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8b8438e-8438-4aff-83ca-938bc8371fad
📒 Files selected for processing (2)
.agents/skills/perf-compare-cudf/SKILL.md.agents/skills/perf-compare-cudf/scripts/compare.py
mhaseeb123
added
3 - Ready for Review
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Sticking this in the same directory as other skills seems right, but beyond that I'm not the best person to review this.
One of the other ci-codeowners who's also a cuDF maintainer, like @vyasr or @bdice , would be bette.r
Think you all should also consider updating CODEOWNERS to have cuDF maintainers, not the build team, review changes in .agents/skills by default.
|
@jameslamb makes sense. I think the |
| ## Step 0: Devcontainer + build environment | ||
|
|
||
| Read and follow `/build-test-cudf` (skill at `.agents/skills/build-test-cudf/SKILL.md`) to: | ||
| - Confirm we are in a cudf devcontainer (username `coder`). If not, stop. |
There was a problem hiding this comment.
Do we really need to make this skill unusable outside of devcontainers? I wonder if we should be at least setting up our skill such that they have good default behaviors in devcontainers but will have some way to reference alternative options when used in other contexts.
There was a problem hiding this comment.
Told it to ensure either we are in cudf devcontainer or we have CUDA, required packages, and build helpers in place. Exit if neither available. I would not mind exiting if not in cudf devcontainer.
There was a problem hiding this comment.
I'm not sure that it's going to be clear to the agent what exactly the requirements are with these situations. Also some of the other details (e.g. latest in the build path) are devcontainer-specific. I think we should just make this skill only work in devcontainers for now, and if someone tries to use it outside of a devcontainer we can ask them to help generalize it.
There was a problem hiding this comment.
Just told it to run in devcontainer (like other skills) or stop and ask the user for instructions. Ok for now I think.
mhaseeb123
added
4 - Needs Review
vyasr
left a comment
vyasr
left a comment
There was a problem hiding this comment.
I think there are still some improvements to be made, but overall this looks good now. I'm approving, but please address the suggestions that make sense to you.
| - Before switching branches, stash unrelated user changes and record the stash name. If the target is the current WIP, keep changes applied for the target run, then stash them before switching to main. | ||
| - Check out the PR with: | ||
| ```bash | ||
| gh pr checkout <PR_NUMBER> --repo rapidsai/cudf | ||
| ``` |
There was a problem hiding this comment.
We can probably help the agent by more explicitly laying out the branching here. Checking out the PR is only necessary in the PR case, and is mutually exclusive with the "keep changes applied" case, right?
| ## 1. Prepare | ||
|
|
||
| - Record the starting branch, `git status --short`, and the exact target (current WIP or cudf PR). | ||
| - Before switching branches, stash unrelated user changes and record the stash name. If the target is the current WIP, keep changes applied for the target run, then stash them before switching to main. |
There was a problem hiding this comment.
Should we tell the skill to always run the target branch case first since if you're benchmarking WIP code then it places the stash/unstash at well-defined points in the workflow? Otherwise depending on the order the agent will have to decide when to stash and unstash, which introduces another point of failure.
|
|
||
| - Return to the starting branch, pop any stash you created, delete temporary branches, and confirm `git status` matches the starting state. | ||
| - Summarize chat with the headline result (regression, improvement, or within noise), relevant metrics, hardware used, branch SHAs, axis coverage, and generated files. | ||
| - Use this report shape for `COMPARISON.md`, adapting the metric columns to the benchmark. GPU time is always useful, but other metrics such as output file size, throughput, compression ratio, or memory usage are also of interest when they change significantly in target vs main. |
There was a problem hiding this comment.
Do you want it to overwrite the benchmarks results from previous runs? It might be helpful to ask it to deterministically produce a name e.g. benchmark_comparisons/"<<PR #>| WIP>_<date>_COMPARISON.json". A subdirectory would keep this more organized too and avoid polluting the repo root.
There was a problem hiding this comment.
It actually is smart enough not to overwrite results but I put specific instructions in 908a991
| ## Step 0: Devcontainer + build environment | ||
|
|
||
| Read and follow `/build-test-cudf` (skill at `.agents/skills/build-test-cudf/SKILL.md`) to: | ||
| - Confirm we are in a cudf devcontainer (username `coder`). If not, stop. |
There was a problem hiding this comment.
I'm not sure that it's going to be clear to the agent what exactly the requirements are with these situations. Also some of the other details (e.g. latest in the build path) are devcontainer-specific. I think we should just make this skill only work in devcontainers for now, and if someone tries to use it outside of a devcontainer we can ask them to help generalize it.
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Seems fine but I want some proof of it working as expected. Do you have PRs where you've tested this skill?
|
/merge |
This PR adds a new AI-agent skill to automatically compare the performance of branch or a PR against the `rapidsai/cudf/main` branch and produce a report Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: #22725
5 participants
Description
This PR adds a new AI-agent skill to automatically compare the performance of branch or a PR against the
rapidsai/cudf/mainbranch and produce a reportChecklist