Fix and rescope the L40S runner stability qualification - #6863
Conversation
The 2026-07-28 qualification run collected only 57 of 135 samples and produced no verdict. Two independent faults caused the shortfall, and both are fixed here. Most Newton tasks crashed on `from newton.solvers import SolverNotifyFlags`. Staging source was 145 commits behind develop while the CI image was built from recent develop, where that symbol had been replaced by ModelFlags. Merging develop into staging removes the mismatch. One allocation died partway through because five parallel allocations shared a single Warp JIT cache directory and raced on creating it. Each allocation now derives its own cache root from the run id, attempt, and run label. Separately, a crashed benchmark could exit zero while FPS regressions were advisory, so execution health is now always enforced. To keep that from failing PRs for reasons outside their control, a crash whose log shows the image missing a symbol the source pins is reported as a stale CI image and stays advisory. Only packages the image installs qualify; a missing Isaac Lab symbol is still a real defect. The qualification workflow no longer promotes samples to the rolling baseline or chains a second gate run. It measures the pool and reports a verdict.
The 2026-07-28 run collected clean evidence for the four PhysX-only buckets and none for the five that load Newton, which all crashed on `from newton.solvers import SolverNotifyFlags`. Re-running the full matrix would spend half the pool time re-measuring buckets that already worked. Restrict both the sampling and the qualification to the Newton-touching backends, cutting the run from 135 to 75 samples. The seeder already accepted a backend allowlist; runner_stability now takes the same list so the expected scope matches what was collected, instead of reporting the PhysX buckets as missing evidence and failing closed. An unknown backend key is rejected rather than silently dropped, since a typo would otherwise narrow the scope and yield a weaker verdict that still reads as qualified.
Greptile SummaryThe PR rescopes L40S stability qualification to Newton-backed buckets, isolates per-allocation caches, and makes ordinary benchmark crashes fail even in advisory mode. It also introduces stale-image diagnosis, but that diagnosis currently suppresses genuine source-caused third-party API failures.
Confidence Score: 4/5The stale-image exception must be narrowed before merging because a genuine PR-caused third-party API crash can currently leave the performance gate successful. The new classifier infers image skew solely from a missing-name string and package allowlist, then suppresses the hard-failure exit without proving the source pins a compatible API or that the image caused the crash. Files Needing Attention: tools/perf_smoke_test/aggregate.py, tools/perf_smoke_test/environment_skew.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Staging workflow change] --> B[Wait for initial gate and quiet L40 pool]
B --> C[Five runner allocations]
C --> D[Seed Newton backend samples]
D --> E[Per-allocation JIT and Kit caches]
D --> F[Download sample records]
F --> G[Qualify expected Newton buckets]
G --> H[Report stability verdict]
Reviews (1): Last reviewed commit: "Scope stability qualification to the New..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # reported loudly but never fails the PR. | ||
| skew = detect_dependency_skew(bench_result.stdout_tail) | ||
| if skew is None: | ||
| has_hard_failure = True | ||
| else: | ||
| print(f"[aggregate] {task_id}/{backend}: stale CI image; {skew.describe()}") |
There was a problem hiding this comment.
When PR source imports or accesses a nonexistent symbol from an allowlisted package such as newton or warp, this branch classifies the resulting hard failure as a stale image using only the log message and package prefix. It then leaves has_hard_failure unset, causing the aggregate command to return success even though the change under test crashed the benchmark.
![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 qualification rescoping and cache-isolation changes are coherent, but the new stale-image exemption is overly broad and can allow genuine PR defects to pass. Aggregate reporting also contradicts the new hard-failure exit behavior and can hide a blocking regression behind a stale-image headline.
- Design and architecture: Separating dependency-skew detection into a reusable module is reasonable, and the backend allowlist fails closed on unknown keys. However, skew classification relies only on the missing symbol’s top-level package being image-provided; it does not verify that the source-pinned dependency actually contains the symbol. Consequently, PR-authored typos or unsupported API usage in Newton, Warp, or Isaac Sim can be misclassified as infrastructure skew and exempted from failure.
- API: The new
--backendsoption remains backward compatible by defaulting to all configured buckets. The material behavior change is that genuine hard failures now return exit code 2 even in non-blocking mode, but the generated advisory-mode text still says results do not fail the PR. The reporting contract must accurately distinguish advisory performance verdicts from always-enforced execution health. - Implementation: The aggregate verdict path has two reporting defects: non-blocking summaries incorrectly claim that no result can fail the check, and stale-image failures take headline precedence over BLOCK results even when a blocking regression determines the exit status. Tighten skew verification and order or phrase the summary so its overall result matches the effective gate outcome.
Significant concerns. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
| # Benchmark execution health is never advisory. A crash, missing result, or | ||
| # invalid benchmark must fail even while FPS regressions are being rolled out | ||
| # in advisory mode. | ||
| if has_hard_failure: |
There was a problem hiding this comment.
🟡 Warning · Implementation — Advisory wording contradicts new failing exit code
Hard failures now return 2 regardless of blocking, but _build_summary_markdown still prints "Advisory: results are reported for review but do not fail the PR" when the gate is non-blocking. Reviewers of an advisory run see a comment claiming the check cannot fail while the job exits 2. Update the advisory mode string to state that benchmark execution failures always fail the check.
| continue | ||
| module = match.group("module") | ||
| package = module.split(".", 1)[0] | ||
| if package not in IMAGE_PROVIDED_PACKAGES: |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Skew guard also excuses PR-side API misuse
Classification depends only on the top-level package name of the missing symbol, with no check that the source-pinned version actually provides it. A PR that mistypes or misuses a Newton/Warp/Isaac Sim symbol produces the same crash signature and is silently excused, so aggregate.main() exits 0 on a real defect. Narrow this by matching only verified skew signatures, or by comparing the installed package version against the pin before exempting.
| elif counts[OracleVerdict.HARD_FAILURE]: | ||
| elif unexplained_failures: | ||
| overall = "❌ One or more benchmarks failed before producing usable performance data" | ||
| elif skewed: |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Stale-image headline outranks blocking regression
The new elif skewed: branch is evaluated before the BLOCK branch. A run with one excused stale-image crash plus one blocking regression reports "main() still returns 1 in blocking mode. Evaluate BLOCK first and keep the stale-image section as an additional note.
|
Superseded by #6864, which lands the gate directly on The fixes here are all carried over. Targeting |
1 participant
Summary
The previous qualification run collected 57 of 135 samples and never reached a verdict. This fixes the causes and narrows the re-run to the buckets that actually failed.
Why it fell short
Two unrelated faults, both confirmed from the run logs:
from newton.solvers import SolverNotifyFlags. The staging source was far behinddevelopwhile the image's Newton was newer, and that symbol had already been replaced byModelFlags. This accounts for 75 of the 78 missing samples.PermissionErroron/tmp/jit-cache/warp/.... All five parallel allocations shared a single Warp JIT cache directory and raced on creating it. This accounts for the remaining 3.What changed
newton,warp,isaacsim, ...) qualify; a missing Isaac Lab symbol is still a real defect and still fails.Depends on a staging catch-up
This PR deliberately contains no merge from
develop, so the diff stays reviewable. It does not on its own fix cause 1: at the current staging tip,newton_manager.pystill importsSolverNotifyFlags, so the Newton buckets would crash again and collect nothing.Staging must be caught up to
developbefore this qualification is worth running. On currentdevelop,newton_manager.pyusesModelFlagsand imports onlySolverBase, SolverKamino, which matches what the image provides. With this PR's guard in place, the crash would at least be reported as a stale image and stay advisory rather than reading as a performance failure.Scope note
The verdict will cover the five Newton buckets. The four PhysX buckets are not re-qualified at this commit, since their July 28 samples carry the old staging SHA and cannot be mixed with new ones. Runner-to-runner spread is largely a hardware property and the Newton buckets sample the same five runners, so this still answers the pool-stability question.
The gate remains in advisory mode.
Test plan
developfirst.Performance Smoke - L40S Runner Stability, which waits for this commit'sPerformance Smoke Testto pass and for the L40 pool to go quiet.PermissionErrorin the JIT cache.INCONCLUSIVEfrom missing evidence.pre-commitis clean (verified locally).