diff --git a/docs/contrib/Metrics.md b/docs/contrib/Metrics.md index 5bef0658d8..8b66f54670 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` @@ -47,12 +48,52 @@ 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: - -- `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`. +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`. 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: + +- `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. 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 + 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`. 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 @@ -62,8 +103,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/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py new file mode 100644 index 0000000000..4363c5a8f2 --- /dev/null +++ b/flow/test/test_checkQorMetrics.py @@ -0,0 +1,542 @@ +#!/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): + _, 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") + 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) + + 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): + 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) + _, 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") + 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_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) + 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_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") + _, _, 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() diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 7146c53e50..29cd38f08b 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -4,9 +4,9 @@ (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 -gate behind `make metadata`. Rule tolerances live in the dashboard, not in +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,24 @@ 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. 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 +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. 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. 0 otherwise. A run the dashboard could not judge -- no baseline, nothing @@ -33,9 +51,12 @@ import argparse import concurrent.futures +import functools import json +import math import os import re +import subprocess import sys import time import urllib.error @@ -65,13 +86,13 @@ 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}$") 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} |" @@ -148,7 +169,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), or raises MetadataError. + + 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 @@ -186,12 +209,88 @@ def load_metadata(path, only_prefix=None): 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 + # 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 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, "")) - 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 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): @@ -234,8 +333,24 @@ 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="", + 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. 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 = { "platform": platform, @@ -245,6 +360,8 @@ def check_run(run, endpoint, api_key, base_commit, job_name, only_prefix=None): "response": None, "error": None, "metric_count": 0, + "dropped": [], + "merge_base": None, } # One run's surprise must not take down the sweep: this is called from a @@ -255,24 +372,41 @@ 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 + + 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 + # 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 @@ -297,6 +431,16 @@ def check_run(run, endpoint, api_key, base_commit, job_name, only_prefix=None): 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']}" @@ -321,6 +465,14 @@ 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." + ) + lines.extend(dropped_lines(result)) return lines totals = response.get("totals", {}) @@ -334,10 +486,34 @@ 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 result["merge_base"]: + lines.append( + 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." + ) + lines.extend(dropped_lines(result)) if result["status"] == "inconclusive": lines.append( @@ -501,15 +677,46 @@ def parse_args(argv): ) parser.add_argument( "--base-commit", - help="Pin the baseline to this commit's build instead of latest master.", + 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", + 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, private " + "ones included with DASHBOARD_API_KEY. Meant for CI. " + "Env: DASHBOARD_JOB_NAME.", ) parser.add_argument( "--metadata", @@ -612,8 +819,30 @@ 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 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 (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: + 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 @@ -640,8 +869,38 @@ 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 == "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}" + + (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 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.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}") @@ -658,9 +917,12 @@ def run_check(argv=None): run, endpoint, api_key, - args.base_commit, + base_commit, args.job_name, args.only_prefix, + commit, + branch, + merge_base_ref, ), runs, ) diff --git a/flow/util/utils.mk b/flow/util/utils.mk index 195db144af..15c20df73d 100644 --- a/flow/util/utils.mk +++ b/flow/util/utils.mk @@ -27,11 +27,16 @@ 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. 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. .PHONY: metadata-check metadata-check: $(PYTHON_EXE) $(UTILS_DIR)/checkQorMetrics.py \