Size reports: Improved comments (#10982)

#### Problem

It's hard to pick out actual size changes, beyond those called out in
the large-increase tables.

Report information can be distributed across multiple collapsed tables,
since the generator often runs before all builds are complete.

#### Change overview

Read back any existing comment and merge in new reports.

Comments now contain:

1. An open table highlighting large increases, if any.
2. A collapsed table of all increases, if any.
3. A collapsed table of all decreases, if any.
4. A collapsed table of all data.

#### Testing

Tested with manual runs:
#10979 (comment)

scripts/tools/memory/gh_report.py

@@ -22,6 +22,7 @@
 import logging
 import os
 import os.path
+import re
 import sqlite3
 import sys
 import zipfile
@@ -101,6 +102,14 @@
             'type': int,
         },
     },
+    'github.limit-pr': {
+        'help': 'Report only on PR, if present.',
+        'metavar': 'PR',
+        'default': 0,
+        'argparse': {
+            'type': int,
+        },
+    },
     Config.group_map('report'): {
         'group': 'output'
     },
@@ -194,8 +203,11 @@ def add_sizes(self, **kwargs):
         bd = {k: kwargs[k] for k in ('hash', 'parent', 'time')}
         cd = {k: kwargs.get(k, 0) for k in ('pr', 'artifact', 'commented')}
         build = self.store_and_return_id('build', thing_id=thing, **bd, **cd)
-        for d in kwargs['sizes']:
-            self.store('size', build_id=build, **d)
+        if build is None:
+            logging.error('Failed to store %s %s %s', thing, bd, cd)
+        else:
+            for d in kwargs['sizes']:
+                self.store('size', build_id=build, **d)
 
     def add_sizes_from_json(self, s: Union[bytes, str], origin: Dict):
         """Add sizes from a JSON size report."""
@@ -331,9 +343,14 @@ def delete_stale_builds(self, build_ids: Iterable[int]):
         self.commit()
 
     def delete_artifact(self, artifact_id: int):
-        if self.gh and artifact_id and artifact_id not in self.deleted_artifacts:
+        if (self.gh and artifact_id
+                and artifact_id not in self.deleted_artifacts):
             self.deleted_artifacts.add(artifact_id)
-            self.gh.actions.delete_artifact(artifact_id)
+            try:
+                self.gh.actions.delete_artifact(artifact_id)
+            except Exception:
+                # During manual testing we sometimes lose the race against CI.
+                logging.error('Failed to delete artifact %d', artifact_id)
 
     def delete_stale_artifacts(self, stale_artifacts: Iterable[int]):
         if not self.config['github.keep']:
@@ -454,11 +471,16 @@ def changes_for_commit(db: SizeDatabase, pr: int, commit: str,
     return df
 
 
-def gh_send_change_report(db: SizeDatabase, df: pd.DataFrame,
-                          tdf: pd.DataFrame) -> bool:
+comment_format_re = re.compile(r"^<!--ghr-comment-format:(\d+)-->")
+
+
+def gh_send_change_report(db: SizeDatabase, df: pd.DataFrame) -> bool:
     """Send a change report as a github comment."""
+
     if not db.gh:
         return False
+
+    # Look for an existing comment for this change.
     pr = df.attrs['pr']
 
     # Check the most recent commit on the PR, so that we don't comment on
@@ -475,61 +497,147 @@ def gh_send_change_report(db: SizeDatabase, df: pd.DataFrame,
     # Check for an existing size report comment. If one exists, we'll add
     # the new report to it.
     title = df.attrs['title']
-    existing_comment_id = 0
+    existing_comment = None
+    existing_comment_format = 0
     for comment in gh_get_comments_for_pr(db.gh, pr):
-        if comment.body.partition('\n')[0] == df.attrs['title']:
-            existing_comment_id = comment.id
-            title = comment.body
+        comment_parts = comment.body.partition('\n')
+        if comment_parts[0].strip() == title:
+            existing_comment = comment
+            if m := comment_format_re.match(comment_parts[2]):
+                existing_comment_format = int(m.group(1))
             break
 
-    md = io.StringIO()
-    md.write(title)
-    md.write('\n')
-
-    if tdf is not None and not tdf.empty:
-        md.write(f'\n**{tdf.attrs["title"]}:**\n\n')
-        memdf.report.write_df(db.config,
-                              tdf,
-                              md,
-                              'pipe',
-                              hierify=True,
-                              title=False,
-                              tabulate={'floatfmt': '5.1f'})
-
-    count = len(df.attrs['things'])
-    platforms = ', '.join(sorted(list(set(df['platform']))))
-    summary = f'{count} build{"" if count == 1 else "s"} (for {platforms})'
-    md.write(f'\n<details>\n<summary>{summary}</summary>\n\n')
-    memdf.report.write_df(db.config,
-                          df,
-                          md,
-                          'pipe',
-                          hierify=True,
-                          title=False,
-                          tabulate={'floatfmt': '5.1f'})
-    md.write('\n</details>\n')
-    text = md.getvalue()
-    md.close()
+    if existing_comment_format != 1:
+        existing_comment = None
+    text = gh_comment_v1(db, df, existing_comment)
 
-    if existing_comment_id:
-        comment = f'updating comment {existing_comment_id}'
-    else:
-        comment = 'as new comment'
-    logging.info('SCR: %s for %s %s', df.attrs['title'], summary, comment)
+    logging.info(
+        'SCR: %s %s', df.attrs['title'],
+        f'updating comment {existing_comment.id}'
+        if existing_comment else 'as new comment')
 
     if db.config['github.dryrun-comment']:
+        logging.debug('%s', text)
         return False
 
     try:
-        if existing_comment_id:
-            db.gh.issues.update_comment(existing_comment_id, text)
+        if existing_comment:
+            db.gh.issues.update_comment(existing_comment.id, text)
         else:
             db.gh.issues.create_comment(pr, text)
         return True
     except Exception:
         return False
 
 
+def gh_comment_v1(db: SizeDatabase, df: pd.DataFrame, existing_comment) -> str:
+    """Format a github comment."""
+
+    if existing_comment:
+        df = v1_comment_merge(df, existing_comment)
+
+    threshold_df = None
+    increase_df = df[df['change'] > 0]
+    if increase_df.empty:
+        increase_df = None
+    elif threshold := db.config['report.increases']:
+        threshold_df = df[df['% change'] > threshold]
+        if threshold_df.empty:
+            threshold_df = None
+    decrease_df = df[df['change'] < 0]
+    if decrease_df.empty:
+        decrease_df = None
+
+    with io.StringIO() as md:
+        md.write(df.attrs['title'])
+        md.write('\n<!--ghr-comment-format:1-->\n\n')
+
+        if threshold_df is not None:
+            md.write(f'**Increases above {threshold:.2g}%:**\n\n')
+            md.write('<!--ghr-report:threshold-->\n\n')
+            v1_comment_write_df(db, threshold_df, md)
+
+        if increase_df is not None:
+            summary = v1_comment_summary(increase_df)
+            md.write('<details>\n')
+            md.write(f'<summary>Increases ({summary})</summary>\n')
+            md.write('<!--ghr-report:increases-->\n\n')
+            v1_comment_write_df(db, increase_df, md)
+            md.write('</details>\n\n')
+
+        if decrease_df is not None:
+            summary = v1_comment_summary(decrease_df)
+            md.write('<details>\n')
+            md.write(f'<summary>Decreases ({summary})</summary>\n')
+            md.write('<!--ghr-report:decreases-->\n\n')
+            v1_comment_write_df(db, decrease_df, md)
+            md.write('</details>\n\n')
+
+        summary = v1_comment_summary(df)
+        md.write('<details>\n')
+        md.write(f'<summary>Full report ({summary})</summary>\n')
+        md.write('<!--ghr-report:full-->\n\n')
+        v1_comment_write_df(db, df, md)
+        md.write('\n</details>\n')
+
+        return md.getvalue()
+
+
+def v1_comment_merge(df: pd.DataFrame, comment) -> pd.DataFrame:
+    with io.StringIO(comment.body) as body:
+        for line in body:
+            if line.startswith('<!--ghr-report:full-->'):
+                body.readline()  # Blank line before table.
+                header, rows = read_hierified(body)
+                break
+    logging.debug('REC: read %d rows', len(rows))
+    df = df.append(pd.DataFrame(data=rows, columns=header).astype(df.dtypes))
+    return df.sort_values(
+        by=['platform', 'target', 'config', 'section']).drop_duplicates()
+
+
+def read_hierified(f):
+    """Read a markdown table in ‘hierified’ format."""
+
+    line = f.readline()
+    header = tuple((s.strip() for s in line.split('|')[1:-1]))
+
+    _ = f.readline()  # The line under the header.
+
+    rows = []
+    for line in f:
+        line = line.strip()
+        if not line:
+            break
+        row = []
+        columns = line.split('|')
+        for i in range(0, len(header)):
+            column = columns[i + 1].strip()
+            if not column:
+                column = rows[-1][i]
+            row.append(column)
+        rows.append(tuple(row))
+
+    return (header, rows)
+
+
+def v1_comment_write_df(db: SizeDatabase, df: pd.DataFrame,
+                        out: memdf.report.OutputOption):
+    memdf.report.write_df(db.config,
+                          df,
+                          out,
+                          'pipe',
+                          hierify=True,
+                          title=False,
+                          tabulate={'floatfmt': '5.1f'})
+
+
+def v1_comment_summary(df: pd.DataFrame) -> str:
+    count = df[['platform', 'target', 'config']].drop_duplicates().shape[0]
+    platforms = ', '.join(sorted(list(set(df['platform']))))
+    return f'{count} build{"" if count == 1 else "s"} for {platforms}'
+
+
 def report_matching_commits(db: SizeDatabase) -> Dict[str, pd.DataFrame]:
     """Report on all new comparable commits."""
     if not (db.config['report.pr'] or db.config['report.push']):
@@ -542,30 +650,24 @@ def report_matching_commits(db: SizeDatabase) -> Dict[str, pd.DataFrame]:
 
     report_push = db.config['report.push']
     report_pr = db.config['report.pr']
+    only_pr = db.config['github.limit-pr']
 
     dfs = {}
     for pr, commit, parent in db.select_matching_commits().fetchall():
         if not (report_pr if pr else report_push):
             continue
 
+        # Github doesn't have a way to fetch artifacts associated with a
+        # particular PR. For testing purposes, filter to a single PR here.
+        if only_pr and pr != only_pr:
+            continue
+
         df = changes_for_commit(db, pr, commit, parent)
         dfs[df.attrs['name']] = df
 
-        if threshold := db.config['report.increases']:
-            tdf = df[df['% change'] > threshold]
-        else:
-            tdf = None
-        if tdf is not None and not tdf.empty:
-            commit = df.attrs['commit']
-            parent = df.attrs['parent']
-            tdf.attrs['name'] = f'L,{commit},{parent}'
-            tdf.attrs['title'] = (
-                f'Increases above {threshold:.1f}% from {parent} to {commit}')
-            dfs[tdf.attrs['name']] = tdf
-
         if (pr and comment_enabled
                 and (comment_limit == 0 or comment_limit > comment_count)):
-            if gh_send_change_report(db, df, tdf):
+            if gh_send_change_report(db, df):
                 # Mark the originating builds, and remove the originating
                 # artifacts, so that they don't generate duplicate report
                 # comments.

scripts/tools/memory/memdf/util/sqlite.py

@@ -97,14 +97,18 @@ def store(self, table: str, **kwargs):
         self.connection().execute(q, v)
 
     def get_matching(self, table: str, columns: List[str], **kwargs):
-        return self.connection().execute(
-            f"SELECT {','.join(columns)} FROM {table}"
-            f"  WHERE {'=? AND '.join(kwargs.keys())}=?",
-            list(kwargs.values()))
+        q = (f"SELECT {','.join(columns)} FROM {table}"
+             f"  WHERE {'=? AND '.join(kwargs.keys())}=?")
+        v = list(kwargs.values())
+        return self.connection().execute(q, v)
 
     def get_matching_id(self, table: str, **kwargs):
-        return self.get_matching(table, ['id'], **kwargs).fetchone()[0]
+        cur = self.get_matching(table, ['id'], **kwargs)
+        row = cur.fetchone()
+        if row:
+            return row[0]
+        return None
 
-    def store_and_return_id(self, table: str, **kwargs) -> int:
+    def store_and_return_id(self, table: str, **kwargs) -> Optional[int]:
         self.store(table, **kwargs)
         return self.get_matching_id(table, **kwargs)