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
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
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
The resolver read warp-lang from source/isaaclab/pyproject.toml, which does not pin it; the pin lives in the repo-root pyproject.toml. The sed therefore matched nothing and the step fell back to the literal string "unpinned", which every cache key in CI has carried since. The key consequently never varied with Warp, so a cache built under a different Warp version restored as a hit, and nothing could pre-create the versioned cache directory that the camera buckets fail to write. Read the root file and fail the step when the pin cannot be resolved, rather than silently degrading the key again.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
AI review — architecture and simplification
This review was generated by AI at the request of the reviewer. It focuses primarily on simplification, reuse of IsaacLab's existing benchmark/CI infrastructure, and the maintenance cost of the proposed design.
Overall finding
I do not think this should land in its current shape. The underlying requirement is useful and comparatively small:
- Define a task/backend matrix.
- Run the supported runtime benchmark.
- Read one FPS metric from its RuntimeBundle.
- Compare it with an authoritative baseline.
- Publish one visible result.
- Record trusted target-branch measurements for future comparisons.
The PR instead adds a largely self-contained performance-CI platform: 10,017 lines across 41 new files, with no deletions.
| Area | Added lines |
|---|---|
| Baseline database, historical seeding, verification, and image eras | 2,023 |
| Oracle, aggregation, GitHub/Omni reporting | 1,676 |
| Benchmark runner, adapters, and subprocess management | 1,407 |
| GitHub workflows | 1,426 |
| Configuration, identities, hashes, and contracts | 881 |
| Tests | 2,479 |
| JSON/TOML/support files | 125 |
Tests are valuable, but they do not reduce the long-term ownership of roughly 7,500 lines of new production/configuration code. This is not just wiring up a benchmark gate; it creates another benchmark runner, result schema, compatibility system, baseline database, image lifecycle, reporting stack, and historical execution engine inside IsaacLab.
The benchmark runner is duplicated
The clearest simplification is tools/perf_smoke_test/perf_runtime.py. Its stated reason for bypassing the supported isaaclab benchmark runtime command is control over warm-up and output layout. Those capabilities already exist:
- isaaclab benchmark runtime supports random-action stepping, exact warm-up exclusion, measured-step count, backend/renderer presets, schema output, and provenance.
- BenchmarkRuntimeRequest and run_runtime_benchmark() expose the same workflow directly to Python and return the typed bundle plus output paths.
- RuntimeBundle already contains run configuration, versions, hardware, runtime metrics, and resources.
The gate should call that API or CLI rather than maintaining a fork of the runtime entrypoint. To preserve this PR's current contract, note that num_frames=300 with warmup_frames=100 currently produces 200 measured steps; the supported API would use num_steps=200 and warmup_steps=100. Alternatively, define num_steps as the measured window explicitly and remove this ambiguity.
The current pipeline then translates the supported schema through several additional representations:
RuntimeBundle → RuntimeSample → BenchResult → baseline record → OracleResult → Omni-GitHub row
Most of the identity and provenance carried through those types already exists in RuntimeBundle. A small consumer can parse the supported bundle directly. If some field is genuinely missing, it should normally be added to the shared benchmark schema rather than reconstructed in a gate-specific parallel contract.
Several large subsystems solve complexity introduced by this architecture
The workflow pulls a published image, overlays PR source on it, and builds a fallback image when pulling fails. That makes source/dependency skew possible, which then motivates image-era manifests, runtime-contract hashes, stale-image classification, fallback behavior, and historical image resolution.
The normal Docker + Tests workflow already builds and publishes a PR-SHA-specific image using the shared ecr-build-push-pull action. The performance job should consume that exact image, preferably from the same workflow or as a dependent workflow/job. Doing so should remove most or all of:
- image_era.py
- the moving-image manifest on perf-baselines
- the pull/build/retag implementation duplicated in this workflow
- the stale-image heuristic in aggregate.py
- a substantial part of runtime-contract matching
The same issue exists in reporting. This PR publishes per-task statuses, an aggregate status, a sticky comment, a job summary, normal artifacts, and a custom Omni-GitHub artifact. IsaacLab already has shared JUnit summary and Omni-GitHub upload actions. Prefer one native check result plus the existing result-ingestion path. If structured performance fields are missing from the shared uploader, extend that shared boundary once rather than adding another serializer and validation path here.
Advisory should be a repository setting, not a second status protocol
The current blocking=false mode forces aggregate.py to exit successfully for BLOCK and HARD_FAILURE, then attempts to communicate failure through a separate commit status. A failing but non-required GitHub check is already advisory. When the gate is ready to enforce, repository branch protection can make that check required without changing the result protocol.
This would eliminate the blocking-mode split, custom aggregate status, and the risk of a green Actions check hiding an unhealthy benchmark.
The latest run demonstrates this problem:
- Nine GPU jobs used about 145 combined L40S runner-minutes.
- Seven completed measurements produced WARN without a usable baseline comparison.
- One produced HARD_FAILURE during initialization.
- One timed out without producing an artifact.
- Aggregate + Verdict still passed.
- This is a fork PR, so the custom statuses and sticky comment were skipped; the meaningful result remained in the job summary/artifact.
That is a large operational cost without answering the core reviewer question: did this PR regress performance?
Baseline storage needs an explicit design decision
Before maintaining an append-only NDJSON database on an orphan Git branch, please establish why the existing internal performance storage/comparison tooling cannot be authoritative. The benchmark framework already emits OmniPerf and typed schema formats, and this PR also uploads to Omni-GitHub.
The perf-baselines implementation brings worktree management, transactional pushes, retries, ancestry filtering, stable sample IDs, branch matching, compatibility hashes, historical checkout execution, automatic reseeding, and verification. Some of these may be legitimate future requirements, but they should not all be prerequisites for an initial advisory smoke test.
The existing baseline branch is populated with records targeted at perf-smoke/develop-staging, while this PR targets develop, so the current live run cannot use them. Automatic historical reseeding should be deferred until the simple protected-branch record/read path is proven useful.
Questions that need explicit answers:
- What existing internal service is intended to store and compare IsaacLab benchmark results, and why can it not be called here?
- What exact metric is authoritative: runtime.total_fps.mean or runtime.environment_step_timing.environment_step_fps.mean?
- Why is an ancestry-filtered rolling MAD window required for the initial gate instead of a service-provided baseline or a simple calibrated threshold?
- Which identity fields materially affect comparability, and which are merely diagnostic?
- Why must all nine L40S jobs run on every PR, including changes unrelated to runtime performance?
- Is a sticky comment/custom status actually required when it cannot run for fork PRs?
Suggested replacement architecture
A much smaller first version would be:
- Reuse the existing change detector or an explicit label/manual/nightly trigger.
- Reuse the PR-SHA CI image from Docker + Tests.
- Keep a small declarative matrix containing task, presets, num_envs, measured steps, warm-up steps, and timeout.
- Invoke isaaclab benchmark runtime or run_runtime_benchmark() directly.
- Read the selected metric from RuntimeBundle.
- Query the existing internal baseline/comparison tooling.
- Emit one normal check/JUnit result and reuse the existing Omni-GitHub upload action.
- Record results only from trusted protected-branch runs.
- Let regressions fail a non-required check during the advisory period.
- Add rolling statistics, automated seeding, sticky comments, or blocking only after actual data shows they are needed.
If the internal baseline/comparison service covers storage and policy, this should plausibly be hundreds of lines rather than ten thousand. If a comparator is genuinely missing, it should still be possible to keep the change to a small, well-tested comparator and a thin workflow integration.
Requested direction
Please reconsider this as a replacement rather than trying to incrementally trim the current implementation. A good next revision would prove one or two buckets end-to-end using the existing benchmark API and normal CI image, then expand the matrix once the baseline lookup and reviewer-visible verdict are demonstrably working.
I inspected the full 41-file change, surrounding benchmark/CI APIs, PR discussion, current baseline branch contents, and the latest live workflow artifacts. I did not run the GPU benchmarks locally. The PR's remote pre-commit and pure-Python gate test checks passed.
| schema-v1 :class:`~isaaclab.benchmark.schema.RuntimeBundle`. It imports only | ||
| the stable building blocks (:mod:`~isaaclab.benchmark.stepping`, | ||
| :mod:`~isaaclab.benchmark.builders`, :mod:`~isaaclab.benchmark.capture`) rather | ||
| than driving ``isaaclab benchmark runtime``, so the gate controls its own |
There was a problem hiding this comment.
AI review — simplification: This custom driver duplicates the supported isaaclab benchmark runtime entrypoint and the typed BenchmarkRuntimeRequest / run_runtime_benchmark() API. The existing implementation already performs random-action stepping, exact warm-up exclusion, backend presets, schema output, and provenance capture. Please call it directly and remove this fork. To preserve the current 300-total/100-warm-up behavior, pass 200 measured steps plus 100 warm-up steps, or redefine the matrix field as measured steps explicitly.
| name: Performance Smoke Test | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
AI review — operational scope: There is no path/change filter here, so every PR into these branches launches nine L40S jobs. The latest run consumed about 145 combined GPU runner-minutes and still produced no usable regression comparison. Please reuse the repository's existing detect-changes action, or start with label/manual/nightly execution until the gate is calibrated and reliable.
| docker tag "${CI_IMAGE_REF}" "${{ env.CI_IMAGE_TAG }}" | ||
| echo "🟢 Tagged ${CI_IMAGE_REF} as ${{ env.CI_IMAGE_TAG }}" | ||
|
|
||
| - name: Build CI image (fallback) |
There was a problem hiding this comment.
AI review — simplification: Pulling a moving image, overlaying PR source, and conditionally building a fallback creates the source/dependency skew that later requires image eras, runtime-contract hashes, and stale-image classification. The normal Docker + Tests workflow already produces a PR-SHA-specific image through ecr-build-push-pull. Please integrate this job with that workflow and consume the exact PR image; most of this image lifecycle and its compensating machinery should then disappear.
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "_comment": "Perf-smoke gate configuration. 'blocking' is intentionally false: in advisory mode aggregate.py exits 0 for every verdict, including HARD_FAILURE, so the gate never fails a pull request on its own. The verdict is still reported -- a BLOCK or HARD FAILURE paints the 'perf-smoke-test' commit status red and is spelled out in the sticky PR comment and the job summary. Gate malfunctions (no bench artifacts at all, unreadable baseline branch, failed baseline push) exit nonzero in both modes, because those mean no trustworthy verdict was produced. Flipping 'blocking' to true is a deliberate, separate rollout: HARD_FAILURE then exits 2 and BLOCK exits 1, failing the aggregate job itself, and the 'perf-smoke-test' status becomes suitable to mark as a required check. Only keys the gate actually reads belong here; the baseline window sizes (MIN_BASELINE_SAMPLES, MAX_BASELINE_SAMPLES) are imported directly by the oracle and the baseline loader and cannot be overridden from this file. runtime_compatibility is intentionally omitted so it is sourced from gate_config.DEFAULT_RUNTIME_COMPATIBILITY (see gate_config.py); add a key only to override the code default.", | |||
| "blocking": false, | |||
There was a problem hiding this comment.
AI review — result semantics: A failing but non-required GitHub check is already advisory. Returning success for BLOCK/HARD_FAILURE and manufacturing a separate red commit status adds another protocol and allowed the latest Aggregate + Verdict job to stay green despite a hard failure and a missing bucket. Let the aggregate check reflect the real verdict; make it required later through branch protection when the rollout is ready.
| # Distinct from the per-task `perf-smoke (...)` statuses, which only report | ||
| # whether each benchmark ran. | ||
| # Skipped for fork pull requests; see the per-task status step for why. | ||
| - name: Report aggregate status |
There was a problem hiding this comment.
AI review — reporting: This custom status path is unavailable for fork PRs, including this PR; the live run produced no commit statuses or sticky verdict comment. Prefer the native Actions check conclusion plus job summary/JUnit/Omni ingestion, which work without write access to the PR. If a PR comment is essential, it should be produced by a separate trusted workflow_run consumer rather than PR-controlled code.
| # push emits no reseed_tasks. No loop risk -- seeding writes to perf-baselines, | ||
| # which triggers neither this gate nor the seed workflow. | ||
| # --------------------------------------------------------------------------- | ||
| reseed: |
There was a problem hiding this comment.
AI review — scope: Automatic historical reseeding is a separate product concern, not a prerequisite for the first advisory gate. The baseline branch is currently populated for perf-smoke/develop-staging and was unusable by this develop-targeted run. Please first prove the simple protected-branch write/read path (preferably through the existing internal performance store), then add seeding only if organic target-branch runs are demonstrably insufficient.
|
It might be good to have some way to enable this once a PR makes it to develop. If it is run on every PR it may take a long time. If we could tag a commit to get the perf numbers it would be very useful and only exercise CI runners on demand |
4 participants
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