Skip to content

Commit b9b2bab

Browse files
authored
autorevert: dispatch advisor on born-red test signals (#8107)
## Summary Closes a structural blind spot in the autorevert lambda: a test introduced by the same commit that breaks it (or by a recent commit that re-enabled / moved / unsharded it) has no green observations in the lookback window, so the existing green→red partition path returns `Ineligible(NO_SUCCESSES)` and never asks the advisor. This PR adds a "born-red" detection path that hands these cases to the AI advisor with framing tailored for the introduction question. ## Motivation [pytorch/pytorch#184104](pytorch/pytorch#184104) (2026-05-21, "[Inductor] Fix flip on 0-d tensors in prims.rev lowering") added two new tests to inductor codegen-dynamic-shapes and registered them as `expectedFailure` on cuda/xpu. The lowering fix actually worked on cuda → "Unexpected success" → FAILED. The lambda saw the failures on every trunk run from the PR landing onward but emitted `Ineligible(NO_SUCCESSES, "no successful commits present in window")` for ~9h until a human revert. Verified from `misc.autorevert_state.outcomes` JSON: ``` trunk:inductor/test_torchinductor_codegen_dynamic_shapes.py::test_flip_zero_dim_dynamic_shapes_cuda → Ineligible / reason="no_successes" / "no successful commits present in window" ``` The lambda was healthy on the same tick — it emitted an `AutorevertPattern` (advisor verdict=revert, confidence=0.97) for an unrelated `linux-jammy-cuda13.0-py3.10-gcc11` job-track signal. The blind spot is per-signal, not lambda-wide. ## Approach Three primitives, all reusing the existing data model: 1. **`Signal.partition_born_red()`** — returns a `PartitionedCommits` only when the signal shape (newest→oldest) is exactly `[FAIL...][EMPTY...]`. No interleaving, no pending-only commits in between. The trailing empty-events commits act as the implicit baseline ("commits where this test signal was not observed"). 2. **`Signal._handle_no_successes()`** — called from the existing early-exit point in `process_valid_autorevert_pattern`. For test-track signals where `partition_born_red()` succeeds and there are ≥2 distinct failing commits (counted by commits, not events — retries / multiple shards on a single commit are still one trunk observation), it either lifts an existing advisor verdict into an `AutorevertPattern` (revert/related) or dispatches a fresh advisor request alongside the `Ineligible(NO_SUCCESSES)` outcome. Job-track no-success signals stay on the original path (likely persistent infra or warm-up). 3. **`DispatchAdvisor.is_born_red` flag** — tells `_build_signal_pattern_json` to relabel the SUCCESSFUL partition. The new label asks the advisor to distinguish four causes from the suspect's diff: (a) ADDED the test, (b) ENABLED / un-skipped / removed a TD filter, (c) MOVED / RENAMED so the test identity is newly visible, (d) sharding / job-selection change. Recommend revert only when the suspect explicitly introduces or enables the failing test (cases a/b/c). Reused infrastructure: - `_check_advisor_verdict()` and `_build_autorevert_pattern()` from the existing partition path — the born-red partition slots in cleanly. - Advisor de-dup (`prior_advisor_exists`) and per-(workflow, commit) cap of 8 in `SignalActionsLogger` — no new infra. ## What changes for the advisor prompt When `is_born_red=True`, the SUCCESSFUL partition in `signal_pattern.json` is labeled: > **no_signal**: baseline commits where this test signal was NOT OBSERVED (no extracted events). The suspect commit is the first commit on which the test signal appears, and it fails. Possible causes — distinguish from the suspect commit's diff: (a) the suspect ADDED this test; (b) the suspect ENABLED/UN-SKIPPED an existing test or removed a TD/skipping filter that previously masked it; (c) the suspect MOVED or RENAMED the test so its identity is newly visible; (d) the suspect changed sharding / job selection so an existing already-failing test became visible. Recommend revert only if the suspect's diff explicitly introduces or enables the failing test (cases a/b/c). A top-level `is_born_red: true` flag is also surfaced in the JSON so the advisor workflow can branch on it cleanly. ## Test plan - [x] `pytorch_auto_revert/tests/test_signal.py::TestBornRedTestSignal` — 11 new tests covering: - canonical `[FAIL][EMPTY]` shape - rejection of shapes that aren't born-red (has successes, no empty tail, no failures, interleaved empty→fail, pending-only between, single commit) - end-to-end dispatch on the 2-failing-commits + 1-empty case - threshold counts distinct commits, not raw events (single commit with 3 retries doesn't dispatch) - job-track signals don't get the born-red path - chronic `[FAIL...]` without empty tail stays on the original `NO_SUCCESSES` path - existing advisor verdict (revert) lifts the born-red partition into `AutorevertPattern` with the right `suspected_commit` and `older_successful_commit` - `NOT_RELATED` verdict blocks (returns `ADVISOR_NOT_RELATED`) - [x] `pytorch_auto_revert/tests/test_signal_actions.py::test_born_red_relabels_baseline_partition` — verifies the JSON relabel and the `is_born_red` top-level flag, including all four ambiguity-cause cues. - [x] Full local suite: `./venv/bin/python -m unittest discover -s pytorch_auto_revert/tests -p "test_*.py"` — 175 tests, all pass. ## Notes - Cross-model adversarial review (`gpt-5.5` via Codex CLI) flagged three MEDIUM concerns; all three were addressed in this PR before submission: the introduction-vs-enable framing in the JSON label, gating on distinct failing commits (not raw events), and the verdict-key check (already keyed by `signal_key + suspect_commit`, no fix needed). - The advisor prompt template (`.github/workflows/claude-autorevert-advisor.yml`) is not touched here — the existing prompt already consumes the partition labels, so the relabel propagates through without a workflow change. If the upstream prompt benefits from explicitly branching on `is_born_red`, that can be a follow-up.
1 parent 92b7f03 commit b9b2bab

4 files changed

Lines changed: 540 additions & 23 deletions

File tree

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/signal.py

Lines changed: 116 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ class DispatchAdvisor:
8686
suspect_commit: str
8787
failed_commits: Tuple[str, ...] # SHAs in failed partition (newest first)
8888
successful_commits: Tuple[str, ...] # SHAs in successful partition (newest first)
89+
# When True, `successful_commits` are baseline commits where this signal
90+
# did NOT exist (test-track born-red case), not green observations.
91+
# The advisor JSON layer relabels them so the model is asked "did the
92+
# suspect commit introduce this failing test?".
93+
is_born_red: bool = False
8994

9095

9196
@dataclass
@@ -496,9 +501,61 @@ def partition_by_autorevert_pattern(self) -> Optional[PartitionedCommits]:
496501

497502
return PartitionedCommits(failed=failed, unknown=unknown, successful=successful)
498503

504+
def partition_born_red(self) -> Optional[PartitionedCommits]:
505+
"""Partition for the test-track "born-red" pattern: failing head + empty-events tail.
506+
507+
Returns a partition only when the signal shape is `[FAIL...][EMPTY...]`
508+
(newest → oldest) with no interleaving:
509+
- At least 2 commits in window
510+
- No commits with success events (otherwise the main partition path applies)
511+
- At least 1 commit with failure events at the head
512+
- At least 1 trailing commit with no events — these are commits where this
513+
test did not run or did not yet exist, used as the implicit baseline
514+
("commits without the signal")
515+
- No pending-only or other-shape commits in between
516+
517+
Trailing empty-events commits are placed in `successful` so the advisor
518+
check / pattern construction can reuse the standard primitives. The
519+
advisor JSON layer relabels them via `DispatchAdvisor.is_born_red` so
520+
the model is asked about test introduction, not green→red transition.
521+
`unknown` is always empty — there is no gap to bisect.
522+
"""
523+
if len(self.commits) < 2 or self.has_successes():
524+
return None
525+
526+
failed: List[SignalCommit] = []
527+
baseline: List[SignalCommit] = []
528+
for c in self.commits:
529+
if c.has_failure:
530+
if baseline:
531+
# Failure after an empty-events commit (newer→older order) —
532+
# out of shape.
533+
return None
534+
failed.append(c)
535+
elif not c.events:
536+
baseline.append(c)
537+
else:
538+
# Pending-only or other shape — bail; the advisor needs a clean
539+
# introduction-vs-baseline split.
540+
return None
541+
542+
if not failed or not baseline:
543+
return None
544+
545+
return PartitionedCommits(failed=failed, unknown=[], successful=baseline)
546+
499547
# Minimum confidence threshold for acting on advisor verdicts
500548
ADVISOR_CONFIDENCE_THRESHOLD = 0.89
501549

550+
# Minimum number of *distinct failing commits* required before dispatching
551+
# an advisor on a born-red test signal. Counting distinct commits (rather
552+
# than events) is deliberate: multiple FAILURE events on the same commit
553+
# (retries, multiple shards / variants of the same test, etc.) describe
554+
# one trunk observation, not two. Requiring two failing commits means the
555+
# failure has persisted across at least one trunk advance — strong enough
556+
# to justify the advisor cost without waiting indefinitely.
557+
REQUIRE_FAILED_COMMITS_BORN_RED = 2
558+
502559
def _build_autorevert_pattern(
503560
self,
504561
partition: "PartitionedCommits",
@@ -571,6 +628,64 @@ def _check_advisor_verdict(
571628
# "unsure" or expired garbage → continue normally
572629
return None
573630

631+
def _handle_no_successes(self) -> Union[AutorevertPattern, Ineligible]:
632+
"""Handle the `not has_successes()` branch.
633+
634+
Default behavior: emit `Ineligible(NO_SUCCESSES)` with no advisor — the
635+
standard green→red partition has no anchor to work from.
636+
637+
Test-track exception (born-red detection): when the signal shape matches
638+
`[FAIL...][EMPTY...]` (`partition_born_red` returns a partition), the
639+
suspect commit is likely the one that introduced the test. Defer to the
640+
AI advisor:
641+
- If a prior advisor verdict exists on the suspect, act on it
642+
(revert/related → `AutorevertPattern`; not_related/garbage → blocked).
643+
- Otherwise, dispatch a fresh advisor request alongside the `Ineligible`
644+
response. Next tick can act on the verdict.
645+
646+
Advisor dedup + the per-(workflow, commit) cap of 8 are handled by
647+
`SignalActionsLogger`; this method just emits the request.
648+
"""
649+
born_red = (
650+
self.partition_born_red() if self.source == SignalSource.TEST else None
651+
)
652+
if born_red is None:
653+
return Ineligible(
654+
IneligibleReason.NO_SUCCESSES,
655+
"no successful commits present in window",
656+
)
657+
658+
if len(born_red.failed) < self.REQUIRE_FAILED_COMMITS_BORN_RED:
659+
# Not enough distinct failing commits to justify advisor cost —
660+
# wait for the next trunk advance. Retries / multiple shards on a
661+
# single commit do not count as independent observations.
662+
return Ineligible(
663+
IneligibleReason.NO_SUCCESSES,
664+
f"born-red test signal: need ≥{self.REQUIRE_FAILED_COMMITS_BORN_RED} "
665+
f"failing commits before advisor dispatch (have "
666+
f"{len(born_red.failed)})",
667+
)
668+
669+
# If the advisor already weighed in on the suspect, use that verdict.
670+
advisor_decision = self._check_advisor_verdict(born_red)
671+
if advisor_decision is not None:
672+
return advisor_decision
673+
674+
# No verdict yet — request one. `successful_commits` carry the implicit
675+
# baseline (commits where the signal didn't exist); `is_born_red=True`
676+
# tells the advisor JSON builder to relabel them accordingly.
677+
advisor = DispatchAdvisor(
678+
suspect_commit=born_red.failed[-1].head_sha,
679+
failed_commits=tuple(c.head_sha for c in born_red.failed),
680+
successful_commits=tuple(c.head_sha for c in born_red.successful),
681+
is_born_red=True,
682+
)
683+
return Ineligible(
684+
IneligibleReason.NO_SUCCESSES,
685+
"born-red test signal: no successful commits in window — advisor dispatched",
686+
advisor=advisor,
687+
)
688+
574689
def process_valid_autorevert_pattern(
575690
self, *, bisection_limit: Optional[int] = None
576691
) -> Union[AutorevertPattern, RestartCommits, Ineligible]:
@@ -591,9 +706,7 @@ def process_valid_autorevert_pattern(
591706
IneligibleReason.FIXED, "signal appears recovered at head"
592707
)
593708
if not self.has_successes():
594-
return Ineligible(
595-
IneligibleReason.NO_SUCCESSES, "no successful commits present in window"
596-
)
709+
return self._handle_no_successes()
597710

598711
partition = self.partition_by_autorevert_pattern()
599712
if partition is None:

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/signal_actions.py

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,33 @@
3535
SignalProcOutcome = Union[AutorevertPattern, RestartCommits, Ineligible]
3636

3737

38+
# Top-level natural-language framing attached to the advisor's
39+
# `signal_pattern.json` payload when `DispatchAdvisor.is_born_red` is set.
40+
# Said once at the top level rather than duplicated into every baseline
41+
# commit's `partition` label.
42+
_BORN_RED_PATTERN_CONTEXT = (
43+
"born-red / newly-observed-red test signal: this test signal has NO "
44+
"observations on any commit older than the suspect in the lookback "
45+
"window. The suspect commit is the first commit on which the signal "
46+
"appears, and it fails. Older commits in the `no_signal` partition "
47+
"have empty events because the test was not extracted there — this "
48+
"is ambiguous and could mean any of:\n"
49+
" (a) the suspect ADDED this test;\n"
50+
" (b) the suspect ENABLED / un-skipped an existing test or removed a "
51+
"TD / skipping filter that previously masked it;\n"
52+
" (c) the suspect MOVED or RENAMED the test so its identity is newly "
53+
"visible;\n"
54+
" (d) the suspect changed sharding / job selection so an existing "
55+
"already-failing test became visible.\n"
56+
"Read the suspect commit's diff and decide which of (a)-(d) applies. "
57+
"Recommend `revert` only if the suspect's diff explicitly introduces "
58+
"or enables the failing test (cases a / b / c). For (d) — sharding "
59+
"or job-selection change exposing a pre-existing failure — recommend "
60+
"`not_related` because the suspect did not cause the test to fail, "
61+
"only made it observable."
62+
)
63+
64+
3865
class CommitPRSourceAction(Enum):
3966
MERGE = "merge"
4067
REVERT = "revert"
@@ -736,15 +763,28 @@ def _build_signal_pattern_json(
736763
"unknown: commits between failed and successful partitions "
737764
"with no resolved events (pending or missing data)"
738765
)
739-
LABEL_SUCCESSFUL = (
740-
"successful: baseline commits where this signal was GREEN "
741-
"before the suspect commit"
742-
)
743-
LABEL_PRIOR = (
744-
"prior: older commits before the successful baseline. "
745-
"Important: the signal may have been fixed and then failed again. "
746-
"Don't make assumptions just based on the presence of failures here."
747-
)
766+
# Born-red / newly-observed-red case: a test-track signal with no green
767+
# observations in the lookback window. The "successful" set actually
768+
# carries commits where this test signal was NOT OBSERVED (no extracted
769+
# events). The label stays short — the natural-language framing of what
770+
# the advisor needs to figure out lives in `pattern_context` at the
771+
# top level (so it's said once, not duplicated per commit).
772+
if dispatch_advisor.is_born_red:
773+
LABEL_SUCCESSFUL = (
774+
"no_signal: baseline commits where this test signal was not "
775+
"observed (no extracted events). See top-level pattern_context."
776+
)
777+
LABEL_PRIOR = "prior: even older commits with no signal observation"
778+
else:
779+
LABEL_SUCCESSFUL = (
780+
"successful: baseline commits where this signal was GREEN "
781+
"before the suspect commit"
782+
)
783+
LABEL_PRIOR = (
784+
"prior: older commits before the successful baseline. "
785+
"Important: the signal may have been fixed and then failed again. "
786+
"Don't make assumptions just based on the presence of failures here."
787+
)
748788

749789
def _fmt_ts(dt: Optional[datetime]) -> str:
750790
if dt is None:
@@ -823,17 +863,18 @@ def _partition_label(sha: str) -> str:
823863
}
824864
)
825865

826-
return json.dumps(
827-
{
828-
"signal_key": signal.key,
829-
"signal_source": signal.source.value if signal.source else "unknown",
830-
"workflow_name": signal.workflow_name,
831-
"job_base_name": signal.job_base_name,
832-
"commit_order": "newest_first",
833-
"suspect_commit": dispatch_advisor.suspect_commit,
834-
"commits": commits_json,
835-
}
836-
)
866+
payload = {
867+
"signal_key": signal.key,
868+
"signal_source": signal.source.value if signal.source else "unknown",
869+
"workflow_name": signal.workflow_name,
870+
"job_base_name": signal.job_base_name,
871+
"commit_order": "newest_first",
872+
"suspect_commit": dispatch_advisor.suspect_commit,
873+
"commits": commits_json,
874+
}
875+
if dispatch_advisor.is_born_red:
876+
payload["pattern_context"] = _BORN_RED_PATTERN_CONTEXT
877+
return json.dumps(payload)
837878

838879
def _commit_message_check_pr_is_revert(
839880
self, commit_message: str, ctx: RunContext

0 commit comments

Comments
 (0)