-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix and rescope the L40S runner stability qualification #6863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| update_baselines_git, | ||
| ) | ||
| from contracts import BenchResult # noqa: E402 | ||
| from environment_skew import DependencySkew, detect_dependency_skew # noqa: E402 | ||
| from gate_config import BASELINE_PUSH_RETRIES, MIN_BASELINE_SAMPLES, load_gate_config # noqa: E402 | ||
| from gate_types import FpsMeanThreshold, OracleVerdict # noqa: E402 | ||
| from gpu_identity import canonical_gpu_model, gpu_model_config_keys # noqa: E402 | ||
|
|
@@ -160,6 +161,18 @@ def _runtime_context(bench_result: BenchResult) -> tuple[str, str]: | |
| return str(gpu_name or "N/A"), runtime or "N/A" | ||
|
|
||
|
|
||
| def _skewed_rows(rows: list[tuple]) -> list[tuple[str, str, DependencySkew]]: | ||
| """Return ``(task_id, backend, skew)`` for failures caused by a stale CI image.""" | ||
| skewed = [] | ||
| for result, bench_result in rows: | ||
| if result.verdict != OracleVerdict.HARD_FAILURE: | ||
| continue | ||
| skew = detect_dependency_skew(bench_result.stdout_tail) | ||
| if skew is not None: | ||
| skewed.append((result.task_id, result.backend, skew)) | ||
| return skewed | ||
|
|
||
|
|
||
| def _row_explanation(result) -> str: | ||
| """Explain one verdict in reviewer-facing language.""" | ||
| if result.verdict == OracleVerdict.HARD_FAILURE: | ||
|
|
@@ -227,16 +240,41 @@ def _build_technical_table(rows: list[tuple]) -> str: | |
| return "\n".join(lines) | ||
|
|
||
|
|
||
| def _build_stale_image_section(skewed: list[tuple[str, str, DependencySkew]]) -> str: | ||
| """Explain that a stale CI image, not the PR, caused these failures.""" | ||
| packages = sorted({skew.package for _, _, skew in skewed}) | ||
| affected = "\n".join(f"- `{task_id}` ({backend}): {skew.describe()}" for task_id, backend, skew in skewed) | ||
| return ( | ||
| "### Stale CI image\n\n" | ||
| f"The prebuilt CI image does not match this PR's pinned {' and '.join(packages)} version, so the " | ||
| "tasks below crashed before producing any FPS. This reflects the image, not the change under review, " | ||
| "so these results are advisory and do not fail the check.\n\n" | ||
| f"{affected}\n\n" | ||
| "This resolves itself once the CI image is rebuilt for the current dependency pins. Re-run the gate " | ||
| "after the next image publish to get real numbers for these tasks." | ||
| ) | ||
|
|
||
|
|
||
| def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str: | ||
| """Build reviewer-first Markdown for the sticky PR comment.""" | ||
| counts = {verdict: 0 for verdict in OracleVerdict} | ||
| for result, _ in rows: | ||
| counts[result.verdict] += 1 | ||
|
|
||
| skewed = _skewed_rows(rows) | ||
| skewed_keys = {(task_id, backend) for task_id, backend, _ in skewed} | ||
| unexplained_failures = sum( | ||
| 1 | ||
| for result, _ in rows | ||
| if result.verdict == OracleVerdict.HARD_FAILURE and (result.task_id, result.backend) not in skewed_keys | ||
| ) | ||
|
|
||
| if not rows: | ||
| overall = "❌ No benchmark results were produced" | ||
| elif counts[OracleVerdict.HARD_FAILURE]: | ||
| elif unexplained_failures: | ||
| overall = "❌ One or more benchmarks failed before producing usable performance data" | ||
| elif skewed: | ||
| overall = "⚠️ The CI image is stale for this PR, so some tasks could not be measured" | ||
| elif counts[OracleVerdict.BLOCK]: | ||
| overall = "🚫 One or more blocking-level performance regressions were detected" | ||
| elif counts[OracleVerdict.WARN]: | ||
|
|
@@ -269,10 +307,14 @@ def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str: | |
| else: | ||
| gpu_name, runtime = "N/A", "N/A" | ||
|
|
||
| return "\n\n".join( | ||
| sections = [ | ||
| f"### Overall result\n\n**{overall}**\n\n{count_summary}\n\n{mode}", | ||
| f"### Run context\n\n- **GPU:** {gpu_name}\n- **Runtime:** {runtime}", | ||
| ] | ||
| if skewed: | ||
| sections.append(_build_stale_image_section(skewed)) | ||
| sections.extend( | ||
| ( | ||
| f"### Overall result\n\n**{overall}**\n\n{count_summary}\n\n{mode}", | ||
| f"### Run context\n\n- **GPU:** {gpu_name}\n- **Runtime:** {runtime}", | ||
| "### How to read this\n\n" | ||
| "Start with **BLOCK** and **HARD FAILURE**, then review any **WARN** rows.\n\n" | ||
| "- **✅ PASS:** no meaningful slowdown was detected.\n" | ||
|
|
@@ -292,6 +334,7 @@ def _build_summary_markdown(rows: list[tuple], *, blocking: bool) -> str: | |
| f"{_build_technical_table(rows)}\n\n</details>", | ||
| ) | ||
| ) | ||
| return "\n\n".join(sections) | ||
|
|
||
|
|
||
| def _write_github_output(**values) -> None: | ||
|
|
@@ -407,7 +450,14 @@ def main() -> int: | |
| if oracle_result.verdict == OracleVerdict.BLOCK: | ||
| has_block = True | ||
| elif oracle_result.verdict == OracleVerdict.HARD_FAILURE: | ||
| has_hard_failure = True | ||
| # 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) | ||
| if skew is None: | ||
| has_hard_failure = True | ||
| else: | ||
| print(f"[aggregate] {task_id}/{backend}: stale CI image; {skew.describe()}") | ||
|
Comment on lines
+455
to
+460
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When PR source imports or accesses a nonexistent symbol from an allowlisted package such as |
||
|
|
||
| if ( | ||
| allow_update | ||
|
|
@@ -526,11 +576,13 @@ def main() -> int: | |
| if baseline_update_failed: | ||
| return 1 | ||
|
|
||
| if blocking: | ||
| if has_block: | ||
| return 1 | ||
| if has_hard_failure: | ||
| return 2 | ||
| # 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Warning · Implementation — Advisory wording contradicts new failing exit code Hard failures now return 2 regardless of |
||
| return 2 | ||
| if blocking and has_block: | ||
| return 1 | ||
| return 0 | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | ||
| # All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| """Detection of source-versus-image dependency skew in a failed benchmark. | ||
|
|
||
| The gate bind-mounts Isaac Lab source over a prebuilt CI image, but the image | ||
| supplies the installed third-party packages (Newton, Warp, Isaac Sim). Between a | ||
| dependency-pin change landing on ``develop`` and the next image publish, a PR's | ||
| source can reference a symbol the installed package does not have yet, which | ||
| crashes every affected task before any FPS is measured. | ||
|
|
||
| That crash says nothing about the PR's performance, so it must not read as a | ||
| performance failure. This module recognizes the crash signature so the gate can | ||
| report a stale image and stay advisory for the affected tasks instead. | ||
|
|
||
| Only packages the *image* installs are eligible. A missing symbol in Isaac Lab's | ||
| own source is a genuine defect in the change under test and still fails. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import re | ||
| from dataclasses import dataclass | ||
|
|
||
| # Packages installed into the CI image rather than bind-mounted from the PR. | ||
| IMAGE_PROVIDED_PACKAGES: frozenset[str] = frozenset( | ||
| { | ||
| "carb", | ||
| "isaacsim", | ||
| "mujoco", | ||
| "mujoco_warp", | ||
| "newton", | ||
| "omni", | ||
| "pxr", | ||
| "warp", | ||
| } | ||
| ) | ||
|
|
||
| # Python spells "this name is not in the installed package" three ways. | ||
| _MISSING_NAME_PATTERNS: tuple[re.Pattern[str], ...] = ( | ||
| re.compile(r"ImportError: cannot import name ['\"](?P<symbol>\w+)['\"] from ['\"](?P<module>[\w.]+)['\"]"), | ||
| re.compile(r"ModuleNotFoundError: No module named ['\"](?P<module>[\w.]+)['\"]"), | ||
| re.compile(r"AttributeError: module ['\"](?P<module>[\w.]+)['\"] has no attribute ['\"](?P<symbol>\w+)['\"]"), | ||
| ) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class DependencySkew: | ||
| """One detected mismatch between the PR's source and the image's packages.""" | ||
|
|
||
| package: str | ||
| module: str | ||
| symbol: str | None | ||
|
|
||
| def describe(self) -> str: | ||
| """Return a one-line reviewer-facing description of the mismatch.""" | ||
| if self.symbol: | ||
| return f"`{self.module}` in the CI image has no `{self.symbol}`" | ||
| return f"`{self.module}` is not installed in the CI image" | ||
|
|
||
|
|
||
| def detect_dependency_skew(log_text: str | None) -> DependencySkew | None: | ||
| """Return the dependency skew a benchmark log indicates, if any. | ||
|
|
||
| Args: | ||
| log_text: Captured benchmark output, typically ``BenchResult.stdout_tail``. | ||
|
|
||
| Returns: | ||
| The detected mismatch, or ``None`` when the log shows no missing symbol | ||
| from an image-provided package. | ||
| """ | ||
| if not log_text: | ||
| return None | ||
| for pattern in _MISSING_NAME_PATTERNS: | ||
| match = pattern.search(log_text) | ||
| if match is None: | ||
| continue | ||
| module = match.group("module") | ||
| package = module.split(".", 1)[0] | ||
| if package not in IMAGE_PROVIDED_PACKAGES: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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 |
||
| continue | ||
| return DependencySkew(package=package, module=module, symbol=match.groupdict().get("symbol")) | ||
| return None | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Suggestion · Implementation — Stale-image headline outranks blocking regression
The new⚠️ The CI image is stale…" as the overall result while
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.