Use nvbench_compare_robust in benchmark workflow - #10636
Use nvbench_compare_robust in benchmark workflow#10636oleksandr-pavlyk wants to merge 15 commits into
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. |
This change makes sure that the same version of NVBench is used when building reference and compare benchmarks (previously, pinned SHAs from respective branches were used and those could be difference). The default is to use top-of-tree NVBench, but this can be controlled via script arguments.
c293017 to
346e96c
Compare
|
/ok to test 346e96c |
|
/ok to test 346e96c |
|
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
WalkthroughChangesNVBench benchmark integration
Assessment against linked issues
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I triggered the benchmark compare workflow manually: https://github.com/NVIDIA/cccl/actions/runs/30951999074 |
Resolve NVBench main once before benchmark builds and pass the pinned SHA through to both CUB build trees. Install cuda-bench compare dependencies in a pre-AWS benchmark venv, use that venv for compare execution, and remove GH_TOKEN from the benchmark container. Also add an opt-in credential drop in compare_paths.sh so the workflow can clear AWS/sccache credential state after CUB builds and before benchmark execution, comparison, and Python benchmark environment setup.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 036b298e-399e-4ec6-b277-c3a90da04c8b
📒 Files selected for processing (4)
.github/workflows/bench.ymlci/bench/README.mdci/bench/compare_paths.shcmake/CCCLGetDependencies.cmake
…port Output default `--display intervals` and, additionally, `--display simple`, `--display explain` and legacy report.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ci/bench/compare_paths.sh (3)
443-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the env-var branch skips the
-xcheck that both fallbacks apply. The workflow always exportsCCCL_BENCH_LEGACY_COMPARE_BIN=/bench-compare-venv/bin/nvbench-compare-legacy(.github/workflows/bench.yml:373-393). If that entry point is missing in the installedcuda-bench[compare]version, every target gets a rc=127 legacy report block in the summary instead of the legacy section being omitted. Gate it:- if [[ -n "${CCCL_BENCH_LEGACY_COMPARE_BIN:-}" ]]; then - printf "%s" "${CCCL_BENCH_LEGACY_COMPARE_BIN}" - return 0 + if [[ -n "${CCCL_BENCH_LEGACY_COMPARE_BIN:-}" ]]; then + if [[ -x "${CCCL_BENCH_LEGACY_COMPARE_BIN}" ]]; then + printf "%s" "${CCCL_BENCH_LEGACY_COMPARE_BIN}" + return 0 + fi + echo "CCCL_BENCH_LEGACY_COMPARE_BIN is set but not executable: ${CCCL_BENCH_LEGACY_COMPARE_BIN}" >&2 + return 1 fi
710-740: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the display list
intervals simple explainis now hardcoded in four places (here,run_python_compare_targetline 479, andwrite_summarylines 881 and 903). Define it once, for examplereadonly COMPARE_DISPLAYS=(intervals simple explain), and iterate over it. That keeps report generation and summary rendering from drifting apart.
816-818: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: use
-sinstead of-f. A comparison that exits 0 with no stdout leaves a zero-byte report, and the summary then contains an empty collapsible block that still counts towardreports_emitted.- if [[ ! -f "${compare_report_file}" ]]; then + if [[ ! -s "${compare_report_file}" ]]; then
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 045528b6-525d-4eea-9724-2e6a7705e48d
📒 Files selected for processing (5)
.github/workflows/bench.ymlci/bench.template.yamlci/bench.yamlci/bench/README.mdci/bench/compare_paths.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/bench.yml
- ci/bench/README.md
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…Lo / Ce / Hi] Use of vertical pipe symbol | messes up markdown rendering
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Require an explicitly configured legacy compare binary to be executable before using it, so missing optional entry points do not produce rc=127 report blocks. Centralize the robust compare display list and skip empty compare report files when rendering the benchmark summary.
What worries me is that even with the robust comparison script, comparing two git SHAs without benchmark changes results in many AMBG runs. I would have hoped for a clearer message that the performance before and after is the same. |
Avoid process substitution in compare runners so report files are fully written before emptiness checks and summary generation. Route the plain and PYTHONPATH compare paths through a shared helper to keep capture behavior consistent.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nvbench_compare_args is passed to robust compare script nvbench_compare_legacy_args is passed to legacy compare script
This helps avoid skipping of tests after multiple warm-up runs are added "skip_reason": "Warmup time did not meet skip_time limit: 6.048us < 15.000us."
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 2h 18m: Pass: 100%/534 | Total: 5d 09h | Max: 2h 06m | Hits: 100%/640656See results here. AI failure analysis1. Merged RAPIDS environment contains incompatible rapids-logger pins · 1 jobExplanation: The RAPIDS build stops while creating its shared conda environment, before any library is configured or compiled. The generated environment simultaneously requires mutually exclusive `rapids-logger` 0.2 and 0.3 versions. Evidence: Build RAPIDS (optional) / rmm ucxx kvikio rapidsmpf cudf cudf_kafka, step 6 Root cause: The job clones the selected RAPIDS repositories from their current upstream `main` branches and merges their dependency files; those snapshots are temporarily inconsistent about the required `rapids-logger` minor version. The log does not preserve dependency provenance, so it does not identify which cloned repository contributes each constraint; the PR changes only benchmark infrastructure and do not modify this RAPIDS setup. Sources: .github/workflows/build-rapids.yml:96, ci/rapids/post-create-command.sh:141. Suggested next steps: Reproduce only environment generation with `RAPIDS_LIBS='rmm ucxx kvikio rapidsmpf cudf cudf_kafka' .devcontainer/launch.sh -d -c 13.3 -H rapids-conda -- ./ci/rapids/rapids-entrypoint.sh`, then inspect each generated repository conda YAML to identify both pins. Pin the out-of-sync repository to a compatible branch/commit through its `RAPIDS_<repo>_GIT_REPO` workflow variable, or update the upstream dependency pin so every selected repository uses the same `rapids-logger` series. Copy this prompt into a coding agentJobs: |
Several issues with that run:
Since then the generated summary was changed:
Please see: https://github.com/NVIDIA/cccl/actions/runs/31379861590 |
|
As noted by @bernhardmgruber there are plenty of undecided comparison, so I looked into some of those.
Here are visualization for
|
|
@bernhardmgruber @gevtushenko believes that for the 4 rows visualized above the decisions should have been FAST, FAST, FAST, SAME because the distribution in 3 cases looks same shape but shifted to the left. This will need to be addressed in a follow-up work though as this PR gets in a metric (# of AMBG) we want to optimize. |
Script may be available and set, but its runtime dependencies are not guaranteed to be installed even if the script itself is available
This change is to be opened as a stand-alone PR
⏱️ CCCL compile-time benchmark comparison: Public headers compile-time benchResult: 1 regression row(s), 6 improvement row(s) above threshold.
Artifacts: reports and traces Direct file processing
🔴 Direct file processing — Regressions
🟢 Direct file processing — Improvements
|
4 participants
Description
This PR switch
bench.yamlworkflow to save benchmark data with binary sidecars storing bulk data for durations and frequencies.Use of
nvbench-comparescript is replaced with use ofnvbench-compare-robustwhich takes bulk data into account.The new script introduces
AMBG(ambiguous/undecided) classification, designated for comparisons where making determination betweenFAST/SLOW/SAMEis difficult.Closes #10635
Checklist