From e767be45c8a52abae057450917e550235e320331 Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Fri, 2 Oct 2026 08:36:36 -0300 Subject: [PATCH 1/8] Check QoR as a checkout of the recorded commit The dashboard resolves inline checks in two ways. A request that names a pipeline (JobName or PipelineId) is a build of that pipeline and follows its CI branch rules. Any other request is a checkout: the dashboard searches its default baseline pipelines and takes a master build that has the design, at the commit's merge base or else the latest one before the commit. So the script no longer sends IsLocal or a default job name. Without --job-name it sends just the commit metadata.json records (--commit / DASHBOARD_COMMIT overrides it, and must be a full 40-character SHA: the dashboard matches it exactly and falls back to the latest master build on a miss). With --job-name it sends that name and the branch, from --branch / DASHBOARD_BRANCH or git, as CI does. A --base-commit pin no longer needs a job name. A run whose metadata.json records no commit is compared against the latest master build, and the report now says so. --api-url / DASHBOARD_API_URL picks the deployment and takes precedence over --beta. The report names the pipeline that provided the baseline. Also drop non-finite metric values before sending: json.dumps renders them as bare Infinity/NaN, and the dashboard rejects the whole request. Signed-off-by: Miguel Dalberto Pedro --- docs/contrib/Metrics.md | 24 ++++- flow/util/checkQorMetrics.py | 170 ++++++++++++++++++++++++++++++----- flow/util/utils.mk | 12 +-- 3 files changed, 176 insertions(+), 30 deletions(-) diff --git a/docs/contrib/Metrics.md b/docs/contrib/Metrics.md index 5bef0658d8c..bd045aa56d4 100644 --- a/docs/contrib/Metrics.md +++ b/docs/contrib/Metrics.md @@ -47,12 +47,28 @@ check did not run. Read the `[WARN]` lines to see which. Pass `--strict` to `checkQorMetrics.py` to get distinct exit codes instead (2 inconclusive, 3 unreachable). -Two environment variables shape the comparison: +The check runs as the commit `metadata.json` records. The dashboard searches +its default baseline pipelines in order and takes a `master` build that has the +design: the one at the commit's merge base with `master`, otherwise the latest +one before the commit. A commit on `master` is its own merge base, so a run of +a `master` commit, such as a downloaded CI artifact, is compared against that +commit's own build. A commit not yet pushed has no merge base on GitHub, so it +is compared against the latest `master` build before it. Without a commit the +run is compared against the latest `master` build. + +These environment variables shape the comparison: - `DASHBOARD_API_KEY` grants access to private platforms. Without it, a - private design reports as inconclusive, not as an authentication error. -- `DASHBOARD_JOB_NAME` names the Jenkins pipeline whose `master` builds are the - baseline. The default is `OpenROAD-flow-scripts-Public`. + private design has no baseline and reports as inconclusive, not as an + authentication error. +- `DASHBOARD_JOB_NAME` checks the run as a build of that CI pipeline instead, + with the pipeline's rules for the branch checked out in this repository: the + previous build on a trunk branch such as `master`, the merge base on any + other branch. A CI job, whose checkout has no branch, also sets + `DASHBOARD_BRANCH` and `DASHBOARD_COMMIT` (or passes `--branch` and + `--commit`). +- `DASHBOARD_API_URL` (`--api-url`) points the check at another deployment and + takes precedence over `--beta`. If you only changed synthesis, you can gate a synthesis-only run, without running the rest of the flow. Only the `synth__` and `constraints__` metrics diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 7146c53e507..f7e6c4d434f 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -4,8 +4,8 @@ (https://dashboard.precisioninno.com). The check POSTs a run's numeric metrics from metadata.json to the dashboard, -which compares them against a real baseline build -- the latest master build -by default -- using the thresholds it stores in rule_configs. This is the QoR +which compares them against a real baseline build -- the master build at the +run's merge base, by default -- using the thresholds it stores in rule_configs. This is the QoR gate behind `make metadata`. Rule tolerances live in the dashboard, not in this repository, and the baseline moves with master, so a merged change becomes the new baseline without a file update. @@ -21,6 +21,19 @@ export DASHBOARD_API_KEY=plt_... # optional; needed for private platforms ./util/checkQorMetrics.py +Each run is checked as the commit metadata.json records (--commit overrides +it). The dashboard searches its default baseline pipelines in order and takes +a master build that has the design: the one at the commit's merge base with +master, else the latest one before the commit. A commit on master is its own +merge base, so a master run is compared against that commit's own build. +Without a commit the run is compared against the latest master build. + +--job-name (DASHBOARD_JOB_NAME) checks as a build of that pipeline instead, with +its CI rules for the branch checked out here (--branch overrides it): the +previous build on a trunk branch, the merge base on any other branch. CI +passes --branch and --commit (or DASHBOARD_BRANCH and DASHBOARD_COMMIT), since +its checkout has no branch. + Exit status: 1 when the dashboard reports a failed rule for any checked run, or when a metadata.json is missing, malformed, or holds no numeric metric. 0 otherwise. A run the dashboard could not judge -- no baseline, nothing @@ -34,8 +47,10 @@ import argparse import concurrent.futures import json +import math import os import re +import subprocess import sys import time import urllib.error @@ -65,8 +80,6 @@ RETRY_BACKOFF_SECONDS = 1.0 RETRYABLE_STATUS = frozenset({429, 500, 502, 503, 504}) -DEFAULT_JOB_NAME = "OpenROAD-flow-scripts-Public" - # metadata.json records the ORFS commit, but genMetrics.py writes the strings # "git not on PATH" / "not a git repo" / "N/A" there when it cannot resolve one. SHA_RE = re.compile(r"^[0-9a-f]{40}$") @@ -148,7 +161,9 @@ class MetadataError(Exception): def load_metadata(path, only_prefix=None): - """Returns (metrics, commit_sha). Raises MetadataError if the file is unusable. + """Returns (metrics, commit_sha, dropped). Raises MetadataError if the file is unusable. + + dropped names the metrics left out because their value is not finite. only_prefix keeps just the metrics whose name starts with one of the given prefixes. The dashboard evaluates only the metrics it receives, so sending @@ -180,6 +195,14 @@ def load_metadata(path, only_prefix=None): # reported metric count honest. metrics = {key: value for key, value in metrics.items() if "__iter__" not in key} + # ORFS writes inf/-inf/NaN for a metric it could not compute (an unconstrained + # timing path reports -inf TNS). json.dumps renders those as bare Infinity/NaN + # tokens, which are not JSON: the dashboard rejects the whole request with + # HTTP 400, so one unusable metric would cost the run its verdict. A value + # that is not finite has nothing to compare against, so leave it out. + dropped = sorted(key for key, value in metrics.items() if not math.isfinite(value)) + metrics = {key: value for key, value in metrics.items() if key not in dropped} + if only_prefix: metrics = { key: value @@ -191,7 +214,27 @@ def load_metadata(path, only_prefix=None): raise MetadataError(f"no numeric metrics in {path}") commit = str(raw.get(COMMIT_KEY, "")) - return metrics, commit if SHA_RE.match(commit) else "" + return metrics, commit if SHA_RE.match(commit) else "", dropped + + +def current_branch(): + """The branch checked out in this repository, or "" when git cannot say. + + A detached HEAD reads as "HEAD", which the dashboard treats as any other + non-trunk branch: it compares against the merge base, which is the right + baseline for a detached checkout too. + """ + try: + output = subprocess.run( + ["git", "rev-parse", "--abbrev-ref", "HEAD"], + capture_output=True, + text=True, + timeout=10, + check=True, + ).stdout + except (OSError, subprocess.SubprocessError): + return "" + return output.strip() def post_check(endpoint, api_key, payload): @@ -234,8 +277,22 @@ def post_check(endpoint, api_key, payload): return None, f"{error_message} (after {RETRY_ATTEMPTS} attempts)" -def check_run(run, endpoint, api_key, base_commit, job_name, only_prefix=None): - """Checks one (platform, design, variant) and returns a result dict.""" +def check_run( + run, + endpoint, + api_key, + base_commit, + job_name, + only_prefix=None, + commit="", + branch="", +): + """Checks one (platform, design, variant) and returns a result dict. + + commit replaces the one metadata.json records. branch is sent only with a + job_name: without one the dashboard ignores it and resolves the baseline + from the commit alone. + """ platform, design, variant, metadata_path = run result = { "platform": platform, @@ -245,6 +302,7 @@ def check_run(run, endpoint, api_key, base_commit, job_name, only_prefix=None): "response": None, "error": None, "metric_count": 0, + "dropped": [], } # One run's surprise must not take down the sweep: this is called from a @@ -255,24 +313,33 @@ def check_run(run, endpoint, api_key, base_commit, job_name, only_prefix=None): # A bad local file is the caller's problem, not the service's: it gets # its own status so it exits 1 instead of passing as inconclusive. try: - metrics, commit = load_metadata(metadata_path, only_prefix) + metrics, recorded_commit, dropped = load_metadata( + metadata_path, only_prefix + ) except MetadataError as error: result["status"] = "invalid" result["error"] = str(error) return result result["metric_count"] = len(metrics) - + result["dropped"] = dropped + commit = commit or recorded_commit + result["commit"] = commit + + # No JobName and no PipelineId makes this a checkout to the dashboard: + # it picks the pipeline itself, the first of its default baseline + # pipelines with a build of this design. A JobName makes it a build of + # that pipeline, resolved by that pipeline's rules for BranchName. payload = { "CommitSha": commit, - "BranchName": "", + "BranchName": branch if job_name else "", "PipelineId": "", - "JobName": job_name, "PlatformName": platform, "DesignName": design, "VariantName": variant, "Metrics": metrics, - "IsLocal": True, } + if job_name: + payload["JobName"] = job_name if base_commit: payload["BaseCommitSha"] = base_commit @@ -334,10 +401,22 @@ def render_run(result, verbose): f"{passed} passed, {failed} failed, {missing} missing " f"({result['metric_count']} metrics sent)" ) + pipeline = response.get("baselinePipeline") lines.append( f"[INFO] baseline build {response.get('baseBuildId')} " - f"({(response.get('baseCommitSha') or '?')[:12]}, {strategy})" + f"({(response.get('baseCommitSha') or '?')[:12]}, {strategy}" + f"{', ' + pipeline if pipeline else ''})" ) + if not result.get("commit") and strategy == "latest-master": + lines.append( + "[WARN] metadata.json records no commit, so this was compared " + "against the latest master build." + ) + if result["dropped"]: + lines.append( + f"[WARN] Left out {len(result['dropped'])} non-finite metric(s): " + + ", ".join(result["dropped"]) + ) if result["status"] == "inconclusive": lines.append( @@ -501,15 +580,34 @@ def parse_args(argv): ) parser.add_argument( "--base-commit", - help="Pin the baseline to this commit's build instead of latest master.", + help="Pin the baseline to this commit's build instead of resolving one.", + ) + parser.add_argument( + "--api-url", + default=os.environ.get("DASHBOARD_API_URL") or None, + help="Dashboard API base URL, e.g. a staging deployment. Takes " + "precedence over --beta. Env: DASHBOARD_API_URL.", + ) + parser.add_argument( + "--commit", + default=os.environ.get("DASHBOARD_COMMIT", ""), + help="Full commit SHA to check as, instead of the one metadata.json " + "records. Env: DASHBOARD_COMMIT.", + ) + parser.add_argument( + "--branch", + default=os.environ.get("DASHBOARD_BRANCH") or None, + help="With --job-name: the branch to check as, instead of the one " + "checked out here, e.g. the PR branch in CI. " + "Env: DASHBOARD_BRANCH.", ) parser.add_argument( "--job-name", - default=os.environ.get("DASHBOARD_JOB_NAME", DEFAULT_JOB_NAME), - help="Canonical Jenkins job-folder name the dashboard uses to scope the master " - f"baseline to a pipeline. Default: {DEFAULT_JOB_NAME}. Override for a " - "secure/private branch whose master-equivalent builds elsewhere, e.g. " - "OpenROAD-flow-scripts-Private.", + default=os.environ.get("DASHBOARD_JOB_NAME") or None, + help="Check as a build of this CI pipeline, with its rules for the " + "branch. Default: the dashboard searches its default baseline " + "pipelines and takes the first with a build of the design. " + "Env: DASHBOARD_JOB_NAME.", ) parser.add_argument( "--metadata", @@ -612,8 +710,21 @@ def selected(entry): def run_check(argv=None): args = parse_args(argv) - endpoint = (BETA_API_URL if args.beta else API_URL) + CHECK_PATH + api_url = args.api_url or (BETA_API_URL if args.beta else API_URL) + endpoint = api_url.rstrip("/") + CHECK_PATH api_key = os.environ.get("DASHBOARD_API_KEY", "") + # The dashboard matches the commit exactly and silently falls back to the + # latest master build on a miss, so anything but a full SHA is an error. + commit = args.commit.strip().lower() + if commit and not SHA_RE.match(commit): + print( + f"[ERROR] --commit {args.commit.strip()!r} is not a full 40-character SHA." + ) + return 1 + # Only a pipeline run uses the branch; a checkout is resolved from the commit. + branch = "" + if args.job_name: + branch = (args.branch if args.branch is not None else current_branch()).strip() if args.metadata: # Single-run mode: the caller (make metadata-check) names the run and @@ -642,6 +753,21 @@ def run_check(argv=None): ) if args.base_commit: header.append(f"[INFO] Baseline pinned to commit {args.base_commit}") + elif args.job_name and branch: + header.append( + f"[INFO] Checking as a build of {args.job_name}, branch {branch}" + + (f", commit {commit}" if commit else "") + ) + elif args.job_name: + header.append( + f"[WARN] Checking as a build of {args.job_name} with no branch (git " + "could not name one, and no --branch): resolved as a feature branch." + ) + else: + header.append( + "[INFO] Baseline from the dashboard's default pipelines, resolved " + "from " + (f"commit {commit}" if commit else "each run's recorded commit") + ) if args.only_prefix: prefixes = " ".join(args.only_prefix) header.append(f"[INFO] Sending only metrics prefixed {prefixes}") @@ -661,6 +787,8 @@ def run_check(argv=None): args.base_commit, args.job_name, args.only_prefix, + commit, + branch, ), runs, ) diff --git a/flow/util/utils.mk b/flow/util/utils.mk index 195db144af5..ca3865e2b07 100644 --- a/flow/util/utils.mk +++ b/flow/util/utils.mk @@ -27,11 +27,13 @@ metadata-generate: -o $(REPORTS_DIR)/metadata.json 2>&1 \ | tee $(abspath $(REPORTS_DIR)/metadata-generate.log) -# The QoR gate: metadata.json against the latest master build on the QoR -# dashboard, using the dashboard's rule configs. Set DASHBOARD_API_KEY for -# a private platform and DASHBOARD_JOB_NAME when the baseline pipeline is -# not OpenROAD-flow-scripts-Public. A failed rule fails the target; an -# unreachable dashboard or a design without a baseline only warns. +# The QoR gate: metadata.json against the QoR dashboard's baseline for the +# commit it records (the master build at its merge base, from the first of +# the dashboard's default pipelines that has the design), using the +# dashboard's rule configs. Set DASHBOARD_API_KEY for a private platform and +# DASHBOARD_JOB_NAME to check as a build of one pipeline instead. A failed +# rule fails the target; an unreachable dashboard or a design without a +# baseline only warns. .PHONY: metadata-check metadata-check: $(PYTHON_EXE) $(UTILS_DIR)/checkQorMetrics.py \ From 216034b478e53727fa65be624f7a00ea77fce3bd Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:06:23 -0300 Subject: [PATCH 2/8] Pin an unpushed commit to its local merge base and warn on latest-master The dashboard finds a checkout's merge base through GitHub, so it cannot find one for a commit that was never pushed. It then compares against the latest master build, which may be newer than the commit's base: changes merged since then read as regressions or hide real ones, and the result moves with master. The report said nothing about it, and the docs claimed the opposite. When no remote branch in the checkout contains the run's commit, the check now computes its merge base with --merge-base (DASHBOARD_MERGE_BASE, default origin/master) and pins the baseline to it. A commit on a remote, or not in the checkout at all, such as a downloaded CI artifact, is still resolved by the dashboard. The pin is skipped with --base-commit or --job-name, since CI checkouts often fetch no remote branches. none turns it off. Every latest-master result now warns and points at --base-commit, which DASHBOARD_BASE_COMMIT now sets so that make metadata-check can reach it. The docs, the module docstring and the utils.mk comment describe the fallback as it is. Signed-off-by: Miguel Dalberto Pedro --- docs/contrib/Metrics.md | 39 ++++++++--- flow/util/checkQorMetrics.py | 122 ++++++++++++++++++++++++++++++++--- flow/util/utils.mk | 5 +- 3 files changed, 145 insertions(+), 21 deletions(-) diff --git a/docs/contrib/Metrics.md b/docs/contrib/Metrics.md index bd045aa56d4..00d99c68b48 100644 --- a/docs/contrib/Metrics.md +++ b/docs/contrib/Metrics.md @@ -8,8 +8,9 @@ a small set of example designs through our complete RTL-to-GDS flow. To keep track of the quality of the results, every CI run uploads its metrics to the [QoR dashboard](https://dashboard.precisioninno.com). The dashboard stores one rule per tracked metric and design (a comparison direction and a -tolerance), and judges a run against a real baseline build: the latest -`master` build by default. That check is the QoR gate in `make metadata`. +tolerance), and judges a run against a real baseline build: the `master` +build at the run's merge base by default. That check is the QoR gate in +`make metadata`. The values a run is measured against are collected by `flow/util/genMetrics.py` into `$(REPORTS_DIR)/metadata.json` @@ -49,12 +50,23 @@ check did not run. Read the `[WARN]` lines to see which. Pass `--strict` to The check runs as the commit `metadata.json` records. The dashboard searches its default baseline pipelines in order and takes a `master` build that has the -design: the one at the commit's merge base with `master`, otherwise the latest -one before the commit. A commit on `master` is its own merge base, so a run of -a `master` commit, such as a downloaded CI artifact, is compared against that -commit's own build. A commit not yet pushed has no merge base on GitHub, so it -is compared against the latest `master` build before it. Without a commit the -run is compared against the latest `master` build. +design: the one at the commit's merge base with `master`. A commit on `master` +is its own merge base, so a run of a `master` commit, such as a downloaded CI +artifact, is compared against that commit's own build. + +The dashboard finds the merge base through GitHub, so it cannot find one for a +commit that was never pushed. The check handles that commit itself: when no +remote branch in your checkout contains the commit, it computes +`git merge-base origin/master` locally and pins the baseline to that +build. A commit that is on a remote, or that is not in your checkout at all, is +left to the dashboard. + +When the dashboard has no `master` build at the merge base, or no merge base, +it compares against the latest `master` build instead (the latest one before +the commit, if it knows the commit's date). That build may be newer than the +commit's base, so changes merged since then can show up as regressions or hide +real ones, and the result moves with `master`. The report warns whenever this +happens; set `DASHBOARD_BASE_COMMIT` to compare against an exact build instead. These environment variables shape the comparison: @@ -67,6 +79,14 @@ These environment variables shape the comparison: other branch. A CI job, whose checkout has no branch, also sets `DASHBOARD_BRANCH` and `DASHBOARD_COMMIT` (or passes `--branch` and `--commit`). +- `DASHBOARD_MERGE_BASE` (`--merge-base`) names the ref an unpushed commit's + merge base is computed against, `origin/master` by default. Set it to your + upstream ref if `origin` is a fork with an outdated `master`, or to `none` to + leave unpushed commits to the dashboard. It does not apply together with + `DASHBOARD_BASE_COMMIT` or `DASHBOARD_JOB_NAME`. +- `DASHBOARD_BASE_COMMIT` (`--base-commit`) pins the baseline to the build of + that commit. Nothing else is tried: if that commit has no build of the + design, the check is inconclusive. - `DASHBOARD_API_URL` (`--api-url`) points the check at another deployment and takes precedence over `--beta`. @@ -78,8 +98,7 @@ are sent, so only their rules are evaluated: make metadata-synth ``` -To check every run under `reports/` at once, or to pin the baseline to a -specific commit, call the script directly: +To check every run under `reports/` at once, call the script directly: ``` shell cd OpenROAD-flow-scripts/flow diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index f7e6c4d434f..8747d425b74 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -5,8 +5,8 @@ The check POSTs a run's numeric metrics from metadata.json to the dashboard, which compares them against a real baseline build -- the master build at the -run's merge base, by default -- using the thresholds it stores in rule_configs. This is the QoR -gate behind `make metadata`. Rule tolerances live in the dashboard, not in +run's merge base, by default -- using the thresholds it stores in +rule_configs. This is the QoR gate behind `make metadata`. Rule tolerances live in the dashboard, not in this repository, and the baseline moves with master, so a merged change becomes the new baseline without a file update. @@ -24,9 +24,13 @@ Each run is checked as the commit metadata.json records (--commit overrides it). The dashboard searches its default baseline pipelines in order and takes a master build that has the design: the one at the commit's merge base with -master, else the latest one before the commit. A commit on master is its own -merge base, so a master run is compared against that commit's own build. -Without a commit the run is compared against the latest master build. +master. A commit on master is its own merge base, so a master run is compared +against that commit's own build. The dashboard finds the merge base through +GitHub, so for a commit that no remote branch here contains, the script +computes it locally against --merge-base (origin/master) and pins the +baseline to it. Without a merge base build, or without a commit, the run is +compared against the latest master build, which may be newer than the +commit's base; the report warns when that happens. --job-name (DASHBOARD_JOB_NAME) checks as a build of that pipeline instead, with its CI rules for the branch checked out here (--branch overrides it): the @@ -46,6 +50,7 @@ import argparse import concurrent.futures +import functools import json import math import os @@ -85,6 +90,8 @@ SHA_RE = re.compile(r"^[0-9a-f]{40}$") COMMIT_KEY = "run__flow__scripts_commit" +DEFAULT_MERGE_BASE_REF = "origin/master" + # NAME_W must match the first field width in ROW_FMT. NAME_W = 46 ROW_FMT = "| {:46} | {:>12} | {:>12} | {:>12} | {:>7} |" @@ -237,6 +244,45 @@ def current_branch(): return output.strip() +def git(*args): + """Runs git in this repository; returns its stdout, or None when it fails.""" + try: + return subprocess.run( + ["git", *args], + capture_output=True, + text=True, + timeout=30, + check=True, + ).stdout.strip() + except (OSError, subprocess.SubprocessError): + return None + + +@functools.lru_cache(maxsize=None) +def local_merge_base(commit, ref): + """Returns (sha, error) for pinning a commit that no remote has. + + The dashboard finds a merge base through GitHub, so a commit that was never + pushed falls back to the latest master build, which may be newer than the + commit's base. Both are None when the dashboard can resolve the commit + itself: it is on a remote branch, or it is not in this repository (a + downloaded CI artifact). error is set when the commit is local but has no + merge base with ref. + """ + if git("cat-file", "-e", f"{commit}^{{commit}}") is None: + return None, None + if git("for-each-ref", "--count=1", "--contains", commit, "refs/remotes"): + return None, None + sha = git("merge-base", commit, ref) + if not sha: + return None, ( + f"commit {commit[:12]} is on no remote and has no merge base with " + f"{ref}. Set DASHBOARD_MERGE_BASE to another ref, or to none to " + "compare against the latest master build." + ) + return sha, None + + def post_check(endpoint, api_key, payload): """POSTs one run, retrying transient failures. @@ -286,12 +332,14 @@ def check_run( only_prefix=None, commit="", branch="", + merge_base_ref=None, ): """Checks one (platform, design, variant) and returns a result dict. commit replaces the one metadata.json records. branch is sent only with a job_name: without one the dashboard ignores it and resolves the baseline - from the commit alone. + from the commit alone. merge_base_ref, when set, pins a commit that no + remote has to its merge base with that ref. """ platform, design, variant, metadata_path = run result = { @@ -303,6 +351,7 @@ def check_run( "error": None, "metric_count": 0, "dropped": [], + "merge_base": None, } # One run's surprise must not take down the sweep: this is called from a @@ -325,6 +374,14 @@ def check_run( commit = commit or recorded_commit result["commit"] = commit + if merge_base_ref and commit and not base_commit: + base_commit, error = local_merge_base(commit, merge_base_ref) + if error: + result["status"] = "invalid" + result["error"] = error + return result + result["merge_base"] = base_commit + # No JobName and no PipelineId makes this a checkout to the dashboard: # it picks the pipeline itself, the first of its default baseline # pipelines with a build of this design. A JobName makes it a build of @@ -388,6 +445,13 @@ def render_run(result, verbose): f"[WARN] {identity}: no baseline to compare against " f"(strategy={strategy}). Nothing was checked." ) + if result["merge_base"]: + lines.append( + f"[WARN] The baseline was pinned to {result['merge_base'][:12]}, " + "the local merge base, which has no build of this design. Set " + "DASHBOARD_MERGE_BASE=none to compare against the latest master " + "build instead." + ) return lines totals = response.get("totals", {}) @@ -407,11 +471,27 @@ def render_run(result, verbose): f"({(response.get('baseCommitSha') or '?')[:12]}, {strategy}" f"{', ' + pipeline if pipeline else ''})" ) - if not result.get("commit") and strategy == "latest-master": + if result["merge_base"]: lines.append( - "[WARN] metadata.json records no commit, so this was compared " - "against the latest master build." + f"[INFO] {result['commit'][:12]} is on no remote, so the baseline " + f"was pinned to its local merge base {result['merge_base'][:12]}" ) + # The latest master build may be newer than the commit's base, so upstream + # changes since then would read as this run's regressions, and the result + # moves with master. + if strategy == "latest-master": + if result.get("commit"): + lines.append( + f"[WARN] The dashboard found no master build at the merge base " + f"of {result['commit'][:12]}, so this was compared against the " + "latest master build, which may be newer than the commit's base. " + "Set DASHBOARD_BASE_COMMIT (--base-commit) for an exact baseline." + ) + else: + lines.append( + "[WARN] metadata.json records no commit, so this was compared " + "against the latest master build." + ) if result["dropped"]: lines.append( f"[WARN] Left out {len(result['dropped'])} non-finite metric(s): " @@ -580,7 +660,18 @@ def parse_args(argv): ) parser.add_argument( "--base-commit", - help="Pin the baseline to this commit's build instead of resolving one.", + default=os.environ.get("DASHBOARD_BASE_COMMIT") or None, + help="Pin the baseline to this commit's build instead of resolving one. " + "Env: DASHBOARD_BASE_COMMIT.", + ) + parser.add_argument( + "--merge-base", + default=os.environ.get("DASHBOARD_MERGE_BASE") or DEFAULT_MERGE_BASE_REF, + metavar="REF", + help="Pin a commit that is on no remote branch to its merge base with " + f"REF, since the dashboard cannot find one. Default: " + f"{DEFAULT_MERGE_BASE_REF}; none turns it off. Ignored with --base-commit " + "or --job-name. Env: DASHBOARD_MERGE_BASE.", ) parser.add_argument( "--api-url", @@ -721,6 +812,11 @@ def run_check(argv=None): f"[ERROR] --commit {args.commit.strip()!r} is not a full 40-character SHA." ) return 1 + # A pipeline run follows its CI branch rules, and CI checkouts often fetch + # no remote branches, so every commit there would look unpushed. + merge_base_ref = None + if not (args.base_commit or args.job_name) and args.merge_base.lower() != "none": + merge_base_ref = args.merge_base # Only a pipeline run uses the branch; a checkout is resolved from the commit. branch = "" if args.job_name: @@ -768,6 +864,11 @@ def run_check(argv=None): "[INFO] Baseline from the dashboard's default pipelines, resolved " "from " + (f"commit {commit}" if commit else "each run's recorded commit") ) + if merge_base_ref: + header.append( + f"[INFO] A commit on no remote branch is pinned to its merge base with " + f"{merge_base_ref} (DASHBOARD_MERGE_BASE=none turns this off)" + ) if args.only_prefix: prefixes = " ".join(args.only_prefix) header.append(f"[INFO] Sending only metrics prefixed {prefixes}") @@ -789,6 +890,7 @@ def run_check(argv=None): args.only_prefix, commit, branch, + merge_base_ref, ), runs, ) diff --git a/flow/util/utils.mk b/flow/util/utils.mk index ca3865e2b07..15c20df73d3 100644 --- a/flow/util/utils.mk +++ b/flow/util/utils.mk @@ -30,7 +30,10 @@ metadata-generate: # The QoR gate: metadata.json against the QoR dashboard's baseline for the # commit it records (the master build at its merge base, from the first of # the dashboard's default pipelines that has the design), using the -# dashboard's rule configs. Set DASHBOARD_API_KEY for a private platform and +# dashboard's rule configs. An unpushed commit's merge base is computed +# locally against DASHBOARD_MERGE_BASE (default origin/master); without one +# the latest master build is used, with a warning. Set DASHBOARD_BASE_COMMIT +# to pin the baseline, DASHBOARD_API_KEY for a private platform and # DASHBOARD_JOB_NAME to check as a build of one pipeline instead. A failed # rule fails the target; an unreachable dashboard or a design without a # baseline only warns. From f1f341e1f260435db75f02567000ed58b93e8484 Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:17:34 -0300 Subject: [PATCH 3/8] Test checkQorMetrics.py against a stubbed dashboard The payload mode, --commit validation, branch resolution, the local merge base pin and the metric filter decide which baseline a run is compared against, and nothing covered them. The tests call run_check() with urlopen replaced by a stub that records each request and replays canned replies, and check the payload, the report and the exit code. The merge base cases run in a temporary repository with a fake origin/master, so the state of the checkout running the tests does not matter. Signed-off-by: Miguel Dalberto Pedro --- flow/test/test_checkQorMetrics.py | 464 ++++++++++++++++++++++++++++++ 1 file changed, 464 insertions(+) create mode 100644 flow/test/test_checkQorMetrics.py diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py new file mode 100644 index 00000000000..0db0a01d8c4 --- /dev/null +++ b/flow/test/test_checkQorMetrics.py @@ -0,0 +1,464 @@ +#!/usr/bin/env python3 + +import contextlib +import io +import json +import os +import subprocess +import sys +import tempfile +import unittest +import urllib.error +from unittest.mock import patch + +sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "util")) + +import checkQorMetrics + +COMMIT = "a" * 40 +BASE = "b" * 40 + + +class FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *args): + self.close() + + +def response(status="pass", strategy="merge-base", failed=0, metrics=()): + return { + "status": status, + "baselineStrategy": strategy, + "baseBuildId": 7, + "baseCommitSha": BASE, + "totals": {"passed": 1, "failed": failed, "missingFromBaseline": 0}, + "metrics": list(metrics), + } + + +class Dashboard: + """Stands in for urlopen: records each payload and answers with replies in turn.""" + + def __init__(self, *replies): + self.replies = list(replies) or [response()] + self.payloads = [] + + def __call__(self, request, timeout): + self.payloads.append(json.loads(request.data)) + reply = self.replies[min(len(self.payloads), len(self.replies)) - 1] + if isinstance(reply, Exception): + raise reply + return FakeResponse(json.dumps(reply).encode("utf-8")) + + +def git(cwd, *args): + return subprocess.run( + ["git", *args], cwd=cwd, check=True, capture_output=True, text=True + ).stdout.strip() + + +class CheckQorMetricsTest(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + environment = { + key: value + for key, value in os.environ.items() + if not key.startswith("DASHBOARD_") + } + environment.update( + GIT_AUTHOR_NAME="test", + GIT_AUTHOR_EMAIL="test@example.com", + GIT_COMMITTER_NAME="test", + GIT_COMMITTER_EMAIL="test@example.com", + GIT_CONFIG_GLOBAL=os.devnull, + GIT_CONFIG_NOSYSTEM="1", + ) + for patcher in ( + patch.dict(os.environ, environment, clear=True), + patch.object(checkQorMetrics.time, "sleep"), + ): + patcher.start() + self.addCleanup(patcher.stop) + checkQorMetrics.local_merge_base.cache_clear() + self.addCleanup(checkQorMetrics.local_merge_base.cache_clear) + # git runs in the current directory, which is flow/ after the import. + # An empty directory outside any repository keeps this checkout out of it. + self.enter(self.tmp.name) + + def enter(self, directory): + previous = os.getcwd() + os.chdir(directory) + self.addCleanup(os.chdir, previous) + + def metadata(self, name="metadata.json", commit=COMMIT, **metrics): + metrics = metrics or {"finish__timing__setup__ws": 1.5} + path = os.path.join(self.tmp.name, name) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w", encoding="utf-8") as metadata_file: + # allow_nan writes the bare Infinity/NaN tokens genMetrics.py writes. + json.dump( + {checkQorMetrics.COMMIT_KEY: commit, **metrics}, + metadata_file, + allow_nan=True, + ) + return path + + def run_check(self, *args, dashboard=None, metadata=None): + """Returns (exit code, output, payloads) for one single-run check.""" + dashboard = dashboard or Dashboard() + argv = ["-m", metadata or self.metadata(), "-p", "asap7", "-d", "gcd"] + output = io.StringIO() + with patch.object( + checkQorMetrics.urllib.request, "urlopen", dashboard + ), contextlib.redirect_stdout(output): + code = checkQorMetrics.run_check(argv + list(args)) + return code, output.getvalue(), dashboard.payloads + + def repository(self): + """A repository with a master commit on origin and an unpushed one after it. + + Returns (repository, master_sha, unpushed_sha). + """ + repository = os.path.join(self.tmp.name, "repo") + os.makedirs(repository) + git(repository, "init", "-q", "-b", "master") + git(repository, "commit", "-q", "--allow-empty", "-m", "master") + master = git(repository, "rev-parse", "HEAD") + git(repository, "update-ref", "refs/remotes/origin/master", master) + git(repository, "commit", "-q", "--allow-empty", "-m", "local") + return repository, master, git(repository, "rev-parse", "HEAD") + + +class PayloadTest(CheckQorMetricsTest): + def test_checkout_sends_only_the_commit(self): + code, _, payloads = self.run_check() + self.assertEqual(code, 0) + payload = payloads[0] + self.assertEqual(payload["CommitSha"], COMMIT) + self.assertEqual(payload["BranchName"], "") + self.assertEqual(payload["PipelineId"], "") + self.assertNotIn("JobName", payload) + self.assertNotIn("BaseCommitSha", payload) + self.assertEqual( + (payload["PlatformName"], payload["DesignName"], payload["VariantName"]), + ("asap7", "gcd", "base"), + ) + self.assertEqual(payload["Metrics"], {"finish__timing__setup__ws": 1.5}) + + def test_branch_is_dropped_without_a_job_name(self): + _, _, payloads = self.run_check("--branch", "feature") + self.assertEqual(payloads[0]["BranchName"], "") + + def test_job_name_sends_the_branch(self): + _, _, payloads = self.run_check("--job-name", "orfs-ci", "--branch", "feature") + self.assertEqual(payloads[0]["JobName"], "orfs-ci") + self.assertEqual(payloads[0]["BranchName"], "feature") + + def test_environment_sets_job_branch_and_commit(self): + os.environ.update( + DASHBOARD_JOB_NAME="orfs-ci", + DASHBOARD_BRANCH="pr-1", + DASHBOARD_COMMIT=BASE, + ) + _, _, payloads = self.run_check() + self.assertEqual(payloads[0]["JobName"], "orfs-ci") + self.assertEqual(payloads[0]["BranchName"], "pr-1") + self.assertEqual(payloads[0]["CommitSha"], BASE) + + def test_base_commit_pins_the_baseline(self): + _, output, payloads = self.run_check("--base-commit", BASE) + self.assertEqual(payloads[0]["BaseCommitSha"], BASE) + self.assertIn(f"Baseline pinned to commit {BASE}", output) + + def test_base_commit_from_the_environment(self): + os.environ["DASHBOARD_BASE_COMMIT"] = BASE + _, _, payloads = self.run_check() + self.assertEqual(payloads[0]["BaseCommitSha"], BASE) + + def test_commit_flag_replaces_the_recorded_commit(self): + _, _, payloads = self.run_check("--commit", BASE.upper()) + self.assertEqual(payloads[0]["CommitSha"], BASE) + + def test_unresolved_recorded_commit_is_sent_empty(self): + _, output, payloads = self.run_check( + metadata=self.metadata(commit="not a git repo"), + dashboard=Dashboard(response(strategy="latest-master")), + ) + self.assertEqual(payloads[0]["CommitSha"], "") + self.assertIn("metadata.json records no commit", output) + + +class CommitValidationTest(CheckQorMetricsTest): + def test_short_commit_is_rejected_before_any_request(self): + code, output, payloads = self.run_check("--commit", "abc1234") + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn("is not a full 40-character SHA", output) + + +class BranchTest(CheckQorMetricsTest): + def test_branch_comes_from_git(self): + repository, _, _ = self.repository() + git(repository, "checkout", "-q", "-b", "feature") + self.enter(repository) + _, _, payloads = self.run_check("--job-name", "orfs-ci", "--merge-base", "none") + self.assertEqual(payloads[0]["BranchName"], "feature") + + def test_branch_flag_beats_git(self): + repository, _, _ = self.repository() + self.enter(repository) + _, _, payloads = self.run_check("--job-name", "orfs-ci", "--branch", "pr-2") + self.assertEqual(payloads[0]["BranchName"], "pr-2") + + def test_detached_head_reads_as_head(self): + repository, master, _ = self.repository() + git(repository, "checkout", "-q", "--detach", master) + self.enter(repository) + _, _, payloads = self.run_check("--job-name", "orfs-ci") + self.assertEqual(payloads[0]["BranchName"], "HEAD") + + def test_no_branch_outside_a_repository_warns(self): + _, output, payloads = self.run_check("--job-name", "orfs-ci") + self.assertEqual(payloads[0]["BranchName"], "") + self.assertIn("with no branch", output) + + +class MergeBaseTest(CheckQorMetricsTest): + def check(self, commit, *args): + return self.run_check( + *args, + metadata=self.metadata(commit=commit), + dashboard=Dashboard(response(strategy="explicit-commit")), + ) + + def test_unpushed_commit_is_pinned_to_its_merge_base(self): + repository, master, unpushed = self.repository() + self.enter(repository) + code, output, payloads = self.check(unpushed) + self.assertEqual(code, 0) + self.assertEqual(payloads[0]["BaseCommitSha"], master) + self.assertEqual(payloads[0]["CommitSha"], unpushed) + self.assertIn(f"pinned to its local merge base {master[:12]}", output) + + def test_pushed_commit_is_left_to_the_dashboard(self): + repository, _, unpushed = self.repository() + git(repository, "update-ref", "refs/remotes/origin/feature", unpushed) + self.enter(repository) + _, _, payloads = self.check(unpushed) + self.assertNotIn("BaseCommitSha", payloads[0]) + + def test_commit_missing_from_the_checkout_is_left_to_the_dashboard(self): + repository, _, _ = self.repository() + self.enter(repository) + _, _, payloads = self.check(COMMIT) + self.assertNotIn("BaseCommitSha", payloads[0]) + + def test_unknown_ref_is_an_error(self): + repository, _, unpushed = self.repository() + self.enter(repository) + code, output, payloads = self.check(unpushed, "--merge-base", "upstream/master") + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn("has no merge base with upstream/master", output) + + def test_ref_from_the_environment(self): + repository, _, unpushed = self.repository() + git(repository, "branch", "trunk", unpushed) + os.environ["DASHBOARD_MERGE_BASE"] = "trunk" + self.enter(repository) + _, _, payloads = self.check(unpushed) + self.assertEqual(payloads[0]["BaseCommitSha"], unpushed) + + def test_none_turns_it_off(self): + repository, _, unpushed = self.repository() + self.enter(repository) + _, _, payloads = self.check(unpushed, "--merge-base", "none") + self.assertNotIn("BaseCommitSha", payloads[0]) + + def test_base_commit_wins(self): + repository, _, unpushed = self.repository() + self.enter(repository) + _, _, payloads = self.check(unpushed, "--base-commit", BASE) + self.assertEqual(payloads[0]["BaseCommitSha"], BASE) + + def test_job_name_skips_it(self): + repository, _, unpushed = self.repository() + self.enter(repository) + _, _, payloads = self.check(unpushed, "--job-name", "orfs-ci") + self.assertNotIn("BaseCommitSha", payloads[0]) + + def test_pinned_merge_base_without_a_build_suggests_turning_it_off(self): + repository, _, unpushed = self.repository() + self.enter(repository) + code, output, _ = self.run_check( + metadata=self.metadata(commit=unpushed), + dashboard=Dashboard(response(status="no_baseline", strategy="none")), + ) + self.assertEqual(code, 0) + self.assertIn("DASHBOARD_MERGE_BASE=none", output) + + +class MetricFilterTest(CheckQorMetricsTest): + def test_non_finite_metrics_are_left_out_and_named(self): + path = self.metadata( + finish__timing__setup__ws=1.5, + finish__timing__setup__tns=float("-inf"), + detailedroute__route__wirelength=float("nan"), + ) + _, output, payloads = self.run_check(metadata=path) + self.assertEqual(payloads[0]["Metrics"], {"finish__timing__setup__ws": 1.5}) + self.assertIn( + "non-finite metric(s): detailedroute__route__wirelength, " + "finish__timing__setup__tns", + output, + ) + + def test_strings_booleans_and_iterations_are_not_sent(self): + path = self.metadata( + finish__timing__setup__ws=1.5, + run__flow__platform="asap7", + synth__flag=True, + detailedroute__route__drc_errors__iter__1=3, + ) + _, _, payloads = self.run_check(metadata=path) + self.assertEqual(payloads[0]["Metrics"], {"finish__timing__setup__ws": 1.5}) + + def test_only_prefix_keeps_matching_metrics(self): + path = self.metadata(synth__design__area=10, finish__timing__setup__ws=1.5) + _, _, payloads = self.run_check("--only-prefix", "synth__", metadata=path) + self.assertEqual(payloads[0]["Metrics"], {"synth__design__area": 10}) + + def test_no_numeric_metric_is_an_error(self): + path = self.metadata(run__flow__platform="asap7") + code, output, payloads = self.run_check(metadata=path) + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn("no numeric metrics", output) + self.assertIn("QoR check: ERROR", output) + + +class ReportTest(CheckQorMetricsTest): + def test_pass(self): + code, output, _ = self.run_check() + self.assertEqual(code, 0) + self.assertTrue(output.rstrip().endswith("QoR check: PASS")) + + def test_fail_lists_the_failed_rule(self): + failed = { + "metricName": "finish__timing__setup__ws", + "status": "fail", + "baselineValue": 2, + "currentValue": 1.5, + "limit": 1.9, + "deltaPct": -25, + } + code, output, _ = self.run_check( + dashboard=Dashboard(response(status="fail", failed=1, metrics=[failed])) + ) + self.assertEqual(code, 1) + self.assertIn("finish__timing__setup__ws", output) + self.assertIn("-25.00", output) + self.assertIn("QoR check: FAIL", output) + + def test_latest_master_warns_with_a_commit(self): + _, output, _ = self.run_check( + dashboard=Dashboard(response(strategy="latest-master")) + ) + self.assertIn(f"merge base of {COMMIT[:12]}", output) + self.assertIn("DASHBOARD_BASE_COMMIT", output) + + def test_merge_base_does_not_warn(self): + _, output, _ = self.run_check() + self.assertNotIn("[WARN]", output) + + def test_inconclusive_exits_0_or_2_with_strict(self): + reply = response(status="inconclusive") + code, output, _ = self.run_check(dashboard=Dashboard(reply)) + self.assertEqual(code, 0) + self.assertIn("QoR check: INCONCLUSIVE", output) + code, _, _ = self.run_check("--strict", dashboard=Dashboard(reply)) + self.assertEqual(code, 2) + + def test_no_baseline_is_inconclusive(self): + code, output, _ = self.run_check( + "--strict", dashboard=Dashboard(response(status="no_baseline")) + ) + self.assertEqual(code, 2) + self.assertIn("no baseline to compare against", output) + + def test_unreachable_retries_then_exits_0_or_3_with_strict(self): + down = urllib.error.URLError("connection refused") + dashboard = Dashboard(down) + code, output, payloads = self.run_check(dashboard=dashboard) + self.assertEqual(code, 0) + self.assertEqual(len(payloads), checkQorMetrics.RETRY_ATTEMPTS) + self.assertIn("[WARN] asap7/gcd/base: dashboard not reached", output) + self.assertNotIn("[ERROR]", output) + code, _, _ = self.run_check("--strict", dashboard=Dashboard(down)) + self.assertEqual(code, 3) + + def test_transient_error_is_retried(self): + busy = urllib.error.HTTPError("url", 503, "busy", {}, io.BytesIO(b"try again")) + self.addCleanup(busy.close) + code, _, payloads = self.run_check(dashboard=Dashboard(busy, response())) + self.assertEqual(code, 0) + self.assertEqual(len(payloads), 2) + + def test_client_error_is_not_retried(self): + bad = urllib.error.HTTPError("url", 400, "bad", {}, io.BytesIO(b"no")) + self.addCleanup(bad.close) + _, output, payloads = self.run_check(dashboard=Dashboard(bad)) + self.assertEqual(len(payloads), 1) + self.assertIn("HTTP 400: no", output) + + def test_non_object_response_is_unreachable(self): + code, output, _ = self.run_check("--strict", dashboard=Dashboard([1, 2])) + self.assertEqual(code, 3) + self.assertIn("unexpected response type: list", output) + + +class SweepTest(CheckQorMetricsTest): + def run_sweep(self, *args, dashboard=None): + dashboard = dashboard or Dashboard() + output = io.StringIO() + with patch.object( + checkQorMetrics.urllib.request, "urlopen", dashboard + ), contextlib.redirect_stdout(output): + code = checkQorMetrics.run_check(list(args)) + return code, output.getvalue(), dashboard.payloads + + def test_sweep_checks_each_run_and_writes_the_log(self): + self.metadata("reports/asap7/gcd/base/metadata.json") + self.metadata("reports/nangate45/aes/base/metadata.json") + os.makedirs("reports/asap7/jpeg/base") + os.makedirs("reports/asap7/gcd/test_variant") + code, output, payloads = self.run_sweep() + self.assertEqual(code, 0) + self.assertCountEqual( + [(p["PlatformName"], p["DesignName"]) for p in payloads], + [("asap7", "gcd"), ("nangate45", "aes")], + ) + self.assertLess(output.index("asap7/gcd/base"), output.index("nangate45/aes")) + self.assertIn("No metadata.json for asap7 jpeg base", output) + with open(checkQorMetrics.LOG_FILENAME, encoding="utf-8") as log_file: + self.assertIn("QoR check: PASS", log_file.read()) + + def test_sweep_filters_by_design(self): + self.metadata("reports/asap7/gcd/base/metadata.json") + self.metadata("reports/asap7/aes/base/metadata.json") + _, _, payloads = self.run_sweep("-d", "aes") + self.assertEqual([p["DesignName"] for p in payloads], ["aes"]) + + def test_no_reports_is_0_or_2_with_strict(self): + code, _, payloads = self.run_sweep() + self.assertEqual(code, 0) + self.assertEqual(payloads, []) + code, _, _ = self.run_sweep("--strict") + self.assertEqual(code, 2) + + +if __name__ == "__main__": + unittest.main() From f69f684a80a3d3378b5d1f40ee82f14c905dcb67 Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:32:28 -0300 Subject: [PATCH 4/8] Replace a stale sweep log when --commit is rejected A --commit that is not a full SHA returned before the sweep log was written, so reports/inline-check.log kept the previous sweep's report, possibly a PASS for a check that never ran. The error now replaces it. A checkout without a reports/ directory still gets no log, since there is nothing to go stale. Signed-off-by: Miguel Dalberto Pedro --- flow/test/test_checkQorMetrics.py | 17 +++++++++++++++++ flow/util/checkQorMetrics.py | 5 ++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py index 0db0a01d8c4..82adb188602 100644 --- a/flow/test/test_checkQorMetrics.py +++ b/flow/test/test_checkQorMetrics.py @@ -446,6 +446,23 @@ def test_sweep_checks_each_run_and_writes_the_log(self): with open(checkQorMetrics.LOG_FILENAME, encoding="utf-8") as log_file: self.assertIn("QoR check: PASS", log_file.read()) + def test_bad_commit_replaces_a_stale_log(self): + self.metadata("reports/asap7/gcd/base/metadata.json") + with open(checkQorMetrics.LOG_FILENAME, "w", encoding="utf-8") as log_file: + log_file.write("QoR check: PASS\n") + code, _, payloads = self.run_sweep("--commit", "abc1234") + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + with open(checkQorMetrics.LOG_FILENAME, encoding="utf-8") as log_file: + log = log_file.read() + self.assertIn("is not a full 40-character SHA", log) + self.assertNotIn("QoR check: PASS", log) + + def test_bad_commit_without_reports_writes_no_log(self): + code, _, _ = self.run_sweep("--commit", "abc1234") + self.assertEqual(code, 1) + self.assertFalse(os.path.exists(checkQorMetrics.REPORTS_FOLDER)) + def test_sweep_filters_by_design(self): self.metadata("reports/asap7/gcd/base/metadata.json") self.metadata("reports/asap7/aes/base/metadata.json") diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 8747d425b74..2ff79a16d9a 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -808,9 +808,12 @@ def run_check(argv=None): # latest master build on a miss, so anything but a full SHA is an error. commit = args.commit.strip().lower() if commit and not SHA_RE.match(commit): - print( + line = ( f"[ERROR] --commit {args.commit.strip()!r} is not a full 40-character SHA." ) + print(line) + if not args.metadata and os.path.isdir(REPORTS_FOLDER): + write_log([line]) return 1 # A pipeline run follows its CI branch rules, and CI checkouts often fetch # no remote branches, so every commit there would look unpushed. From f573df5f2f1db6b6ede756016c9555ef26417200 Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:32:28 -0300 Subject: [PATCH 5/8] Report non-finite metrics only for what would be sent The non-finite metrics were listed before the --only-prefix filter, so metadata-check-synth warned about finish__ metrics it never sends. They are now split off after it. The warning also reaches the no_baseline report, which returned before printing it. A file whose values are all non-finite said "no numeric metrics", which sends the reader looking for an empty metadata.json; the error now names the non-finite metrics. A prefix that matches nothing says so, too. Signed-off-by: Miguel Dalberto Pedro --- flow/test/test_checkQorMetrics.py | 41 +++++++++++++++++++++++++++++++ flow/util/checkQorMetrics.py | 40 +++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py index 82adb188602..31e03ca6862 100644 --- a/flow/test/test_checkQorMetrics.py +++ b/flow/test/test_checkQorMetrics.py @@ -331,6 +331,47 @@ def test_only_prefix_keeps_matching_metrics(self): _, _, payloads = self.run_check("--only-prefix", "synth__", metadata=path) self.assertEqual(payloads[0]["Metrics"], {"synth__design__area": 10}) + def test_non_finite_metric_outside_the_prefix_is_not_named(self): + path = self.metadata( + synth__design__area=10, finish__timing__setup__tns=float("-inf") + ) + _, output, payloads = self.run_check("--only-prefix", "synth__", metadata=path) + self.assertEqual(payloads[0]["Metrics"], {"synth__design__area": 10}) + self.assertNotIn("non-finite", output) + + def test_non_finite_metrics_are_named_without_a_baseline(self): + path = self.metadata( + finish__timing__setup__ws=1.5, finish__timing__setup__tns=float("-inf") + ) + _, output, _ = self.run_check( + metadata=path, dashboard=Dashboard(response(status="no_baseline")) + ) + self.assertIn("non-finite metric(s): finish__timing__setup__tns", output) + + def test_only_non_finite_metrics_is_an_error_naming_them(self): + path = self.metadata( + finish__timing__setup__tns=float("-inf"), + finish__timing__setup__ws=float("nan"), + ) + code, output, payloads = self.run_check(metadata=path) + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn( + "no finite numeric metrics in " + f"{path}; non-finite: finish__timing__setup__tns, " + "finish__timing__setup__ws", + output, + ) + + def test_no_metric_matching_the_prefix_is_an_error_naming_it(self): + path = self.metadata(finish__timing__setup__ws=1.5) + code, output, payloads = self.run_check( + "--only-prefix", "synth__", "constraints__", metadata=path + ) + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn("starts with synth__ or constraints__", output) + def test_no_numeric_metric_is_an_error(self): path = self.metadata(run__flow__platform="asap7") code, output, payloads = self.run_check(metadata=path) diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 2ff79a16d9a..2d3a57e075c 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -202,6 +202,17 @@ def load_metadata(path, only_prefix=None): # reported metric count honest. metrics = {key: value for key, value in metrics.items() if "__iter__" not in key} + if only_prefix: + metrics = { + key: value + for key, value in metrics.items() + if any(key.startswith(prefix) for prefix in only_prefix) + } + if not metrics: + raise MetadataError( + f"no numeric metric in {path} starts with " + " or ".join(only_prefix) + ) + # ORFS writes inf/-inf/NaN for a metric it could not compute (an unconstrained # timing path reports -inf TNS). json.dumps renders those as bare Infinity/NaN # tokens, which are not JSON: the dashboard rejects the whole request with @@ -210,14 +221,12 @@ def load_metadata(path, only_prefix=None): dropped = sorted(key for key, value in metrics.items() if not math.isfinite(value)) metrics = {key: value for key, value in metrics.items() if key not in dropped} - if only_prefix: - metrics = { - key: value - for key, value in metrics.items() - if any(key.startswith(prefix) for prefix in only_prefix) - } - if not metrics: + if dropped: + raise MetadataError( + f"no finite numeric metrics in {path}; non-finite: " + + ", ".join(dropped) + ) raise MetadataError(f"no numeric metrics in {path}") commit = str(raw.get(COMMIT_KEY, "")) @@ -421,6 +430,16 @@ def check_run( return result +def dropped_lines(result): + """The warning naming the non-finite metrics left out of the request, if any.""" + if not result["dropped"]: + return [] + return [ + f"[WARN] Left out {len(result['dropped'])} non-finite metric(s): " + + ", ".join(result["dropped"]) + ] + + def render_run(result, verbose): """Builds the report lines for one run.""" identity = f"{result['platform']}/{result['design']}/{result['variant']}" @@ -452,6 +471,7 @@ def render_run(result, verbose): "DASHBOARD_MERGE_BASE=none to compare against the latest master " "build instead." ) + lines.extend(dropped_lines(result)) return lines totals = response.get("totals", {}) @@ -492,11 +512,7 @@ def render_run(result, verbose): "[WARN] metadata.json records no commit, so this was compared " "against the latest master build." ) - if result["dropped"]: - lines.append( - f"[WARN] Left out {len(result['dropped'])} non-finite metric(s): " - + ", ".join(result["dropped"]) - ) + lines.extend(dropped_lines(result)) if result["status"] == "inconclusive": lines.append( From 9a4eb605069de27eb1fc589b34bc1863a9495caf Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:32:28 -0300 Subject: [PATCH 6/8] Document that a private platform needs only DASHBOARD_API_KEY DASHBOARD_JOB_NAME used to be how a local run picked the pipeline holding a private platform's baseline. It now checks the run as a CI build, with that pipeline's branch rules, and turns off the local merge base pin. The default baseline pipelines include the private ones, visible to a caller with a key, so a local run needs only DASHBOARD_API_KEY. The docs, the module docstring and the --job-name help now say so. Signed-off-by: Miguel Dalberto Pedro --- docs/contrib/Metrics.md | 17 +++++++++++------ flow/util/checkQorMetrics.py | 6 ++++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/docs/contrib/Metrics.md b/docs/contrib/Metrics.md index 00d99c68b48..8b66f546702 100644 --- a/docs/contrib/Metrics.md +++ b/docs/contrib/Metrics.md @@ -70,15 +70,20 @@ happens; set `DASHBOARD_BASE_COMMIT` to compare against an exact build instead. These environment variables shape the comparison: -- `DASHBOARD_API_KEY` grants access to private platforms. Without it, a - private design has no baseline and reports as inconclusive, not as an - authentication error. +- `DASHBOARD_API_KEY` grants access to private platforms. With it, a private + design is resolved like a public one: the dashboard's default baseline + pipelines include the private ones, and only a caller with a key can see + their builds. Without it, a private design has no baseline and reports as + inconclusive, not as an authentication error. - `DASHBOARD_JOB_NAME` checks the run as a build of that CI pipeline instead, with the pipeline's rules for the branch checked out in this repository: the previous build on a trunk branch such as `master`, the merge base on any - other branch. A CI job, whose checkout has no branch, also sets - `DASHBOARD_BRANCH` and `DASHBOARD_COMMIT` (or passes `--branch` and - `--commit`). + other branch. It is meant for CI jobs, which also set `DASHBOARD_BRANCH` and + `DASHBOARD_COMMIT` (or pass `--branch` and `--commit`) since their checkout + has no branch. Earlier versions of the check used it to pick the pipeline + that holds a private platform's baseline; that is no longer needed, so leave + it unset for a local run, private or not. With it set, an unpushed commit is + not pinned to its local merge base. - `DASHBOARD_MERGE_BASE` (`--merge-base`) names the ref an unpushed commit's merge base is computed against, `origin/master` by default. Set it to your upstream ref if `origin` is a fork with an outdated `master`, or to `none` to diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 2d3a57e075c..46935cd1db3 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -36,7 +36,8 @@ its CI rules for the branch checked out here (--branch overrides it): the previous build on a trunk branch, the merge base on any other branch. CI passes --branch and --commit (or DASHBOARD_BRANCH and DASHBOARD_COMMIT), since -its checkout has no branch. +its checkout has no branch. A local run of a private platform does not need it: +DASHBOARD_API_KEY alone lets the default pipelines resolve it. Exit status: 1 when the dashboard reports a failed rule for any checked run, or when a metadata.json is missing, malformed, or holds no numeric metric. @@ -713,7 +714,8 @@ def parse_args(argv): default=os.environ.get("DASHBOARD_JOB_NAME") or None, help="Check as a build of this CI pipeline, with its rules for the " "branch. Default: the dashboard searches its default baseline " - "pipelines and takes the first with a build of the design. " + "pipelines and takes the first with a build of the design, private " + "ones included with DASHBOARD_API_KEY. Meant for CI. " "Env: DASHBOARD_JOB_NAME.", ) parser.add_argument( From 1ef62438a7ac378a2b96ca07776401eb588488de Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:32:29 -0300 Subject: [PATCH 7/8] Require a full SHA for --base-commit The dashboard matches the pinned commit exactly, so a short or uppercase --base-commit found no build and reported no baseline: an inconclusive run that exits 0, which quietly turns the gate off. It is now checked like --commit, lowercased and rejected with exit 1 unless it is a full SHA, and DASHBOARD_BASE_COMMIT with it. Signed-off-by: Miguel Dalberto Pedro --- flow/test/test_checkQorMetrics.py | 16 ++++++++++++++++ flow/util/checkQorMetrics.py | 31 ++++++++++++++++--------------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py index 31e03ca6862..f35f3c0595c 100644 --- a/flow/test/test_checkQorMetrics.py +++ b/flow/test/test_checkQorMetrics.py @@ -198,6 +198,22 @@ def test_short_commit_is_rejected_before_any_request(self): self.assertEqual(payloads, []) self.assertIn("is not a full 40-character SHA", output) + def test_short_base_commit_is_rejected_before_any_request(self): + code, output, payloads = self.run_check("--base-commit", "abc1234") + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + self.assertIn("--base-commit 'abc1234' is not a full 40-character SHA", output) + + def test_short_base_commit_from_the_environment_is_rejected(self): + os.environ["DASHBOARD_BASE_COMMIT"] = "abc1234" + code, _, payloads = self.run_check() + self.assertEqual(code, 1) + self.assertEqual(payloads, []) + + def test_base_commit_is_sent_lowercased(self): + _, _, payloads = self.run_check("--base-commit", f" {BASE.upper()} ") + self.assertEqual(payloads[0]["BaseCommitSha"], BASE) + class BranchTest(CheckQorMetricsTest): def test_branch_comes_from_git(self): diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 46935cd1db3..d9a4dd3417a 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -822,21 +822,22 @@ def run_check(argv=None): api_url = args.api_url or (BETA_API_URL if args.beta else API_URL) endpoint = api_url.rstrip("/") + CHECK_PATH api_key = os.environ.get("DASHBOARD_API_KEY", "") - # The dashboard matches the commit exactly and silently falls back to the - # latest master build on a miss, so anything but a full SHA is an error. - commit = args.commit.strip().lower() - if commit and not SHA_RE.match(commit): - line = ( - f"[ERROR] --commit {args.commit.strip()!r} is not a full 40-character SHA." - ) - print(line) - if not args.metadata and os.path.isdir(REPORTS_FOLDER): - write_log([line]) - return 1 + # The dashboard matches both commits exactly. A missed --commit falls back to + # the latest master build and a missed --base-commit to no baseline, both + # silently, so anything but a full SHA is an error. + commit = (args.commit or "").strip().lower() + base_commit = (args.base_commit or "").strip().lower() + for flag, value in (("--commit", commit), ("--base-commit", base_commit)): + if value and not SHA_RE.match(value): + line = f"[ERROR] {flag} {value!r} is not a full 40-character SHA." + print(line) + if not args.metadata and os.path.isdir(REPORTS_FOLDER): + write_log([line]) + return 1 # A pipeline run follows its CI branch rules, and CI checkouts often fetch # no remote branches, so every commit there would look unpushed. merge_base_ref = None - if not (args.base_commit or args.job_name) and args.merge_base.lower() != "none": + if not (base_commit or args.job_name) and args.merge_base.lower() != "none": merge_base_ref = args.merge_base # Only a pipeline run uses the branch; a checkout is resolved from the commit. branch = "" @@ -868,8 +869,8 @@ def run_check(argv=None): "[INFO] No DASHBOARD_API_KEY set — checking public data only. " "Private platforms will report as inconclusive, not as an auth error." ) - if args.base_commit: - header.append(f"[INFO] Baseline pinned to commit {args.base_commit}") + if base_commit: + header.append(f"[INFO] Baseline pinned to commit {base_commit}") elif args.job_name and branch: header.append( f"[INFO] Checking as a build of {args.job_name}, branch {branch}" @@ -906,7 +907,7 @@ def run_check(argv=None): run, endpoint, api_key, - args.base_commit, + base_commit, args.job_name, args.only_prefix, commit, From 51c7b16ca471cbbec52e5f50d09606c8faa01495 Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Wed, 21 Oct 2026 12:32:29 -0300 Subject: [PATCH 8/8] Warn on a detached HEAD and on an ignored --branch With --job-name, a detached HEAD is resolved as a feature branch, just as an empty branch is, but only the empty branch warned. Both warn now. A --branch (or DASHBOARD_BRANCH) without --job-name is ignored by design, and the report now says so instead of dropping it silently. Also shortens an overlong docstring line. Signed-off-by: Miguel Dalberto Pedro --- flow/test/test_checkQorMetrics.py | 8 ++++++-- flow/util/checkQorMetrics.py | 12 +++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py index f35f3c0595c..4363c5a8f2f 100644 --- a/flow/test/test_checkQorMetrics.py +++ b/flow/test/test_checkQorMetrics.py @@ -149,8 +149,9 @@ def test_checkout_sends_only_the_commit(self): self.assertEqual(payload["Metrics"], {"finish__timing__setup__ws": 1.5}) def test_branch_is_dropped_without_a_job_name(self): - _, _, payloads = self.run_check("--branch", "feature") + _, output, payloads = self.run_check("--branch", "feature") self.assertEqual(payloads[0]["BranchName"], "") + self.assertIn("--branch (DASHBOARD_BRANCH) is ignored", output) def test_job_name_sends_the_branch(self): _, _, payloads = self.run_check("--job-name", "orfs-ci", "--branch", "feature") @@ -233,8 +234,11 @@ def test_detached_head_reads_as_head(self): repository, master, _ = self.repository() git(repository, "checkout", "-q", "--detach", master) self.enter(repository) - _, _, payloads = self.run_check("--job-name", "orfs-ci") + _, output, payloads = self.run_check("--job-name", "orfs-ci") self.assertEqual(payloads[0]["BranchName"], "HEAD") + self.assertIn( + "[WARN] Checking as a build of orfs-ci with a detached HEAD", output + ) def test_no_branch_outside_a_repository_warns(self): _, output, payloads = self.run_check("--job-name", "orfs-ci") diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index d9a4dd3417a..29cd38f08b9 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -169,7 +169,7 @@ class MetadataError(Exception): def load_metadata(path, only_prefix=None): - """Returns (metrics, commit_sha, dropped). Raises MetadataError if the file is unusable. + """Returns (metrics, commit_sha, dropped), or raises MetadataError. dropped names the metrics left out because their value is not finite. @@ -871,6 +871,11 @@ def run_check(argv=None): ) if base_commit: header.append(f"[INFO] Baseline pinned to commit {base_commit}") + elif args.job_name and branch == "HEAD": + header.append( + f"[WARN] Checking as a build of {args.job_name} with a detached HEAD " + "(no --branch): resolved as a feature branch." + ) elif args.job_name and branch: header.append( f"[INFO] Checking as a build of {args.job_name}, branch {branch}" @@ -891,6 +896,11 @@ def run_check(argv=None): f"[INFO] A commit on no remote branch is pinned to its merge base with " f"{merge_base_ref} (DASHBOARD_MERGE_BASE=none turns this off)" ) + if args.branch and not args.job_name: + header.append( + "[WARN] --branch (DASHBOARD_BRANCH) is ignored without --job-name: " + "a checkout is resolved from its commit." + ) if args.only_prefix: prefixes = " ".join(args.only_prefix) header.append(f"[INFO] Sending only metrics prefixed {prefixes}")