From e3b1631561f6885f56131fe69e4385a7a6985162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Sj=C3=B6lund?= Date: Thu, 17 Sep 2026 07:35:47 +0200 Subject: [PATCH 1/2] Publish the pull request report before commenting The report of pull request 16696 was never uploaded: the directory `branches/history/pr/16696` does not exist on the server, while the comment linking to it was posted. The stage printed the summary with `cat history/pr-*/00_comment.md`, a path from before #330 put a pull request under `pr/`. The glob matched nothing, `cat` failed, and the `sshPublisher` step after it never ran - the comment was already posted by then, from inside pr-report.py. The path is `history/pr//00_comment.md` now, and the order is the one the branch reports already use: generate, upload, then announce. Nothing between a report and its upload can stop it any more, and the link in the comment is a report that is there when it is posted. Announcing after the upload means posting a summary that has already been written, which is what `--comment-only` does: it reads the markdown a run left beside its report and posts that, without the database or the report. Assisted-by: Claude Opus 5 (1M context) --- .CI/Jenkinsfile | 17 ++++---- README.md | 5 +++ pr-report.py | 104 +++++++++++++++++++++++++----------------------- 3 files changed, 69 insertions(+), 57 deletions(-) diff --git a/.CI/Jenkinsfile b/.CI/Jenkinsfile index 1394614..1e28f8d 100644 --- a/.CI/Jenkinsfile +++ b/.CI/Jenkinsfile @@ -656,21 +656,22 @@ pipeline { } } sh 'rm -rf history' + sh "./pr-report.py '${pullRequest()}' --baseline='${(params.pull_request_baseline ?: 'master').trim()}'" + // Uploaded before it is announced, as the branch reports are: the + // comment links to the report, and nothing after this may be what stops + // it from being published. + sshPublisher(publishers: [sshPublisherDesc(configName: 'LibraryTestingReports', transfers: [sshTransfer(sourceFiles: 'history/**')])]) + // The summary is in the build log as well, so a run without a token + // still leaves it somewhere to copy from. + sh "cat 'history/pr/${pullRequest()}/00_comment.md'" script { - def report = "./pr-report.py '${pullRequest()}' --baseline='${(params.pull_request_baseline ?: 'master').trim()}'" if (params.pull_request_comment) { // Whoever the token belongs to is who the comment comes from. withCredentials([string(credentialsId: 'OpenModelica-Hudson', variable: 'GITHUB_TOKEN')]) { - sh "${report} --comment" + sh "./pr-report.py '${pullRequest()}' --comment-only" } - } else { - sh report } } - // The summary is in the build log as well, so a run without a token - // still leaves it somewhere to copy from. - sh 'cat history/pr-*/00_comment.md' - sshPublisher(publishers: [sshPublisherDesc(configName: 'LibraryTestingReports', transfers: [sshTransfer(sourceFiles: 'history/**')])]) } } } diff --git a/README.md b/README.md index b4bedc2..6c500e6 100644 --- a/README.md +++ b/README.md @@ -469,6 +469,11 @@ logged in as. In Jenkins it is the `pull_request_comment` parameter, which takes the token from an `OpenModelica-Hudson` credential; without one the report is still written and published, and the summary is in the build log. +`--comment-only` posts the summary a run already wrote, without generating the +report again or touching the database. Jenkins uses it to comment after the +upload rather than before it, so that the link in the comment is a report that +is already there. + Two things make a difference mean something other than "the pull request did this", and the report says so when they apply: **the machine**, since runs on different hardware compare the hardware as much as the change, and **the diff --git a/pr-report.py b/pr-report.py index 8f67635..f1873dd 100755 --- a/pr-report.py +++ b/pr-report.py @@ -26,6 +26,7 @@ parser.add_argument('--githuburl', default="https://github.com/OpenModelica/OpenModelica") parser.add_argument('--markdown', default="", help='where to write the summary to comment on the pull request with (default: //00_comment.md)') parser.add_argument('--comment', action='store_true', help='post that summary on the pull request, replacing the one posted by an earlier run') +parser.add_argument('--comment-only', action='store_true', help='post the summary an earlier run left, without generating the report again') resultsdb.addArgument(parser) args = parser.parse_args() @@ -50,6 +51,60 @@ baseline = shared.resultTable(args.baseline) prurl = "%s/pull/%s" % (args.githuburl, pr) repo = args.githuburl.split("github.com/")[-1].strip("/") +markdownname = args.markdown or os.path.join(args.historypath, branch, "00_comment.md") + +# A run of the same pull request replaces the comment of the one before it +# rather than adding to a pile; this is how it recognises its own. +COMMENTMARKER = "" + +def githubToken(): + """A token to post with: the environment, or whoever gh is logged in as.""" + for var in ["GITHUB_TOKEN", "GH_TOKEN"]: + if os.environ.get(var): + return os.environ[var] + try: + return subprocess.check_output(["gh", "auth", "token"], + stderr=subprocess.DEVNULL).decode("utf-8").strip() + except Exception: + return None + +def github(url, token, data=None, method=None): + request = urllib.request.Request( + url, method=method, + data=json.dumps(data).encode("utf-8") if data is not None else None, + headers={"Accept": "application/vnd.github+json", + "Authorization": "Bearer %s" % token, + "Content-Type": "application/json"}) + return json.loads(urllib.request.urlopen(request).read().decode("utf-8")) + +def postComment(number, body): + """Post the summary on the pull request, or update the one already there.""" + token = githubToken() + if not token: + return ("No token to comment with: set GITHUB_TOKEN, or log in with gh. " + "The comment is in %s." % markdownname) + api = "https://api.github.com/repos/%s/issues" % repo + try: + page = 1 + while True: + comments = github("%s/%s/comments?per_page=100&page=%d" % (api, number, page), token) + for comment in comments: + if COMMENTMARKER in (comment.get("body") or ""): + github(comment["url"], token, {"body": body}, method="PATCH") + return "Updated %s" % comment["html_url"] + if len(comments) < 100: + break + page += 1 + return "Commented on %s" % github("%s/%s/comments" % (api, number), token, + {"body": body})["html_url"] + except urllib.error.HTTPError as e: + raise SystemExit("Could not comment on %s#%s: %s\n%s" + % (repo, number, e, e.read().decode("utf-8", "replace"))) + +if args.comment_only: + with open(markdownname, encoding="utf-8") as fin: + print(postComment(pr, fin.read())) + raise SystemExit(0) db = resultsdb.connect(args.db) cursor = db.cursor() @@ -220,54 +275,6 @@ def classify(group, times): return (colour, " ".join(msgs), "performance improved" if colour == "betterPerformance" else "performance regression") -# A run of the same pull request replaces the comment of the one before it -# rather than adding to a pile; this is how it recognises its own. -COMMENTMARKER = "" - -def githubToken(): - """A token to post with: the environment, or whoever gh is logged in as.""" - for var in ["GITHUB_TOKEN", "GH_TOKEN"]: - if os.environ.get(var): - return os.environ[var] - try: - return subprocess.check_output(["gh", "auth", "token"], - stderr=subprocess.DEVNULL).decode("utf-8").strip() - except Exception: - return None - -def github(url, token, data=None, method=None): - request = urllib.request.Request( - url, method=method, - data=json.dumps(data).encode("utf-8") if data is not None else None, - headers={"Accept": "application/vnd.github+json", - "Authorization": "Bearer %s" % token, - "Content-Type": "application/json"}) - return json.loads(urllib.request.urlopen(request).read().decode("utf-8")) - -def postComment(number, body): - """Post the summary on the pull request, or update the one already there.""" - token = githubToken() - if not token: - return ("No token to comment with: set GITHUB_TOKEN, or log in with gh. " - "The comment is in %s." % markdownname) - api = "https://api.github.com/repos/%s/issues" % repo - try: - page = 1 - while True: - comments = github("%s/%s/comments?per_page=100&page=%d" % (api, number, page), token) - for comment in comments: - if COMMENTMARKER in (comment.get("body") or ""): - github(comment["url"], token, {"body": body}, method="PATCH") - return "Updated %s" % comment["html_url"] - if len(comments) < 100: - break - page += 1 - return "Commented on %s" % github("%s/%s/comments" % (api, number), token, - {"body": body})["html_url"] - except urllib.error.HTTPError as e: - raise SystemExit("Could not comment on %s#%s: %s\n%s" - % (repo, number, e, e.read().decode("utf-8", "replace"))) - counts = {"improved": 0, "regression": 0, "performance improved": 0, "performance regression": 0} rows = [] markdownrows = [] @@ -391,7 +398,6 @@ def postComment(number, body): markdown += ["
Caveats", ""] markdown += ["- %s" % c.replace("→", "->") for c in caveats + [note]] markdown += ["", "
", "", "---", "Generated by the OpenModelica library testing"] -markdownname = args.markdown or os.path.join(historydir, "00_comment.md") comment = "\n".join([COMMENTMARKER] + markdown) + "\n" with codecs.open(markdownname, "w", encoding="utf-8") as fout: fout.write(comment) From b36843ed3a1bed5f2c9a8735716fda054fda1c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Sj=C3=B6lund?= Date: Thu, 17 Sep 2026 07:45:45 +0200 Subject: [PATCH 2/2] Total what the two runs spent per phase The report says which models changed phase and which got slower, and nothing about what the run as a whole cost. Testing pull request 16696 moved 83 of 19842 models and spent 8% more in the backend, which the report had no way of saying. The page and the comment now carry a table of both runs summed per phase over the models they have in common: | Phase | master | pr/16696 | Change | Change, same phase | | -------- | -------- | -------- | ------ | ------------------ | | frontend | 1:44:57 | 1:44:37 | -0.3% | -0.6% | | backend | 3:12:28 | 3:27:54 | +8.0% | +7.6% | The last column is the same sum over only the models that reached the same phase in both runs. A model that now fails in the backend stops paying for the compilation and the simulation it no longer reaches, so without that column a run that broke models reads as a faster one: here the whole simulation looks 1.9% cheaper, while the models that still simulate spend 0.4% more. `exectime`, the whole run of a model, is the total. Assisted-by: Claude Opus 5 (1M context) --- README.md | 5 +++++ pr-report.py | 47 +++++++++++++++++++++++++++++++++++++++++++++++ pr.html.tpl | 9 +++++++++ 3 files changed, 61 insertions(+) diff --git a/README.md b/README.md index 6c500e6..211d4fe 100644 --- a/README.md +++ b/README.md @@ -462,6 +462,11 @@ kind of page as the nightly regression reports, next to `00_comment.md`, a summary to comment on the pull request with. Both are published with the other reports. +Both also total what the two runs spent per phase on the models they have in +common, next to the same totals over only the models that reached the same +phase in both: a model that fails earlier stops paying for the phases it no +longer reaches, which makes those phases look cheaper than they are. + `--comment` posts that summary on the pull request, replacing the one an earlier run posted. It posts as whoever the token belongs to: `GITHUB_TOKEN` or `GH_TOKEN` in the environment, or the account [`gh`](https://cli.github.com) is diff --git a/pr-report.py b/pr-report.py index f1873dd..8080d57 100755 --- a/pr-report.py +++ b/pr-report.py @@ -40,6 +40,8 @@ timeAbs = 10 # Ignore performance regressions for times <10s... PHASES = [(1,"frontend"),(2,"backend"),(3,"simcode"),(4,"templates"),(5,"compile"),(6,"simulate")] +# The columns the totals are summed over; exectime is the whole run of a model. +TIMES = ["frontend","backend","simcode","templates","compile","simulate","verify","exectime"] m = re.match(r"^(?:pr[-/])?([0-9]+)$", args.pullrequest.strip()) if not m: @@ -197,6 +199,25 @@ def changedModels(table1, date1, table2, date2, libnames): return cursor.fetchall() +def phaseTotals(table1, date1, table2, date2, libnames): + """What the two runs spent, per phase, on the models they both have. + + Twice: over every compared model, and over those that reached the same phase + in both runs. A model that now fails earlier stops paying for the phases it + no longer reaches, which makes those phases look cheaper than they are. + """ + inlibs = ",".join("'%s'" % libname for libname in sorted(libnames)) + cols = ["sum(a.%s),sum(b.%s)" % (t, t) for t in TIMES] + cols += ["sum(CASE WHEN a.finalphase=b.finalphase THEN a.%s ELSE 0 END)," + "sum(CASE WHEN a.finalphase=b.finalphase THEN b.%s ELSE 0 END)" % (t, t) + for t in TIMES] + cols += [db.countIf("a.finalphase=b.finalphase")] + query = """SELECT %s FROM %s AS a JOIN %s AS b ON a.libname=b.libname AND a.model=b.model + WHERE a.date=? AND b.date=? AND a.libname IN (%s) AND a.finalphase>=0 AND b.finalphase>=0 + """ % (",".join(cols), db.quote(table1), db.quote(table2), inlibs) + return [v or 0 for v in cursor.execute(query, (date1, date2)).fetchone()] + + prdate = newestRun(branch, args.date) if not prdate: raise SystemExit("No results for %s%s" % (branch, " at or before %d" % args.date if args.date else "")) @@ -223,8 +244,10 @@ def changedModels(table1, date1, table2, date2, libnames): groups.setdefault((d1, d2), []).append(libname) changes = [] +totals = [0] * (4*len(TIMES) + 1) for ((d1, d2), libs) in sorted(groups.items()): changes += changedModels(baseline, d1, branch, d2, libs) + totals = [t + v for (t, v) in zip(totals, phaseTotals(baseline, d1, branch, d2, libs))] changes = sorted(changes, key=lambda x: (x[1], x[0])) # Models one of the runs has and the other does not: a library that grew a model, @@ -328,6 +351,24 @@ def classify(group, times): "difference can also come from something merged into %s since the pull request was " "branched." % (baseline, baseline)) +def relative(before, after): + return "%+.1f%%" % (100.0*(after-before)/before) if before else "" + +numSamePhase = totals[-1] +totalrows = [] +markdowntotals = [] +for (i, name) in enumerate(TIMES): + (t1, t2) = totals[2*i:2*i+2] + (s1, s2) = totals[2*len(TIMES)+2*i:2*len(TIMES)+2*i+2] + cells = ["total" if name == "exectime" else name, + friendlyStr(t1), friendlyStr(t2), relative(t1, t2), relative(s1, s2)] + totalrows.append("%s" % "".join("%s" % c for c in cells)) + markdowntotals.append("| %s |" % " | ".join(cells)) +totalnote = ("Time spent on the %d models both runs have. The last column counts only the %d " + "that reached the same phase in both: a model that fails earlier stops paying " + "for the phases it no longer reaches." + % (numCompared, numSamePhase)) + reportname = "%s..%s.html" % (dateStr(basedate), dateStr(prdate)) historydir = os.path.join(args.historypath, branch) os.makedirs(historydir, exist_ok=True) @@ -347,6 +388,8 @@ def classify(group, times): ("#HOST1#", html.escape(", ".join(sorted(basehosts)))), ("#HOST2#", html.escape(", ".join(sorted(prhosts)))), ("#NUMCOMPARED#", str(numCompared)), + ("#TOTALS#", "\n".join(totalrows)), + ("#TOTALNOTE#", totalnote), ("#NUMIMPROVE#", str(counts["improved"])), ("#NUMREGRESSION#", str(counts["regression"])), ("#NUMPERFIMPROVE#", str(counts["performance improved"])), @@ -389,6 +432,10 @@ def classify(group, times): "%d models compared, **%d improved, %d regressions**, performance %d improved, %d regressions." % (numCompared, counts["improved"], counts["regression"], counts["performance improved"], counts["performance regression"]), + "", + "| Phase | `%s` | `%s` | Change | Change, same phase |" % (baseline, branch), + "| --- | --- | --- | --- | --- |"] + markdowntotals + [ + "", totalnote, "", "[Full report](%s)" % reporturl, ""] if markdownrows: markdown += ["
%d models affected" % len(markdownrows), "", diff --git a/pr.html.tpl b/pr.html.tpl index 34a5670..e64104b 100644 --- a/pr.html.tpl +++ b/pr.html.tpl @@ -42,6 +42,15 @@ the pull request was branched.

Models only in the baseline run#NUMONLYBASELINE# +

Time

+ +

#TOTALNOTE#

+ + + +#TOTALS# +
Phase#BASELINE##BRANCH#ChangeChange, same phase
+

Library Changes

LibraryChange