PR report publish before comment

.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/**')])])
       }
     }
   }

README.md

@@ -462,13 +462,23 @@ 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
 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

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: <historypath>/<branch>/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()
 
@@ -39,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:
@@ -50,6 +53,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 = "<!-- openmodelica-library-testing: pull request report -->"
+
+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()
@@ -142,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 ""))
@@ -168,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,
@@ -220,54 +298,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 = "<!-- openmodelica-library-testing: pull request report -->"
-
-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 = []
@@ -321,6 +351,24 @@ def postComment(number, body):
         "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("<tr>%s</tr>" % "".join("<td>%s</td>" % 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)
@@ -340,6 +388,8 @@ def postComment(number, body):
   ("#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"])),
@@ -382,6 +432,10 @@ def postComment(number, body):
             "%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 += ["<details><summary>%d models affected</summary>" % len(markdownrows), "",
@@ -391,7 +445,6 @@ def postComment(number, body):
 markdown += ["<details><summary>Caveats</summary>", ""]
 markdown += ["- %s" % c.replace("&rarr;", "->") for c in caveats + [note]]
 markdown += ["", "</details>", "", "---", "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)

pr.html.tpl

@@ -42,6 +42,15 @@ the pull request was branched.</p>
 <tr><td>Models only in the baseline run</td><td>#NUMONLYBASELINE#</td></tr>
 </table>
 
+<h2>Time</h2>
+
+<p>#TOTALNOTE#</p>
+
+<table>
+<tr><th>Phase</th><th>#BASELINE#</th><th>#BRANCH#</th><th>Change</th><th>Change, same phase</th></tr>
+#TOTALS#
+</table>
+
 <h2>Library Changes</h2>
 <table>
 <tr><th>Library</th><th>Change</th></tr>