Add an AI_NOT_RELATED bucket for advisor-cleared failures - #8671
Add an AI_NOT_RELATED bucket for advisor-cleared failures#8671izaitsevfb wants to merge 7 commits into
Conversation
[ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
Bot
added
the
CLA Signed
…h readers Review of the AI_NOT_RELATED bucket raised three things about this file set. The saved query `advisor_verdicts_for_pr` ran twice per PR per Dr.CI pass: once to render the inline "AI verdict:" badge, once to decide which failures stop blocking. Two reads can straddle a verdict landing, and the two callers resolved the current verdict by different rules -- the badge took the first row per (sha, signal key), the gate sorted and refused ties -- so rows tied at the newest timestamp could show a green "not related" badge on a job that kept blocking. drci.ts now reads once and hands the rows to both, and `resolveVerdict` moves to advisorVerdictUtils so both resolve identically. It returns the representative ROW so the badge's summary comes from the same row whose confidence the gate judged, and among agreeing ties that is the least confident one. Tests covered the pure predicates but not the wiring. `fetchSuppressibleJobIds` loses its query and becomes the synchronous `suppressibleJobIds`, which makes the flag gate, the head-commit row filter and the `dr_ci_<name>` key mapping directly testable; the read decision becomes `shouldReadAdvisorVerdicts`, also tested. New advisorComment tests cover the badge side of the shared resolver. `MIN_SUPPRESSION_CONFIDENCE` restated `confidenceBucket`'s `high` threshold, so retuning the badge scale would have left the comment saying "probably not related" while the gate still suppressed. It now asks for the bucket. Because that ties a merge gate to a UI scale, a test pins the effective floor: moving it turns the test red rather than moving the gate quietly. Two things this fixes that the review did not raise. Only ask for verdicts when the PR actually has new or unclassified failures -- both consumers bail on an empty job list, so reading for the common no-failure PR bought a round trip nothing could use. And a confidence that is not a real number in [0, 1] now makes the key ambiguous: "least confident wins" is a comparison, NaN loses every comparison, so a NaN row beside a 0.95 row resolved to 0.95 and suppressed. The flag predicates move to advisorFlags.ts. advisorConfig.ts is imported by a React component and documents itself as free of server-only behavior, and these read process.env; keeping them out of the consumers also lets the read decision avoid importing the module that reaches the AWS SDK. Suppression stays behind DRCI_ADVISOR_SUPPRESSION_ENABLED and off everywhere. The display path and the gate still apply different freshness rules, which is unchanged here and documented in advisorSuppression.ts.
All three verdicts make the same claim about the PR under review -- this failure is not evidence against it -- so all three now leave the blocking set at high confidence. Blocking a PR on a broken runner was never the intent. `garbage` belongs with the cleared ones rather than with `unsure`, which keeps blocking. It is not "we cannot tell": the advisor reaches it by comparing the job against its own baselines and finding the signal flips red/green across unrelated commits. `unsure` is the real absence of a claim. The verdict set becomes a total map over AdvisorVerdictType rather than a list of the cleared ones, so adding a verdict fails to compile until someone decides which side it falls on -- verified by adding one and watching tsc name this file. The comment also states the limit the confidence and freshness gates do not cover: a verdict says a failure looks environmental, not that the PR is innocent of causing it, and a change to CI config, a Dockerfile or a submodule pin can produce a genuine infrastructure-shaped failure that is the PR's fault. `producedATestOutcome` narrows that -- an infra fault leaving the job `cancelled` never reaches here -- but it does not close it. The rendered section said "NOT RELATED TO THIS PR" and "judged them unrelated to this PR", which is false for a job cleared as an infra fault or an unusable signal. It now names all three reasons. Measured over 30 days on PR-side verdicts at high confidence, joined to each job's own conclusion: not_related 1179 on `failure`, infra_issue 976, garbage 1. So the suppressible population roughly doubles. The merge-side cap on how many gates one merge may skip is deliberately unchanged. Still behind DRCI_ADVISOR_SUPPRESSION_ENABLED, off everywhere.
…ue (#195502) ✴️ iz2: opened on behalf of @izaitsevfb ## The gap The AI CI Advisor prompt already knew that a flaky history must not override a causal chain to the suspect — the `garbage` bullet says exactly that. `infra_issue` carried no equivalent rule. That matters because **a commit can cause an infrastructure-shaped failure.** Moving a submodule pointer, editing a workflow or action YAML, changing a Dockerfile or image tag, bumping a dependency pin, touching build configuration, a runner label, or a setup script — any of these produces a log that reads like an ordinary infra fault: a checkout that fails, an image that will not pull, a dependency that will not resolve, a build that dies before any test runs. Nothing in the prompt told the model to read the suspect diff before dismissing such a failure. This is not hypothetical. Among live high-confidence PR-side verdicts there is an `infra_issue` reading: > The build failed because CMake couldn't checkout the `torch-xpu-ops` git submodule at commit `2c058add…`. This is a git/network infrastructure failure unrelated to the s… A bad submodule pin in the suspect produces precisely that. ## What changed Three edits, all inside the prompt: 1. **New Principle 4.** For precedence purposes `not_related`, `infra_issue` and `garbage` form one dismissal family: none of them may override an established causal chain to the suspect. They stay distinct verdicts with distinct positive-evidence requirements. Causality is settled first. 2. **`infra_issue` now requires positive evidence that the suspect did not cause the breakage** — not merely the absence of a found chain. 3. **The closing paragraph states the decision order explicitly** instead of only tie-breaking among the dismissals. ## What this deliberately does *not* do It is not a licence for a looser `related`, and the prompt says so in both directions: - Merely touching CI machinery is **not** a causal chain. File overlap or a bare plausible mechanism stays `unsure` (Principle 2). - Symmetrically, failing to find a chain does **not** establish that the suspect was independent — where both a suspect-caused and an external explanation remain plausible, that is `unsure` too, not a dismissal. The existing bias toward `unsure` over any dismissal, and the existing "a false `related` wrongly blames a clean commit" guard, are unchanged. ## Why now Dr.CI is gaining the ability to move high-confidence dismissals out of the merge-blocking set (pytorch/test-infra#8671, behind a flag and currently off). Once that is on, a false dismissal silently lets a broken PR merge, so the precedence rule needs to be explicit rather than implied for one verdict out of three. ## Test plan Prompt-only change; no code paths altered. - `.github/workflows/claude-autorevert-advisor.yml` parses as YAML, and the prompt string reached through the parsed document (not just the file text) contains each new clause — confirming the block scalar absorbed them correctly. - The `--json-schema` verdict enum is byte-identical: `["related","unsure","not_related","infra_issue","garbage"]`. No new verdict value, so no consumer changes. - Reviewed cross-model at high effort. That pass caught two real defects in my first draft, both fixed here: the original wording said "if the suspect touched the machinery that broke, that is `related`", which equated file overlap with causation and contradicted Principle 2; and the original decision order was binary (chain / no chain), which omitted "not established" and could have turned failure-to-prove-causation into proof of independence. Pull Request resolved: #195502 Approved by: https://github.com/pytorchgreenlight
Per Ivan: the heading and blurb were doing too much work. "NEW FAILURES SUPPRESSED BY AI" says what happened in the reader's vocabulary, and the description stops at the claim that matters. Dropping the tail also removes two defects. The blurb ended in a full stop and the renderer appends its own colon, so the summary read "...shown beside it.:". And that same sentence promised a per-job verdict that is absent whenever suppression is on and the inline-verdict flag is off -- two independent flags, so that combination is reachable. Moved directly after NEW FAILURES, since these are the jobs lifted out of it and belong beside it rather than at the end past CRCR.
… does not exist Four comment/copy corrections. No behavior change. The suppressed-jobs blurb said the advisor "judged the failures unrelated to the change". That is true only for `not_related`. `infra_issue` says the environment broke -- which this file's own comment notes a PR can itself cause via a Dockerfile, CI config or submodule pin -- and `garbage` judges the signal rather than the change. Shortening the section dropped the umbrella wording that covered all three; restore it as "not evidence against this change", which is still short, and record why so the next shortening keeps it. VERDICT_DISPOSITION claimed the residual exposure is "bounded by the merge-side cap on how many gates one merge may skip". That cap is in trymerge.py in pytorch/pytorch#195503, which has not landed, so the bound does not exist today. State the cap as proposed and the flag being off as what actually holds it. confidentEnoughToSuppress justified deriving the gate from confidenceBucket by saying the comment could otherwise read "probably not related" while the gate suppresses. verdictBadge returns for `garbage` and `infra_issue` before it consults the bucket, so those labels never hedge and the rationale describes `not_related` only. Note the case that falls out: a sub-threshold `infra_issue` renders an unhedged "infra issue" next to a job that still blocks, about 6% of `infra_issue` rows in a recent 30-day sample. That is a label fix in advisorBadge.ts, on the already-live comment path, so it stays a follow-up. suppressibleJobIds said the shared read means comment and gate "cannot disagree about a job". Sharing rows only stops them resolving different verdicts; the gate still adds confidence, freshness and conclusion tests the badge does not, so a confident badge beside a blocking job is expected, not a bug.
|
Are u planning to add |
|
Is there any trial run on a PyTorch PR that you could share? I want to see if this overlaps with Greenlight section, also recently added. |
| // VERCEL_ENV (unlike auto-dispatch, which fires real workflow_dispatches). | ||
| export function advisorCommentEnabled(owner: string, repo: string): boolean { | ||
| return ( | ||
| process.env.DRCI_ADVISOR_COMMENT_ENABLED === "true" && |
There was a problem hiding this comment.
When this DRCI_ADVISOR_COMMENT_ENABLED flag is off, let's make sure that trymerge can work like it's today.
| } | ||
| const newestTimestamp = rows | ||
| .map((r) => r.timestamp) | ||
| .reduce((a, b) => (a > b ? a : b)); |
There was a problem hiding this comment.
Highlight this snippet where we compare timestamp as string, is that safe? Why not also use dayjs here to be sure
There was a problem hiding this comment.
this should be fine
The string timestamp compare is safe — the column is DateTime64(3), server and column timezone both UTC, always 23 fixed-width chars, so lexicographic is chronological
| repo: string | ||
| ): boolean { | ||
| return ( | ||
| process.env.DRCI_ADVISOR_SUPPRESSION_ENABLED === "true" && |
There was a problem hiding this comment.
Nit: the agent recommends me against having 2 flags like this as it opens the door to mistake like setting DRCI_ADVISOR_SUPPRESSION_ENABLED=true while turning off DRCI_ADVISOR_COMMENT_ENABLED=false. Do we really need 2 flags here?
❌ 5 New Failures, 7 Unrelated FailuresAs of commit a945a2028e52611cd20e2988a4a0420c03ab6104 with merge base 40e238cee98065a2526d8420affade6be07912d7 ( NEW FAILURES - The following jobs have failed:
NEW FAILURES SUPPRESSED BY AI (non-blocking) - The following jobs failed, but the AI CI Advisor judged the failures unrelated to the change:
CRCR (non-blocking) - The following CRCR downstream CI jobs failed but are non-blocking:
|
Huy: "let's make sure that trymerge can work like it's today" when DRCI_ADVISOR_COMMENT_ENABLED is off, and separately, two independent flags "opens the door to mistake like setting DRCI_ADVISOR_SUPPRESSION_ENABLED=true while turning off DRCI_ADVISOR_COMMENT_ENABLED=false". Both point at one quadrant. Suppression keyed only to its own flag meant comment-off + suppression-on moved a job out of the blocking set while buildAdvisorVerdictLines returned nothing, so the comment named a section the AI had cleared without naming the verdict that cleared it -- and trymerge stopped blocking on a flag Huy's request never mentions. advisorSuppressionEnabled now requires advisorCommentEnabled, so that combination is unreachable rather than merely discouraged, and the comment flag being off restores today's trymerge behaviour exactly. Keeping two flags rather than collapsing to one: the split is the live production state -- the comment flag is on and suppression is off -- so a single flag would either enable suppression immediately or turn off a badge that is already shipping. The flag surface is now pinned by a truth table over both flags, plus the unset-is-off case, and shouldReadAdvisorVerdicts no longer claims suppression alone is a state that can ask for a read.
Huy, reviewing this alongside pytorch/test-infra#8671: "it's weird to not see AI_PENDING being handled somewhere, or do I miss it?" He is right, in the other direction. Nothing in test-infra emits an AI_PENDING key -- the producer was written as part of the original stack but never went into #8671, so Dr.CI's classification dict has never contained the category. is_ai_pending therefore reads an always-empty list, its branch in get_classifications can never fire, and AdvisorWaitWindow has nothing to wait for. Dead code guarding a case that cannot occur. Remove the matcher, the classification branch, the pending branch in categorize_checks, AdvisorWaitWindow and its budget constant, along with the tests that exercised them, and stop popping AI_PENDING from the stale check-run summary since it can never be in one. The surviving AI_NOT_RELATED tests move into the existing TestAdvisorNotRelated class rather than keeping a second class of their own. The problem the feature aimed at is real -- `@pytorchbot merge` adds ciflow/trunk, so trunk jobs start as the merge does and a fresh failure can kill the merge before the advisor has looked at it. Nothing here addresses that, and it should be taken up together with a producer rather than shipped as a consumer waiting on a key no one sends.
|
@huydhn , you review nits should be addressed, please take another look when you have a chance |
…ue (pytorch#195502) ✴️ iz2: opened on behalf of @izaitsevfb ## The gap The AI CI Advisor prompt already knew that a flaky history must not override a causal chain to the suspect — the `garbage` bullet says exactly that. `infra_issue` carried no equivalent rule. That matters because **a commit can cause an infrastructure-shaped failure.** Moving a submodule pointer, editing a workflow or action YAML, changing a Dockerfile or image tag, bumping a dependency pin, touching build configuration, a runner label, or a setup script — any of these produces a log that reads like an ordinary infra fault: a checkout that fails, an image that will not pull, a dependency that will not resolve, a build that dies before any test runs. Nothing in the prompt told the model to read the suspect diff before dismissing such a failure. This is not hypothetical. Among live high-confidence PR-side verdicts there is an `infra_issue` reading: > The build failed because CMake couldn't checkout the `torch-xpu-ops` git submodule at commit `2c058add…`. This is a git/network infrastructure failure unrelated to the s… A bad submodule pin in the suspect produces precisely that. ## What changed Three edits, all inside the prompt: 1. **New Principle 4.** For precedence purposes `not_related`, `infra_issue` and `garbage` form one dismissal family: none of them may override an established causal chain to the suspect. They stay distinct verdicts with distinct positive-evidence requirements. Causality is settled first. 2. **`infra_issue` now requires positive evidence that the suspect did not cause the breakage** — not merely the absence of a found chain. 3. **The closing paragraph states the decision order explicitly** instead of only tie-breaking among the dismissals. ## What this deliberately does *not* do It is not a licence for a looser `related`, and the prompt says so in both directions: - Merely touching CI machinery is **not** a causal chain. File overlap or a bare plausible mechanism stays `unsure` (Principle 2). - Symmetrically, failing to find a chain does **not** establish that the suspect was independent — where both a suspect-caused and an external explanation remain plausible, that is `unsure` too, not a dismissal. The existing bias toward `unsure` over any dismissal, and the existing "a false `related` wrongly blames a clean commit" guard, are unchanged. ## Why now Dr.CI is gaining the ability to move high-confidence dismissals out of the merge-blocking set (pytorch/test-infra#8671, behind a flag and currently off). Once that is on, a false dismissal silently lets a broken PR merge, so the precedence rule needs to be explicit rather than implied for one verdict out of three. ## Test plan Prompt-only change; no code paths altered. - `.github/workflows/claude-autorevert-advisor.yml` parses as YAML, and the prompt string reached through the parsed document (not just the file text) contains each new clause — confirming the block scalar absorbed them correctly. - The `--json-schema` verdict enum is byte-identical: `["related","unsure","not_related","infra_issue","garbage"]`. No new verdict value, so no consumer changes. - Reviewed cross-model at high effort. That pass caught two real defects in my first draft, both fixed here: the original wording said "if the suspect touched the machinery that broke, that is `related`", which equated file overlap with causation and contradicted Principle 2; and the original decision order was binary (chain / no chain), which omitted "not established" and could have turned failure-to-prove-causation into proof of independence. Pull Request resolved: pytorch#195502 Approved by: https://github.com/pytorchgreenlight
2 participants
✴️ iz2: opened on behalf of @izaitsevfb.
The AI CI Advisor already analyzes NEW and Unclassified failures and renders its verdict inline in the pytorchbot comment, but the verdict currently changes nothing: the job still sits in the NEW FAILURES section and still blocks the merge.
This moves the failures the advisor has cleared into their own non-blocking bucket,
AI_NOT_RELATED, in the same place and shape as the existingCRCR_L3block. Two things fall out of that for free:newFailedJobsand the "N New Failures" header both derive fromfailedJobs, so cleared jobs leave the NEW section automatically. They get their own collapsed section, still carrying theirAI verdict:line, and count toward "Unrelated".failures[pr]is both the/api/drci/drciresponse and theDr.CIcheck-run summary, sotrymerge.pycan read the new category with no extra request and no new auth surface. The merge-bot side is [trymerge] Stop blocking merges on failures the AI CI Advisor cleared pytorch#195503; nothing in this PR affects merges on its own.Which verdicts are cleared
not_related,infra_issueandgarbage, at high confidence. All three make the same claim about the PR under review — this failure is not evidence against it — and they are encoded as a total map overAdvisorVerdictType, so adding a verdict fails to compile until someone decides which side it falls on.garbagesits with the cleared ones rather than withunsurebecause it is an evidenced claim about the signal, not the absence of one: the advisor reaches it by comparing the job against its own baselines and finding the red flips across unrelated commits.unsureis the real "cannot tell", and it keeps blocking, as dorelatedandrevert.Measured over 30 days on PR-side verdicts at high confidence, joined to each job's own conclusion:
not_related1179 jobs,infra_issue976,garbage1.The two flags, and why suppression depends on the comment
Two deployment flags, not one:
DRCI_ADVISOR_COMMENT_ENABLED— render the inlineAI verdict:line. Display only.DRCI_ADVISOR_SUPPRESSION_ENABLED— move cleared failures out of the blocking set.Suppression requires the comment flag and is inert without it. Suppression with the comment dark would move a job into the non-blocking section while
buildAdvisorVerdictLinesreturns nothing, so the reader is told the AI cleared the job but never which verdict cleared it or why — and trymerge would stop blocking on a flag that says nothing about merges. That combination has no use, so it is unreachable by construction rather than left to deployment discipline.They stay two flags rather than one because the display half can be enabled on its own while the merge gate stays off, which is the intended rollout order. The whole flag surface is pinned by a truth table.
The rest of the predicate
Every branch fails toward blocking:
highbucket. Asked ofconfidenceBucketrather than restating its threshold, so retuning the badge scale cannot leave the comment saying "probably not related" while this still suppresses. A test pins the effective floor so that retune cannot be silent either.completed_at. Verdicts are keyed by(sha, job name)with no run identity, so a rerun at the same head would otherwise inherit the previous run's verdict.failure. A cancelled or timed-out job produced no outcome to clear. This gate is what keeps an infra fault that stopped the job before it concluded from being cleared.[0, 1]makes the whole key ambiguous.One read, one resolver
The saved query
advisor_verdicts_for_prused to run twice per PR per pass — once for the inline badge, once for the gate — and the two callers resolved the current verdict by different rules, so rows tied at the newest timestamp could render a green "not related" badge on a job the gate kept blocking.drci.tsnow reads once and hands the rows to both, through a sharedresolveVerdict. The read is also skipped entirely when the PR has no new or unclassified failures, which is most PRs.resolveVerdictpicks the newest row by comparing thetimestampstrings directly. That is safe rather than incidental:misc.autorevert_advisor_verdicts.timestampisDateTime64(3)on a UTC server, so every row in a response renders fixed-widthYYYY-MM-DD HH:MM:SS.mmmand lexicographic order is chronological order.dayjsis used in the one place the formats actually differ — comparing a ClickHouse timestamp against GitHub's ISO-8601completed_atin the freshness gate.Known limitations
run_idis the advisor's own dispatch run, not the job's.infra_issueverdict says a failure looks environmental, not that the PR did not cause it. A change to a submodule pin, workflow YAML, a Dockerfile or a dependency pin can produce a genuine infrastructure-shaped failure that is the PR's fault. [advisor] Makerelatedoutrank every dismissal, including infra_issue pytorch#195502 addresses this in the advisor prompt by makingrelatedoutrank every dismissal.All three are conditions on enabling, not on landing the code.
Safety
No-op unless both
DRCI_ADVISOR_SUPPRESSION_ENABLED=trueandDRCI_ADVISOR_COMMENT_ENABLED=true, and the repo is advisor-enabled. A ClickHouse error falls back to an empty set, so the worst case is today's behaviour. The lookup runs before the CRCR block so CRCR L4 jobs, which are pushed intofailedJobsthere and are never advisor-analyzed, cannot be eligible; extraction mutates the arrays in place because the same objects back both the failures dict and the comment renderer.Test plan
torchci/test/advisorSuppression.test.ts— every predicate branch, the verdict set, verdict ambiguity, non-finite and out-of-range confidence in both orderings, timestamp staleness, the head-commit row filter, thedr_ci_<name>key mapping, the repo gate, and a truth table over both flags including the unset-is-off case.torchci/test/advisorComment.test.ts— the badge side of the shared resolver, including that a conflicting tie renders no badge and that a tied pair renders the least-confident row whole.torchci/test/advisorVerdictSource.test.ts— the read guard, including that a PR with no eligible jobs costs no query and that the suppression flag alone reads nothing.tsc --noEmit, prettier andnext lintclean. Full jest run green: all 62 suites pass.NaNconfidence would have been discarded in favour of a usable one, a needless query on PRs with no failures, and the section wording, which said "NOT RELATED TO THIS PR" and would have been false for a job cleared as an infra fault.Not covered: the
drci.tsorchestration block itself has no automated test. Its constituent decisions are extracted and unit-tested instead; covering the orchestration itself would mean importingdrci.tsand its whole dependency graph. Worth deciding before the flag goes on.