Add checkQorMetrics.py inline check script - #4458
Open
migueldalberto wants to merge 1 commit into
Open
Conversation
Sweeps flow/reports for metadata.json files, POSTs metrics to the QoR
dashboard's /compare/check/inline endpoint, and reports which rules a
local change would fail against the latest baseline build. Always
exits 0 since it reports rather than gates.
Signed-off-by: Miguel Dalberto Pedro <miguel.pedro@precisioninno.com>
Send JobName so the local baseline resolves to the right pipeline
Empty PipelineId/BranchName don't leave the baseline "unscoped" as
assumed -- the dashboard's PipelineClassification.Classify("") falls
through to null just like Classify(null), and
ResolveLocalBaselineAsync then filters on PipelineName == null,
matching whatever stale/legacy master build has no pipeline recorded
instead of the real latest master build.
JobName (the canonical Jenkins job-folder name, e.g.
jenkins-ci's utilGetJobFolderName()) takes priority over that
classification and scopes the baseline to the actual pipeline.
Default it to OpenROAD-flow-scripts-Public, the pipeline that
uploads master's dashboard metrics, and let --job-name/
$DASHBOARD_JOB_NAME override it for secure/private branches.
Verified end to end: re-running against the same real Jenkins
artifact that previously reported a bogus FAIL against baseline
build 4 now resolves to the real latest master build and reports
PASS with 0% deltas, matching the actual CI result.
Signed-off-by: Miguel Dalberto Pedro <miguel.pedro@precisioninno.com>
Fix the usage docs and narrow the command-line surface
The docstring claimed a plain `make` leaves a metadata.json behind. It
does not: the default goal stops at `finish`, and only `make metadata`
runs genMetrics.py, so a developer following it found nothing to check.
Drop --reports and --log. The sibling report scripts (genReport.py,
genReportTable.py, uploadMetadata.py) all chdir to flow/ and walk a
literal "reports"; making the root configurable here only let the sweep
diverge from what ingestion sees, and needed an INVOCATION_DIR dance to
keep relative paths meaning what the user typed. The log path is now
fixed and reported as an absolute path -- it was printed relative to
flow/, naming a path that does not exist from where the script was run.
Drop --target/--api-url for a --beta flag over two fixed https URLs. An
arbitrary --api-url could send DASHBOARD_API_KEY over a plaintext hop;
with both URLs baked in, that is structurally impossible rather than
guarded at runtime.
Drop the -v alias: genReport.py reads it as --verbose and genMetrics.py
binds it to the variant, so either meaning surprises half the callers.
Report a run directory with no metadata.json from the caller instead of
from the walk, so the warning respects --platform/--design/--variant and
reaches the log file. A scoped run no longer warns about designs it was
never asked to check.
Retry transient request failures three times with linear backoff. One
blip previously cost a design its entire check across a 61-design sweep.
Non-transient HTTP statuses return at once, since re-sending the same
payload only earns the same answer.
Signed-off-by: Miguel Dalberto Pedro <miguel.pedro@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, checkQorMetrics.py, designed to validate local flow runs against QoR dashboard rules by posting numeric metrics to an API. The feedback focuses on improving the script's robustness and portability. Key recommendations include wrapping check_run and fmt_value in try-except blocks to handle unexpected API responses or non-numeric values gracefully, and explicitly specifying encoding='utf-8' when reading and writing files to prevent platform-dependent encoding issues.
Comment on lines
+204
to
+231
| metrics, commit = load_metadata(metadata_path) | ||
| if not metrics: | ||
| result["error"] = f"no numeric metrics in {metadata_path}" | ||
| return result | ||
| result["metric_count"] = len(metrics) | ||
|
|
||
| payload = { | ||
| "CommitSha": commit, | ||
| "BranchName": "", | ||
| "PipelineId": "", | ||
| "JobName": job_name, | ||
| "PlatformName": platform, | ||
| "DesignName": design, | ||
| "VariantName": variant, | ||
| "Metrics": metrics, | ||
| "IsLocal": True, | ||
| } | ||
| if base_commit: | ||
| payload["BaseCommitSha"] = base_commit | ||
|
|
||
| response, error = post_check(endpoint, api_key, payload) | ||
| if error is not None: | ||
| result["error"] = error | ||
| return result | ||
|
|
||
| result["response"] = response | ||
| result["status"] = response.get("status", "unknown") | ||
| return result |
Contributor
There was a problem hiding this comment.
To ensure the script's guarantee of always exiting with status 0, wrap the core execution of check_run in a try-except block. Additionally, validate that the API response is a dictionary before accessing .get() to prevent AttributeError crashes if the server returns an unexpected payload format (e.g., a list or string).
try:
metrics, commit = load_metadata(metadata_path)
if not metrics:
result[\"error\"] = f\"no numeric metrics in {metadata_path}\"
return result
result[\"metric_count\"] = len(metrics)
payload = {
\"CommitSha\": commit,
\"BranchName\": \"\",
\"PipelineId\": \"\",
\"JobName\": job_name,
\"PlatformName\": platform,
\"DesignName\": design,
\"VariantName\": variant,
\"Metrics\": metrics,
\"IsLocal\": True,
}
if base_commit:
payload[\"BaseCommitSha\"] = base_commit
response, error = post_check(endpoint, api_key, payload)
if error is not None:
result[\"error\"] = error
return result
if not isinstance(response, dict):
result[\"error\"] = f\"unexpected response type: {type(response).__name__}\"
return result
result[\"response\"] = response
result[\"status\"] = response.get(\\
Comment on lines
+76
to
+83
| def fmt_value(value): | ||
| """Render a metric value without inventing precision it does not have.""" | ||
| if value is None: | ||
| return "n/a" | ||
| number = float(value) | ||
| if number == int(number) and abs(number) < 1e15: | ||
| return str(int(number)) | ||
| return f"{number:.4f}" |
Contributor
There was a problem hiding this comment.
To prevent potential runtime crashes if the dashboard returns non-numeric values (or unexpected types), wrap the float conversion in a try-except block to handle ValueError or TypeError gracefully.
def fmt_value(value):
\"\"\"Render a metric value without inventing precision it does not have.\"\"\"
if value is None:
return \"n/a\"
try:
number = float(value)
if number == int(number) and abs(number) < 1e15:
return str(int(number))
return f\"{number:.4f}\"
except (ValueError, TypeError):
return str(value)
Comment on lines
+123
to
+124
| with open(path) as metadata_file: | ||
| raw = json.load(metadata_file) |
Contributor
There was a problem hiding this comment.
Specify encoding='utf-8' when opening files to ensure portability and avoid platform-dependent encoding issues (e.g., on Windows or environments with non-UTF-8 defaults).
with open(path, encoding=\"utf-8\") as metadata_file:
raw = json.load(metadata_file)
Comment on lines
+482
to
+484
| os.makedirs(os.path.dirname(log_path), exist_ok=True) | ||
| with open(log_path, "w") as log_file: | ||
| log_file.write("\n".join(lines) + "\n") |
Contributor
There was a problem hiding this comment.
Specify encoding='utf-8' when writing the log file to ensure consistent UTF-8 output across all execution environments.
os.makedirs(os.path.dirname(log_path), exist_ok=True)
with open(log_path, \"w\", encoding=\"utf-8\") as log_file:
log_file.write(\"\\n\".join(lines) + \"\\n\")
Member
🔍 QoR checkMetrics reflect the PR merge build — i.e. what will land on the target branch. Advisory — results are log-only and do not affect build status. The authoritative QoR gate remains the local rules-file check. Commit 62 design(s) checked — 0 with regression(s), 0 without a comparable baseline.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flow/util/checkQorMetrics.py, a local inline check: sweepsreports/<platform>/<design>/<variant>/metadata.json, POSTs each run's numeric metrics to the QoR dashboard's/compare/check/inlineendpoint, and reports which rules would fail against the latest baseline build (or a pinned--base-commit).checkMetadata.py: compares against a real baseline build + the dashboard's liverule_configsthresholds instead of the committedrules-<variant>.jsongoldens, so metric drift that moves with master doesn't false-positive.