Add the perf-smoke performance regression gate - #6864
Conversation
Greptile SummaryThis PR adds an advisory performance-regression gate, baseline storage and seeding machinery, benchmark identity contracts, reporting, and unit-test workflows.
Confidence Score: 4/5The PR should not merge until pull-request-controlled scripts are isolated from credentials that can forge comments and commit statuses; the reseed secret inheritance should also be narrowed. The workflow grants issues and statuses write access at workflow scope, then checks out and executes the pull request’s Python files on the host, including one invocation with Files Needing Attention: .github/workflows/perf-smoke-test.yaml, .github/workflows/perf-smoke-seed-baselines.yaml
|
| Filename | Overview |
|---|---|
| .github/workflows/perf-smoke-test.yaml | Adds the main gate orchestration, but pull-request-controlled scripts execute with issue-comment and commit-status write permissions. |
| .github/workflows/perf-smoke-seed-baselines.yaml | Adds serialized historical baseline seeding; secret scope is broader than necessary through the caller’s secrets inheritance. |
| tools/perf_smoke_test/aggregate.py | Aggregates artifacts, evaluates buckets, emits reports, and identifies under-filled baselines; its execution under a write-capable PR job creates the workflow security issue. |
| tools/perf_smoke_test/baseline_manager.py | Implements matching, append-only updates, worktree commits, and push retries; no publishable changed-path defect was established. |
| tools/perf_smoke_test/oracle.py | Implements hard-failure, fixed-floor, MAD, percentage-floor, and insufficient-window verdict logic with focused tests. |
| tools/perf_smoke_test/seed_baselines.py | Replays historical commits in Docker and publishes baseline records with ancestry verification and cache isolation. |
| tools/subprocess_runner.py | Adds subprocess phase classification and timeout handling without invoking commands through a shell. |
Sequence Diagram
sequenceDiagram
participant PR as Pull request / protected push
participant Gate as perf-smoke-test workflow
participant GPU as GPU benchmark jobs
participant Oracle as aggregate.py / oracle.py
participant Base as perf-baselines branch
participant GH as GitHub statuses/comments
PR->>Gate: Trigger workflow
Gate->>GPU: Fan out task/backend matrix
GPU-->>Gate: Upload normalized results
Gate->>Oracle: Aggregate artifacts and context
Oracle->>Base: Read matching rolling baselines
Base-->>Oracle: Eligible samples
Oracle->>GH: Publish per-task verdicts and summary
alt Protected develop push
Oracle->>Base: Append valid samples
Gate->>Gate: Reseed under-filled buckets
end
Reviews (1): Last reviewed commit: "Fold skew detection into aggregate" | Re-trigger Greptile
There was a problem hiding this comment.
Isaac Lab Review Bot
The advisory perf-smoke gate has a coherent producer, oracle, baseline, and reporting structure, but the proposed workflow currently contains concrete correctness and integration gaps. Seed workflow defaults make documented input modes unreachable, non-develop PRs can select the wrong fallback image, retry artifacts can be misattributed, fork reporting can fail on read-only tokens, configured window sizes are ignored, and crashed runs can still be scored from existing output. The stale-image classifier can also misclassify PR-introduced dependency API misuse.
- Design and architecture: The launch-config and runtime-contract partitioning, ancestry-aware append-only baselines, and separation of read-only aggregation from trusted baseline writes are sound. However, the stale-image exception is too broad: any missing symbol from an image-provided package is treated as environment skew, even when changed Isaac Lab source introduced an invalid package API call. That weakens the execution-health contract and should be tied to stronger evidence such as installed-versus-pinned version skew.
- API: No existing public Isaac Lab package API is changed, but the new workflow and configuration surfaces are internally inconsistent. Empty
tasksandbranchesinputs cannot reach their documented all-task and explicit-commit behaviors because expression fallbacks replace empty values, and the advertised baseline window settings ingate_config.jsonare ignored in favor of module constants. These contracts should either be implemented as documented or narrowed to reflect actual behavior. - Implementation: The benchmark-to-baseline path needs correction before relying on its results. Main and release PRs can fall back to the develop image because target-branch selection uses the synthetic event ref. Retry cleanup leaves the first attempt's timestamped runtime bundle available, while the oracle ignores nonzero exits and failure phases, allowing stale output from a failed run to be scored and appended. Aggregate status and comment writes also fail for fork PRs with read-only tokens. Fix target-branch image resolution, clear actual runtime bundles between attempts, make execution failure authoritative in the oracle, and tolerate unavailable reporting permissions.
Significant concerns. Posted 7 actionable findings inline.
Automated review; human maintainers own approval decisions.
| # A crash caused by the image lacking a symbol this source pins is a | ||
| # property of the image, not of the change under test, so it is | ||
| # reported loudly but never fails the PR. | ||
| skew = detect_dependency_skew(bench_result.stdout_tail) |
There was a problem hiding this comment.
🔵 Suggestion · Design Architecture — Skew heuristic can excuse PR defects
detect_dependency_skew classifies any missing name in an image-provided package as a stale image. Isaac Lab source that calls a nonexistent warp/newton API raises the same ImportError/AttributeError, so a genuine defect in the change under test clears has_hard_failure and is reported as an image problem. Narrow the rule, for example to failures raised while importing the image package, or compare pinned versus installed versions first.
Benchmark a matrix of tasks on the L40S pool for each PR, compare the result against a rolling baseline held on the perf-baselines branch, and post a per-task verdict. The gate is advisory: gate_config.json sets blocking to false, so it never fails a PR on its own. Turning it into an enforcing check is a separate, deliberate rollout. The gate was developed on perf-smoke/develop-staging. Running it there meant benchmarking three-week-old source inside a container built from current develop, which crashed every Newton task on an API rename the source predated. Landing it on develop removes that whole class of failure, because the source under test and the image now move together. Carried over from the staging work: per-allocation JIT cache roots so concurrent seeding cannot race on cache creation, a hard failure now always exits non-zero rather than being masked by advisory mode, and a stale-image guard that reports a crash as an out-of-date CI image when the log shows a symbol missing from an image-provided package. Isaac Lab source is mounted from the PR, so a missing symbol there is still a real defect and still fails. Left on staging: the runner-stability qualification and the image-era automation. Neither is reachable from develop without further work, and the era roller would have fired an image build and a full seeding run on the first develop push because no era manifest exists yet.
analyze_seed_variance.py only printed a run-to-run FPS spread table into the seeding job summary; nothing consumed its output. The underlying samples stay on perf-baselines, so the same spread can be recomputed whenever it is wanted.
environment_skew.py had a single caller. Moving the detector next to the code that reports it puts the whole stale-image path in one place, and its tests join the aggregate reporting tests they belong with.
The gate skips draft pull requests, but pull_request defaults to the opened, synchronize and reopened types. A PR opened as a draft therefore went unbenchmarked when it was marked ready and waited for an unrelated push. Subscribing to ready_for_review closes that window.
The workflow granted issues:write and statuses:write to every job. A same-repo pull request runs its own checked-out Python on the host, so config and validate held write scopes while running PR code that never calls the API, and bench held issues:write it does not use. Default to contents:read and grant each write scope on the one job that needs it. The reseed job also passed secrets: inherit to the seeding workflow, which declares only NGC_API_KEY. Pass that credential explicitly instead of handing over every secret the repository holds.
Oracle: a run could write a complete bundle and then die, and compare() judged it on FPS alone. It scored PASS and became eligible for the baseline, so the window could be learned from a failed run. Execution health is now checked before any measurement, matching the per-task commit status, which already treated a nonzero exit or any failure phase as unhealthy. Fork pull requests: the status and comment steps called the API under if: always(). A fork gets a read-only token whatever the permissions block requests, so those calls returned 403 and turned the job red for every external contributor. They are now skipped unless the head is the same repository; the verdict is still in the job summary. Benchmark retry: the cleanup removed perf_smoke_test_info.json, which build_bench_result writes later and so cannot exist yet. The real leftover is the first attempt's benchmark_runtime_*.json, which the normalizer globs, so a retry that died early reported the failed attempt's FPS under its own exit code. Image selection: the tag switched on GITHUB_REF_NAME, which on a pull_request is the synthetic <n>/merge ref and on a merge_group is a gh-readonly-queue ref. Every PR therefore selected latest-develop, so a PR into main or release/** measured develop's image and produced a runtime_contract_hash no baseline on those branches could match. The tag now follows the target branch. Seed inputs: `inputs.x || default` treats an explicitly empty value as unset, making the documented "empty = all tasks" and the commits and commit_branch modes unreachable behind an undocumented __ALL_TASKS__ sentinel. Both inputs now pass through, and the sentinel is gone. Gate config: min_baseline_samples and max_baseline_samples were surfaced by load_gate_config but never read, so editing them silently did nothing. Removed rather than threaded; the constants in gate_config.py remain the single source of truth. Workflow wiring is now covered by tests, since every defect above except the first lived in YAML that no module test could reach.
isaac-sim#6564 promoted the benchmark framework out of the internal namespace into isaaclab.benchmark and removed the old one. perf_runtime.py still imported the retired path, so every bench container died at module import before the app launched: all nine buckets reported HARD_FAILURE(phase=import) while the bench jobs themselves stayed green. Every symbol the driver uses is available unchanged at the new path -- BaseIsaacLabBenchmark, BenchmarkMonitor, the builders/capture/stepping submodules and schema.StartupTime -- so this is a namespace move, not an API migration. Six docstring references carried the retired path too. perf_runtime.py is the only gate module that imports Isaac Lab, and it runs only inside the CI container, so nothing in this suite or in a local check could see the breakage. test_framework_imports.py resolves each framework import statically against source/ in the current checkout, which fails on the machine that rebases instead of on the GPU runner an hour later.
gate_config.json, the workflow comments and the PR description all said the gate never fails a pull request on its own, but aggregate.py returned 2 on any HARD_FAILURE regardless of the blocking flag, and the aggregate job has no continue-on-error. An nvcr.io outage or an image drift unrelated to the change under test therefore painted a red check on somebody else's PR. The exit code now answers "did the gate run?", never "what did the gate conclude?". In advisory mode every verdict exits 0; blocking:true is the rollout step that makes HARD_FAILURE exit 2 and BLOCK exit 1. Gate malfunctions -- no bench artifacts at all, an unreadable baseline branch, a failed baseline push -- stay fatal in both modes, because those mean no trustworthy verdict was produced. Advisory must mean "does not fail the PR", not "says nothing", so the verdict now travels as a step output and drives the perf-smoke-test commit status directly instead of being inferred from the step outcome. A missing verdict reports as such rather than as success. Two things also stopped the diagnostics reaching anyone. The step runs under bash -e, so a nonzero aggregate exit aborted it before the job summary was written; the call is now wrapped and the status re-raised afterwards. And fork pull requests get a read-only token, so the comment and statuses are skipped -- they now get an explicit notice pointing at the job summary and the artifact.
The push run is the only thing that appends to perf-baselines, but every push shared one concurrency group with cancel-in-progress, so the next merge killed the run that would have published. develop lands about 10 commits a day with a median gap of 45 minutes against a perf run that takes over an hour: measured over the last 100 develop commits, only 37% of pushes had enough clearance to finish, and 18 of 99 gaps were under 5 minutes. The window could never reach MIN_BASELINE_SAMPLES, so every bucket would sit on NO_BASELINE or INSUFFICIENT_WINDOW indefinitely and the gate would never become useful. Pushes now key their group by commit, so each one runs to completion. Pull requests keep cancel-in-progress, which is the half that is actually wanted: a stale run for an outdated push is worth superseding.
An adversarial audit of the previous commit found it had introduced a silent green. _verdict_outputs branched on has_hard_failure and has_block alone, and has_hard_failure is deliberately cleared for crashes excused as CI-image skew, so a run in which all nine buckets crashed at import reported overall_verdict=PASS, status_state=success, "no meaningful performance regression detected" -- an affirmative claim over measurements that never happened. The same fell out of a partial matrix, since aggregate only bails when there are zero artifacts. Reproduced with the suite's own newton SolverNotifyFlags fixture, which is the incident this excuse exists for. The verdict now comes from the rows. Any HARD_FAILURE row keeps the run out of PASS whether or not it was excused, and the description says nothing was measured rather than that nothing regressed. A shortfall against the expected bucket count reports how many buckets are missing instead of grading the survivors; the count comes from tasks.json and the check disables itself if that cannot be read, so it can never fail a run by itself. The import guard had the mirror-image gap: _FRAMEWORK_ROOTS was a hard-coded tuple, and "isaaclab_tasks" is not "isaaclab", so the four imports that bring in setup_preset_cli and resolve_task_config were never checked. Framework roots are now derived from the source tree.
The previous commit taught _verdict_outputs about missing buckets but not _build_summary_markdown, which is what produces verdict_summary.md -- and that one file is both the job summary and the sticky PR comment. The two surfaces are computed by different code paths, so a run where 8 of 9 buckets reported put a red status reading "only 8 of 9 buckets reported a result" directly above a comment headlined "No meaningful performance regressions detected ... 0 benchmark failures". A reviewer reading the comment would conclude the red check was gate noise and merge a change whose one regressing task was never measured. Reproduced end to end against the real nine-bucket matrix. Coverage is now computed once, in _coverage(), and passed to both surfaces, so they cannot disagree by construction. The comment headline ranks a shortfall above skew, BLOCK and WARN -- the rows that did arrive may all be clean, but the change is not covered, so no all-clear may be printed -- and the count line names the buckets that never reported rather than only counting them. Counts are over distinct buckets, so a duplicated artifact cannot inflate the total, and an unreadable tasks.json yields an empty result that can never invent a failure. The advisory banner claimed a red status always shows up "in this table", which was untrue for exactly this case; it now points at the overall result instead. The new tests drive the real aggregate.main() over the real matrix and compare both surfaces, which is the check that was missing: the previous tests exercised _verdict_outputs in isolation and could not have caught a divergence.
The verdict chain picks the most severe condition, so a run with a skew-excused crash on one bucket and a genuine BLOCK on another described itself as "CI image looks stale" and never mentioned the regression. Same for a BLOCK alongside a bucket that never reported. The status colour was right in both cases, but the description misattributed the cause -- the same failure mode the skew excuse already risks, and the one a reviewer acts on. The description is now additive: the most severe condition still sets the verdict, and a blocking-level regression is named alongside it.
3af72f1 to
0c74096
Compare
Adds a performance regression gate for pull requests. It benchmarks a fixed matrix of tasks on the L40S runner pool, compares each result against a rolling baseline stored on the
perf-baselinesbranch, and posts a per-task verdict as a sticky PR comment and per-task commit statuses.The gate is advisory.
gate_config.jsonsetsblocking: false, so it never fails a PR on its own. Making it an enforcing required check would be a separate rollout.How a run works
perf-smoke-test.yamlruns six jobs.configreads the Isaac Sim version fromconfig.yaml, andvalidateruns static checks ontasks.jsonso a malformed matrix fails on a free runner in seconds rather than after an hour of GPU time. Both are skipped for draft PRs.benchfans out one GPU job per task/backend bucket. Each job resolves and pulls the CI image, restores a Warp JIT cache keyed on the Warp version, runsperf_runtime.pyinside Docker with the repo bind-mounted, retries once on failure, normalizes the output intoperf_smoke_test_result.json, and posts a per-task commit status.aggregatedownloads every bench artifact, loads the matching baselines, runs the oracle, writes the summary comment, and uploads results to omni-github. It runs withalways()so a crashed bucket still gets reported instead of vanishing.baseline_updateappends the run's samples toperf-baselines, and only on pushes tomain,develop, or a release branch. Pull requests are strictly read-only against the baseline branch, so nothing a PR does can move the numbers it is judged against.reseedfills buckets that came out under-filled, by calling the seeding workflow for just those tasks at the pushed commit. Without it, a bucket that resets (say, after a launch-config change) would take five separate pushes to become useful again. It is self-limiting: once a bucket is full it stops being reported as under-filled, and seeding writes toperf-baselines, which triggers no workflow.How the verdict is decided
oracle.pyproduces one ofPASS,WARN,BLOCK, orHARD_FAILUREper bucket, and the most severe signal wins.A run is a
HARD_FAILUREif the launch configuration did not match what was requested, if the result file is missing, or if the reported FPS is missing or not positive. A run reporting no forward progress is a dead run rather than a slow one, so it fails regardless of what the baseline says.Otherwise the result is compared against the rolling window. The baseline contributes a median and a median absolute deviation, and the gate blocks when the measured FPS falls below
median - 4.0 x MADand the drop is at least 3 percent. The percentage floor exists so that a bucket with an unusually tight MAD cannot block on a fraction of a percent. Betweenmedian - 2.5 x MADand the block line the verdict isWARN. There is also a noise floor so that a very tight baseline does not make the thresholds hypersensitive.Buckets can additionally declare fixed FPS floors in
tasks.json. A crossed gating floor forces aBLOCKon its own; a reporting-only floor is recorded without changing the verdict.With no baseline, or fewer than five samples in the window, the verdict is
WARNand the reason is stated asNO_BASELINEorINSUFFICIENT_WINDOW. The gate says it does not know rather than implying a pass.What gets measured
Nine task/backend buckets:
Isaac-Cartpole-Directon physx and on newtonIsaac-Velocity-Flat-G1on physx and on newtonIsaacContrib-Factory-GearMesh-Directon physxIsaac-Reorient-Cube-Shadow-Camera-Benchmark-Directacross four physics/renderer combinations (physx and newton, each with the RTX renderer and the Newton renderer)perf_runtime.pydrives each task with random actions and no policy, discards warmup frames at the source, and reports steady-state mean FPS.Baselines and how samples are matched
Baselines live on the orphan
perf-baselinesbranch as append-only NDJSON, one file per GPU model, task, and backend.A stored sample is only eligible if it matches on GPU model, task, backend, target branch, launch-config hash, baseline epoch, benchmark contract hash, and runtime contract hash, and if its commit is an ancestor of the PR's merge base. Ancestry matters because a sample from an unmerged branch describes code that may never land. The window holds between 5 and 20 samples, nearest commits first.
The hashes are the reason a verdict can be trusted: change the number of environments, the seed, the frame count, or the CUDA/driver pairing, and the fingerprint changes, so old samples stop matching instead of being silently compared against a different experiment.
Keeping the environment stable
image_era.pyderives a key from the container's inputs and looks it up in a manifest onperf-baselines. When an immutable image is recorded for that key the gate pins it; otherwise it falls back to the publishedlatest-developtag, and builds only if that pull fails. Pinning matters because a nightly rebuild of a floating tag would otherwise shift the environment underneath a baseline that was measured in the old one.When something goes wrong
Benchmarks retry once, since a single flaky container start should not read as a regression.
A crash always exits non-zero. Advisory mode suppresses regression failures, not execution failures, so a benchmark that never ran cannot be mistaken for one that ran fine.
Skew detection in
aggregate.pyhandles one specific case that would otherwise blame the wrong person. The gate mounts PR source over a prebuilt image, so between a dependency pin landing and the next image publish, source can reference a symbol the installed package does not have yet. When a crash log shows a missing name from a package the image provides (newton,warp,isaacsim,mujoco, and so on), the summary reports a stale CI image and those tasks stay advisory. Isaac Lab's own source is mounted from the PR, so a missing symbol there is a real defect and still fails.Workflows
perf-smoke-test.yamlis the gate itself, described above.perf-smoke-seed-baselines.yamlwalks a slice of a branch's history, re-runs each commit's own benchmark, and appends the results, so a new bucket can be filled without waiting for five organic pushes. It runs on manual dispatch, defaulting to a dry run, and is called by the gate's reseed job. It resolves its image through the same code path the gate uses, so seeded samples and live runs share an environment.perf-smoke-unit-tests.yamlruns the gate's own tests onubuntu-latest.The Python
Under
tools/perf_smoke_test/, roughly by role:oracle.py,gate_types.py,gate_config.pybaseline_manager.py,verify_baselines.py,seed_baselines.pybackend_identity.py,gpu_identity.py,launch_config.py,runtime_contract.py,contracts.py,hashing.py,image_era.pyperf_runtime.py,build_bench_result.py,benchmark_result_adapter.py,write_launch_config.pytasks.json,task_config.py,tasks_to_ci_matrix.py,validate_tasks.py,gate_config.jsonaggregate.py,omni_github.py,github_gate_context.pytools/subprocess_runner.pyclassifies which phase a failed subprocess died in, which is what lets a config mismatch be told apart from a crash mid-benchmark.Tests
72 tests covering the oracle's verdicts and thresholds, baseline matching and ancestry, contract hashing, seeding and cache isolation, skew detection, and the aggregate reporting path.
They are pure Python: no GPU, no simulator, no Isaac Lab install.
tools/perf_smoke_test/pyproject.tomlmakes pytest root there so the isaaclab-importingtools/conftest.pyabove it is not loaded, which is the same arrangementtools/skills/andtools/changelog/already use.What to know before merging
It runs on every PR into develop from day one. The trigger list already covers
pull_request: [main, develop, release/**], so that is real L40S pool time on every PR. More of a capacity question than a code question.Baselines start empty. Every sample on
perf-baselinestoday is stampedtarget_branch: perf-smoke/develop-staging, and matching requires an exact target-branch match, so none carry over. All nine buckets begin with no baseline and reportNO_BASELINEorINSUFFICIENT_WINDOWuntil five samples accumulate, which happens on its own fromdeveloppushes. It sorts itself out after a few, but the gate is not useful on day one.Six of the nine buckets have inert thresholds. Every Newton bucket and three camera buckets carry a
0.0hard floor, which can never fire. Harmless while the gate is advisory, but they need real values before it could block anything.There is no runner-stability verdict yet. The PhysX buckets produced clean samples on July 28; the Newton buckets never ran, because the staging branch had drifted 216 commits behind and its source called
newton.solvers.SolverNotifyFlags, renamed toModelFlagsin the meantime. That was the frozen branch showing its age rather than a problem with the gate, and it does not arise ondevelop, where source and image move together. Landing here rather than on staging is what retires that failure mode. This supersedes #6863, which carried the same fixes against staging.Test plan
python3 -m pytest tools/perf_smoke_test/source/developpush runs the gate and begins filling baselines