Policy: red is the PR author's move, green is the maintainers'; QoR and pr-head are reported

Aperta
#4,778 0 commenti 0 reazioni 2 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

@vvbandeira ci sta già lavorando.

Dal 1/11/2026.

Valutazione

Questa issue non è ancora stata valutata.

Descrizione

The ask

Out of 20 red PRs of mine on 2026-11-04, across OpenROAD and ORFS, 2 were mine to fix.

The red/green check should say whose move it is. Red means the author has something to fix. Green means the maintainers decide: review, judge the QoR table, merge or hold. Anything that isn't the author's to fix (a CI outage, a broken master, a flaky test, a QoR difference) should leave the check green. The maintainers see it in the report and act on it there.

To put this into effect, cut two wires:

  • A QoR difference no longer marks a design's stage as failed. It stays in the inline-check PR comment, where maintainers read it today. The comparison rows that are correctness checks with a limit of zero keep failing the stage: detailedroute__route__drc_errors, detailedplace__design__violations and constraints__clocks__count.
  • pr-head keeps running and stays on the PR as information. Only pr-merge sets red/green. This supersedes #4726.

The engineering is done. The comparison runs, the baseline is maintained, and the PR comment with a table per design is posted on every PR. Nothing that runs today stops running, nobody's role changes, and the decision can be reverted the same way.

flowchart LR
  T["Build, lint, unit tests"] --> G(["Green: ready for the<br/>maintainers' judgement"])
  F["Flows complete,<br/>no DRC errors"] --> G
  Q["QoR comparison"] --> C["Inline-check comment<br/>on the PR"]
  H["pr-head"] --> C
  G --> J(["Maintainers' judgement"])
  C --> J
  Q -.-|"✕ cut: no longer sets"| S["Red/green status"]
  H -.-|"✕ cut: no longer sets"| S
Whose move is it
Problem Seen on 2026-11-04 Author's move Maintainers' move
The PR breaks the build, a test or a flow (crash, DRC error, missing file) 2 Fix the PR —
The PR doesn't merge cleanly 0 Rebase —
A maintainer asked for changes (a held QoR regression among them) — Make the changes —
The PR depends on an unmerged upstream PR and isn't a draft 0 Mark it a draft —
QoR difference caused by the PR 1 — Judge it: merge and track it in an issue, or ask for changes
QoR difference already on master 12 designs — Track it on master
Master is broken, and the PR inherits it 12 — Say "needs fix on master"; pr-merge re-runs on its own once master is fixed
CI outage: download mirror, GitHub API, registry, lost machine, timeout 3 — Say "CI outage"; pr-merge retries on its own
Flaky test unrelated to the change 1 — Fix or quarantine the test
The PR depends on an unmerged upstream PR 1 — Review the dependency

Of the 20 reds, 2 were the author's move and 18 were the maintainers'.

The fix for master can come from anyone, a contributor included. The maintainers' move is to say what the PR is waiting on, "needs fix on master". That leaves evidence the PR was reviewed, for their own benefit and the author's.

pr-head is information, not a vote

pr-head builds the PR on the master it branched from. pr-merge builds it on the master it will land on, which is the only thing that gets merged. A failure the PR causes shows in pr-merge, so a vote from pr-head adds nothing about the PR. All it adds is a second chance to turn red on everything in the table that isn't the author's move. If each check passes with chance p, independently of the PR, the PR is red with chance 1 − p² instead of 1 − p:

Pass rate of each check Red with pr-head voting Red, pr-merge only
80 % 36 % 20 %
79 %: OpenROAD master, 31 of the last 39 builds 37 % 21 %
40 %: ORFS master, 16 of the last 40 builds 84 % 60 %

pr-head stays on the PR, where the author can use it to tell "my change" from "master moved".

When master is red

ORFS master has been red since 2026-10-23: builds 1714 to 1736, 23 in a row, the last green one 1713. The failure has changed along the way. First it was a crash in CTS on asap7/swerv_wrapper (RSZ-1008) and asap7/aes-block, now QoR failures on 12 designs. No open or pinned issue says so. A user taking an ORFS snapshot sees nothing on the repository saying that master doesn't build cleanly, and hasn't for 12 days.

While master is red, every PR inherits the red, so the check can no longer tell a PR that is ready from one that isn't. Review slows down: 21 PRs merged in the 12 red days, against 30 in the 12 days before. That includes the PRs that would fix master, and the fix waits on "someone else". Whoever writes the fix, nobody has said it is theirs to find.

Proposed:

  • A red master gets an issue the day it goes red, pinned. It names the failing designs and the first bad build, and it is closed when master is green. Users see it on the repository, and a PR can link to it in place of a "needs fix on master" comment that is repeated on every PR.
  • A PR that fixes master is reviewed first. The fix can come from anyone. Reviewing it is the maintainers' move.
  • With QoR reported rather than voting, a red master means a real breakage, a crash or a DRC error, not a moved baseline, so a red master is rarer and worth the pinned issue every time.
Why

The baseline now keeps itself up to date. Merge a PR, and the next comparison runs against a newer master build. The routine of editing rules-base.json to move thresholds is over:

Merged in 2026 PRs touched rules-base.json
May to August 158 57
September 73 16
October 89 1

Thanks to everyone who built this. It's a real quality-of-life improvement, and this decision builds on it.

This is already how we work. Maintainers read the QoR table, weigh it against what the change is for, and decide. In September and October, 31 of 162 merged PRs, about one in five, went in with a red pr-merge check after a maintainer judged the change ready. Most of those logs have expired, so the reds can't all be attributed to QoR. Either way, a red check already doesn't decide merges: the maintainers' judgement does. This decision makes the check say the same thing.

The QoR job is big and thorough, so it is exposed. It builds dozens of designs through the whole flow, for hours, across many machines. That thoroughness is what makes it valuable, and it also makes it the job most likely to fail through no fault of its own or of the PR: a lost machine, a download that fails, a timeout. A PR is just as ready for judgement after a QoR difference or an outage it didn't cause, and maintainers already treat both that way.

The comment is already where the detail is. The inline-check comment lists each design's regressions with the numbers, the baseline and the limit, which a red status can't show. Keeping QoR there keeps it in front of the people who decide.

QoR needs judgement. What a QoR change means depends on what the change is for: a deliberate trade, a side effect, or a refactor meant to change nothing. The reviewers know that intent. The numbers often need reading in context too (#4583): WNS is compared as a percentage of a small number, and in 13 of 51 designs WNS is set by an input/output budget in the .sdc, not by the design's logic.

The maintainers judge what happens to a QoR regression:

flowchart LR
  R["QoR regression<br/>in a PR"] --> M{"Maintainers<br/>judge"}
  M -->|"cause elsewhere, intended trade,<br/>or contributor can't act"| I["Merge, and track<br/>it in an issue"]
  M -->|"affects a lot of stakeholders,<br/>or contributor can fix it"| H["Ask for changes"]
  • Merge, and track it in an issue: when the cause is in OpenROAD or another design, the trade is intended, or the contributor can't act on it. An issue can be assigned to whoever can fix it, and it saves the contributor a PR that waits and needs rebasing.
  • Ask for changes: when the regression affects a lot of stakeholders, or the contributor can fix it in the PR. The PR is then the author's move.

Master keeps working at every commit. Users take snapshots of ORFS and rely on that, and the red/green check keeps guarding it. Open QoR issues on master show them the trade-offs when they decide whether a new OpenROAD and ORFS combination is worth taking for their own designs.

What it gives us
  • Authors: red is a to-do list of things they can fix. Green means "my change works", QoR is a table to discuss, and first-time contributors see one clear status.
  • Maintainers: green is the review queue. A CI outage, a broken master or a flaky test is theirs to name, and it shows in the report without turning every open PR red. Merging a judged PR needs no explanation for its status.
  • A more resilient CI: a QoR difference, from this PR or from anywhere else, shows in the comment and leaves the check green. A crash, a DRC error, or a failed build or test stays red, as today.
  • Red points at the problem: whoever opens the CI log of a red PR is looking for one real failure the PR caused. Nobody has to dig through a long log to find out that a red check was a QoR difference or an outage.
  • Users: a red master is a pinned issue they can see.
Related
  • #4726: pr-head should not vote (superseded by this issue)
  • #4583: compare by the reg2reg minimum clock period
  • #4604: compare against the recorded commit's baseline
  • The-OpenROAD-Project/OpenROAD#11476: the same policy for pr-head in OpenROAD
Method

gh pr list --state merged for 2026-05-01 to 2026-11-01: the final CI-Public/pr-merge status and the changed files of each PR, counted by month. The 13 of 51 designs are from the sweep in #4583.

Red PRs on 2026-11-04: every open OpenROAD and ORFS PR by one author, classified from the Jenkins console logs. Master pass rates are from the last 40 master builds of each job. The red streak is ORFS master builds 1713 to 1736. Merges are gh pr list --state merged for the 12 days before and after 2026-10-23 15:17 UTC.

Lingua principale
Verilog
Stelle
782
Fork
552
Merge medio
3g 4h
PR unite (30g)
61

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Nessun modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di The-OpenROAD-Project/OpenROAD-flow-scripts

Tutte le issue di The-OpenROAD-Project/OpenROAD-flow-scripts

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.