From d30a275dc4cd3b533ad6e29dd79e8bdf473025ed Mon Sep 17 00:00:00 2001 From: Miguel Dalberto Pedro Date: Thu, 12 Nov 2026 11:59:59 -0300 Subject: [PATCH] Show the per-metric comparison table by default in checkQorMetrics.py The QoR check report now always lists every metric a rule evaluated, with a new Status column (PASS/FAIL), instead of only the failing ones. The new --quiet (-q) flag leaves the table out; failed rules, warnings and the verdict are still reported. Ruled metrics that are absent from the baseline build are now always named, one per line, rather than only counted. --verbose, which used to gate both, is removed. The failing-rule test fixture now uses the field names the dashboard sends (baseValue, targetValue, thresholdValue). Signed-off-by: Miguel Dalberto Pedro --- docs/contrib/Metrics.md | 2 +- flow/test/test_checkQorMetrics.py | 87 +++++++++++++++++++++++++++++-- flow/util/checkQorMetrics.py | 42 ++++++++------- 3 files changed, 107 insertions(+), 24 deletions(-) diff --git a/docs/contrib/Metrics.md b/docs/contrib/Metrics.md index 8b66f54670..b7b035080f 100644 --- a/docs/contrib/Metrics.md +++ b/docs/contrib/Metrics.md @@ -109,7 +109,7 @@ To check every run under `reports/` at once, call the script directly: cd OpenROAD-flow-scripts/flow ./util/checkQorMetrics.py # sweep, log in reports/inline-check.log ./util/checkQorMetrics.py --base-commit # pin the baseline build -./util/checkQorMetrics.py --verbose # show passing rules and absent metrics +./util/checkQorMetrics.py --quiet # leave out the per-metric table ``` If the check reports a failed rule, review it to make sure the change in diff --git a/flow/test/test_checkQorMetrics.py b/flow/test/test_checkQorMetrics.py index 4363c5a8f2..107fa95b20 100644 --- a/flow/test/test_checkQorMetrics.py +++ b/flow/test/test_checkQorMetrics.py @@ -411,9 +411,9 @@ def test_fail_lists_the_failed_rule(self): failed = { "metricName": "finish__timing__setup__ws", "status": "fail", - "baselineValue": 2, - "currentValue": 1.5, - "limit": 1.9, + "baseValue": 2, + "targetValue": 1.5, + "thresholdValue": 1.9, "deltaPct": -25, } code, output, _ = self.run_check( @@ -424,6 +424,87 @@ def test_fail_lists_the_failed_rule(self): self.assertIn("-25.00", output) self.assertIn("QoR check: FAIL", output) + def table_reply(self): + passed = { + "metricName": "synth__design__instance__area", + "status": "pass", + "baseValue": 100, + "targetValue": 101, + "thresholdValue": 110, + "deltaPct": 1, + } + failed = { + "metricName": "finish__timing__setup__ws", + "status": "fail", + "baseValue": 2, + "targetValue": 1.5, + "thresholdValue": 1.9, + "deltaPct": -25, + } + absent = [ + {"metricName": name, "status": "missing_from_baseline"} + for name in ("route__wirelength", "cts__clock__skew") + ] + return Dashboard( + response(status="fail", failed=1, metrics=[passed, failed, *absent]) + ) + + def table_rows(self, output): + return [line for line in output.splitlines() if line.startswith("| ")] + + def test_table_shows_every_evaluated_metric_with_its_status(self): + _, output, _ = self.run_check(dashboard=self.table_reply()) + rows = self.table_rows(output) + self.assertEqual(len(rows), 4) + self.assertTrue(rows[0].rstrip("| ").endswith("Status")) + self.assertIn("finish__timing__setup__ws", rows[2]) + self.assertTrue(rows[2].rstrip().endswith("| FAIL |")) + self.assertIn("synth__design__instance__area", rows[3]) + self.assertTrue(rows[3].rstrip().endswith("| PASS |")) + self.assertEqual(len({len(row) for row in rows}), 1) + + def test_passing_table_has_no_error_line(self): + passed = { + "metricName": "synth__design__instance__area", + "status": "pass", + "baseValue": 100, + "targetValue": 101, + "thresholdValue": 110, + "deltaPct": 1, + } + code, output, _ = self.run_check( + dashboard=Dashboard(response(metrics=[passed])) + ) + self.assertEqual(code, 0) + self.assertEqual(len(self.table_rows(output)), 3) + self.assertFalse( + [line for line in output.splitlines() if line.startswith("[ERROR]")] + ) + + def test_absent_metrics_are_named_not_tabled(self): + _, output, _ = self.run_check(dashboard=self.table_reply()) + self.assertIn( + "[WARN] 2 ruled metric(s) absent from the baseline build, not evaluated:\n" + " cts__clock__skew\n" + " route__wirelength\n", + output, + ) + for row in self.table_rows(output): + self.assertNotIn("route__wirelength", row) + + def test_quiet_leaves_out_only_the_table(self): + for flag in ("--quiet", "-q"): + code, output, _ = self.run_check(flag, dashboard=self.table_reply()) + self.assertEqual(code, 1) + self.assertEqual(self.table_rows(output), []) + self.assertIn("QoR regression detected", output) + self.assertIn(" cts__clock__skew", output) + self.assertIn("QoR check: FAIL", output) + + def test_verbose_is_rejected(self): + with self.assertRaises(SystemExit), contextlib.redirect_stderr(io.StringIO()): + self.run_check("--verbose") + def test_latest_master_warns_with_a_commit(self): _, output, _ = self.run_check( dashboard=Dashboard(response(strategy="latest-master")) diff --git a/flow/util/checkQorMetrics.py b/flow/util/checkQorMetrics.py index 8a0f4a85f4..1d11e8cdf5 100755 --- a/flow/util/checkQorMetrics.py +++ b/flow/util/checkQorMetrics.py @@ -39,6 +39,10 @@ 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. +Each run's report has a table of every metric a rule evaluated, with its +baseline, current value, limit, change and PASS/FAIL status (--quiet leaves +the table out), and names each ruled metric absent from the baseline build. + 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 @@ -95,7 +99,7 @@ # NAME_W must match the first field width in ROW_FMT. NAME_W = 46 -ROW_FMT = "| {:46} | {:>12} | {:>12} | {:>12} | {:>7} |" +ROW_FMT = "| {:46} | {:>12} | {:>12} | {:>12} | {:>7} | {:6} |" BANNER = "-" * 79 @@ -446,7 +450,7 @@ def dropped_lines(result): ] -def render_run(result, verbose): +def render_run(result, quiet): """Builds the report lines for one run.""" identity = f"{result['platform']}/{result['design']}/{result['variant']}" lines = [BANNER] @@ -527,13 +531,14 @@ def render_run(result, verbose): ) metrics = response.get("metrics", []) - shown = metrics if verbose else [m for m in metrics if m.get("status") == "fail"] - shown = [m for m in shown if m.get("status") != "missing_from_baseline"] + shown = [m for m in metrics if m.get("status") != "missing_from_baseline"] - if shown: - lines.append(ROW_FMT.format("Metric", "Baseline", "Current", "Limit", "Delta%")) + if shown and not quiet: + lines.append( + ROW_FMT.format("Metric", "Baseline", "Current", "Limit", "Delta%", "Status") + ) lines.append( - ROW_FMT.format("-" * NAME_W, "-" * 12, "-" * 12, "-" * 12, "-" * 7) + ROW_FMT.format("-" * NAME_W, "-" * 12, "-" * 12, "-" * 12, "-" * 7, "-" * 6) ) for metric in sorted(shown, key=lambda m: m.get("metricName", "")): delta = metric.get("deltaPct") @@ -544,6 +549,7 @@ def render_run(result, verbose): fmt_value(metric.get("targetValue")), fmt_value(metric.get("thresholdValue")), fmt_delta(delta), + str(metric.get("status", "?")).upper(), ) ) @@ -560,22 +566,17 @@ def render_run(result, verbose): f"limit {fmt_exact_value(metric.get('thresholdValue'))})" ) - # A design can have hundreds of absent metrics, so name them only on request. absent = sorted( metric.get("metricName", "?") for metric in metrics if metric.get("status") == "missing_from_baseline" ) - if absent and verbose: - lines.append( - "[WARN] Rules not evaluated, metric absent from the baseline build: " - + ", ".join(absent) - ) - elif absent: + if absent: lines.append( - f"[WARN] Rules not evaluated for {len(absent)} metric(s) absent from " - "the baseline build. Use --verbose to list them." + f"[WARN] {len(absent)} ruled metric(s) absent from the baseline " + "build, not evaluated:" ) + lines.extend(f" {name}" for name in absent) # A rule that fell back reports a different threshold than the one it was # configured with, so surface it. thresholdPct is deliberately not shown in @@ -779,10 +780,11 @@ def parse_args(argv): help="Concurrent requests. Default: 8.", ) parser.add_argument( - "--verbose", + "--quiet", + "-q", action="store_true", - help="Report every rule-checked metric, not only the failing ones, " - "and name the metrics absent from the baseline build.", + help="Leave out the per-metric comparison table. Failed rules, warnings " + "and the verdict are still reported.", ) args = parser.parse_args(argv) if args.metadata and not (args.platform and args.design): @@ -948,7 +950,7 @@ def run_check(argv=None): body = [] for result in results: - body.extend(render_run(result, args.verbose)) + body.extend(render_run(result, args.quiet)) body.extend(summarize(results)) for line in body: