Skip to content

Asv benchmarks in ci#1699

Open
coreyjadams wants to merge 6 commits into
mainfrom
asv-benchmarks-in-ci
Open

Asv benchmarks in ci#1699
coreyjadams wants to merge 6 commits into
mainfrom
asv-benchmarks-in-ci

Conversation

@coreyjadams

Copy link
Copy Markdown
Collaborator

This PR will run our ASV benchmarks nightly in CI.

It doesn't track over time, yet, but it will generate the plots for perf reasons at least.

Description

Checklist

Dependencies

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

@copy-pr-bot

copy-pr-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a new nightly GitHub Actions workflow (github-nightly-benchmarks.yml) that runs the ASV benchmark suite on a GPU H100 runner, publishes an HTML dashboard, generates functional benchmark plots, and uploads all artifacts without persisting history across runs.

  • The workflow correctly reuses warm uv/JIT caches from the main nightly, uses --python=same to avoid CUDA environment mismatches, and gracefully handles asv's exit-code-2 partial-failure case.
  • A push trigger on the development branch was intentionally added for testing but annotated "REMOVE this push trigger before merging" — it was not removed before filing the PR.
  • Minor hardening gaps: safe.directory uses an unnecessary wildcard, and post-failure publish/upload steps run unconditionally even when asv run exits with a fatal infrastructure error.

Important Files Changed

Filename Overview
.github/workflows/github-nightly-benchmarks.yml New nightly workflow for ASV benchmarks; well-commented and structured, but the push trigger explicitly marked for removal before merging was not removed, and minor hardening issues exist around the safe.directory wildcard and post-failure publish steps.

Reviews (1): Last reviewed commit: "Merge branch 'main' into asv-benchmarks-..." | Re-trigger Greptile

Comment on lines +49 to +52
# REMOVE this push trigger before merging.
push:
branches:
- asv-benchmarks-in-ci

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Push trigger left in — explicitly marked for removal

The push block (lines 49–52) carries an inline comment saying "REMOVE this push trigger before merging." It fires on every push to asv-benchmarks-in-ci, burning a GPU H100 runner slot and artifact storage each time. If that branch persists after the merge, the trigger will keep firing on any future pushes to it.

Comment on lines +169 to +170
git config --global --add safe.directory "$GITHUB_WORKSPACE"
git config --global --add safe.directory '*'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Overly-broad safe.directory bypass

git config --global --add safe.directory '*' disables git's ownership guard for every directory on the runner, not just $GITHUB_WORKSPACE. The first line already covers the specific workspace path; the wildcard is unnecessary. Narrowing to $GITHUB_WORKSPACE alone is sufficient for asv's needs.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +236 to +239
- name: Publish ASV HTML report
if: ${{ !cancelled() }}
run: |
uv run --no-sync asv publish

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Publish and upload steps run even on fatal benchmark infrastructure failures

if: ${{ !cancelled() }} is true even when the "Run ASV benchmarks" step exits with a fatal non-2 code. In that case asv publish runs against an empty .asv/results/ directory. Adding && steps.run-benchmarks.outcome != 'failure' (or giving the step an id and checking its outcome) would avoid publishing vacuous artifacts.

@ktangsali ktangsali left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just added a minor comment about cuda versions.

name: ASV Benchmarks
runs-on: linux-amd64-gpu-h100-latest-1
container:
image: nvidia/cuda:12.8.1-cudnn-devel-ubuntu24.04

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, any particular reason to keep it at cuda 12.8 instead of the latest one?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I'm scared, thats' why 😅

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right though we should update our CI to cuda13. We should do it in one swoop, but it should probably go separately from this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants