Merge pull request #5976 from fma-rc/ci/pr-labels

ci: label pull requests that unblock a title or change performance

.github/workflows/labels.yml

@@ -28,6 +28,9 @@ jobs:
           force_label ci            5319e7 '.github'
           force_label agc           1d76db 'libSceAgc*'
           force_label shader        1d76db 'core/shader'
+          force_label 'unblocks title' 0e8a16 'Names a title ID on the Unblocks line'
+          force_label perf          fef2c0 'Performance change'
+          force_label 'perf numbers missing' e99695 'perf pull request without a before and after summary line'
 
           label relinker  5319e7 'core/relinker'
           label libc      fbca04 'libc'

.github/workflows/pr-labels.yml

@@ -0,0 +1,35 @@
+name: PR labels
+
+on:
+  pull_request_target:
+    branches: [main]
+    types: [opened, edited, reopened, synchronize]
+  schedule:
+    - cron: '45 * * * *'
+  workflow_dispatch:
+
+permissions: {}
+
+concurrency:
+  group: pr-labels-${{ github.event.pull_request.number || 'all' }}
+
+jobs:
+  labels:
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    permissions:
+      contents: read
+      pull-requests: write
+      issues: write
+    steps:
+      - uses: actions/checkout@v7
+      - env:
+          GH_TOKEN: ${{ github.token }}
+          GITHUB_REPOSITORY: ${{ github.repository }}
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+        run: |
+          if [ -n "$PR_NUMBER" ]; then
+            python3 -I tools/pr_labels.py --pr "$PR_NUMBER"
+          else
+            python3 -I tools/pr_labels.py --all
+          fi

CMakeLists.txt

@@ -119,6 +119,8 @@ if(BUILD_TESTING)
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_overlap_dependencies.py)
         add_test(NAME pr_overlap_tools COMMAND ${Python3_EXECUTABLE}
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_overlap.py)
+        add_test(NAME pr_labels COMMAND ${Python3_EXECUTABLE}
+            ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_labels.py)
         add_test(NAME progress_history COMMAND ${Python3_EXECUTABLE}
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_progress_history.py)
         add_test(NAME progress_report COMMAND ${Python3_EXECUTABLE}

CONTRIBUTING.md

@@ -58,12 +58,12 @@ The [pull request template](.github/pull_request_template.md) is the checklist f
 - A pull request that conflicts with `main` and has no activity for 7 days is labeled `stale` with a reminder. It is not closed.
 - If a pull request needs another one first, say so on the checklist line (`Depends on: #N`; `none` otherwise).
 - If a pull request fixes a reported issue, write `Fixes #N` on its checklist line (`none` otherwise). GitHub closes the issue on merge, and the `fix error` label is added within the hour.
-- If a pull request fixes a stop or a wrong result seen on a title, name it on the checklist line with the title ID and its [Game bring-up](https://github.com/boykopovar/AnyPS5/discussions/categories/game-bring-up) discussion (`Unblocks: PPSA01325 #3232`; `none` otherwise). Reviewers review these first.
+- If a pull request fixes a stop or a wrong result seen on a title, name it on the checklist line with the title ID and its [Game bring-up](https://github.com/boykopovar/AnyPS5/discussions/categories/game-bring-up) discussion (`Unblocks: PPSA01325 #3232`; `none` otherwise). Reviewers review these first. Such a pull request gets the `unblocks title` label.
 - For GPU changes, list each tested GPU's vendor, model, driver and OS on the checklist line. CI runs GPU tests only on lavapipe, so reviewers may ask for a run on another vendor before merging.
 - Fixes a title needs go to `main` as pull requests. A Game bring-up discussion lists the open pull requests and local changes the title still needs; update the list when one is merged, closed or replaced.
 - The PR overlap comment lists open pull requests that change the same lines or exports; agree with their authors which one goes first and declare it with `Depends on`.
 - Run the tests before opening the pull request and describe what was tested (OS, title or homebrew).
-- A pull request that claims a change in speed shows the summary lines of `main` and of the change, from the same machine and scene, and attaches the traces, captured as the [Performance report](.github/ISSUE_TEMPLATE/performance.yml) issue form describes. A slow scene is reported with the same form.
+- A pull request that claims a change in speed shows the summary lines of `main` and of the change, from the same machine and scene, and attaches the traces, captured as the [Performance report](.github/ISSUE_TEMPLATE/performance.yml) issue form describes. A slow scene is reported with the same form. A pull request titled `perf(...)` or `perf:` is labeled `perf`, and `perf numbers missing` until its description holds two valid summary lines with different commits.
 - Changes to the AGC driver, the shader recompiler or VideoOut name each GPU the GPU tests ran on (vendor, model, driver, OS) on the `GPUs tested` checklist line. CI runs these tests only on lavapipe, and hardware drivers fail where lavapipe and other vendors pass, so a reviewer can ask for a run on another vendor before merging.
 - Investigation notes, reports, screenshots and logs go in the pull request, not in the repository. Images for documentation go in the [gist](https://gist.github.com/boykopovar/0e53f2e1426f29ecd41e3b51540b8a90) comments.
 - Say whether the change was written with AI assistance. The author of the pull request is responsible for every line of it.

tools/pr_labels.py

@@ -0,0 +1,153 @@
+import argparse
+import json
+import os
+import re
+import subprocess
+import sys
+import time
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parent))
+import performance_compare
+
+UNBLOCKS = re.compile(r'Unblocks:[ \t]*([^\r\n]*(?:\r?\n[ \t]*[-*][ \t]+(?!\[[ xX]\])[^\r\n]*)*)')
+TITLE_ID = re.compile(r"(?<![A-Za-z0-9])[A-Z]{4}\d{5}(?!\d)")
+PERF = re.compile(r"perf(\(|!?:)")
+FENCE = re.compile(r"```[^\r\n]*\r?\n(.*?)```", re.S)
+MARKER = "<!-- pr-labels-perf -->"
+UNBLOCKS_LABEL = "unblocks title"
+PERF_LABEL = "perf"
+MISSING_LABEL = "perf numbers missing"
+MANAGED = {UNBLOCKS_LABEL, PERF_LABEL, MISSING_LABEL}
+WRITE_PAUSE = 1.0
+COMMENT = (f"{MARKER}\nThis pull request is labeled `perf`, so it needs the summary line of a capture on `main` and one with the change, "
+           "from the same title, scene and machine, as the Performance report form asks. "
+           "Please paste both `aps5-summary.jsonl` lines, attach the two traces and add the output of `tools/trace_digest.py`.")
+
+
+def gh(*args):
+    return subprocess.run(["gh", *args], capture_output=True, text=True, encoding="utf-8", check=True).stdout
+
+
+def repository():
+    return os.environ.get("GITHUB_REPOSITORY") or gh("repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner").strip()
+
+
+def strip_comments(body):
+    return re.sub(r"<!--.*?-->", "", body or "", flags=re.S)
+
+
+def title_ids(body):
+    match = UNBLOCKS.search(strip_comments(body))
+    return set(TITLE_ID.findall(match.group(1))) if match else set()
+
+
+def is_perf(title):
+    return bool(PERF.match(title or ""))
+
+
+def has_numbers(body):
+    body = strip_comments(body)
+    candidates = [line.strip() for line in body.splitlines()] + [block.strip() for block in FENCE.findall(body)]
+    commits = set()
+    for text in candidates:
+        if not text.startswith("{"):
+            continue
+        try:
+            summary = json.loads(text)
+            if isinstance(summary, dict):
+                commits.add(performance_compare.validate(summary)["commit"])
+        except (ValueError, RecursionError):
+            continue
+    return len(commits) >= 2
+
+
+def labels_for(pr):
+    wanted = set()
+    if title_ids(pr["body"]):
+        wanted.add(UNBLOCKS_LABEL)
+    if is_perf(pr["title"]):
+        wanted.add(PERF_LABEL)
+        if not has_numbers(pr["body"]):
+            wanted.add(MISSING_LABEL)
+    current = set(pr["labels"])
+    return wanted - current, (MANAGED & current) - wanted
+
+
+def decode_pages(out):
+    decoder = json.JSONDecoder()
+    offset = 0
+    items = []
+    while offset < len(out):
+        while offset < len(out) and out[offset].isspace():
+            offset += 1
+        if offset == len(out):
+            break
+        page, offset = decoder.raw_decode(out, offset)
+        if not isinstance(page, list):
+            raise ValueError("gh api --paginate returned a non-array page")
+        items.extend(page)
+    return items
+
+
+def pull(data):
+    return {"number": data["number"], "title": data["title"], "body": data["body"] or "",
+            "labels": [label["name"] for label in data["labels"]], "state": data["state"]}
+
+
+def commented(repo, number):
+    out = gh("api", "--paginate", f"repos/{repo}/issues/{number}/comments",
+             "--jq", f'[.[] | select(.body | contains("{MARKER}"))] | length')
+    return sum(int(count) for count in out.split())
+
+
+def process(repo, pr, dry_run, comment=True):
+    if pr["state"] != "open":
+        return set(), set(), False
+    add, remove = labels_for(pr)
+    wants_comment = comment and (MISSING_LABEL in add or (MISSING_LABEL in pr["labels"] and MISSING_LABEL not in remove))
+    post = wants_comment and not commented(repo, pr["number"])
+    if not dry_run:
+        if add or remove:
+            args = ["pr", "edit", str(pr["number"]), "-R", repo]
+            if add:
+                args += ["--add-label", ",".join(sorted(add))]
+            if remove:
+                args += ["--remove-label", ",".join(sorted(remove))]
+            gh(*args)
+        if post:
+            gh("api", "-X", "POST", f"repos/{repo}/issues/{pr['number']}/comments", "-f", f"body={COMMENT}")
+    return add, remove, post
+
+
+def main():
+    parser = argparse.ArgumentParser(description="Label pull requests from their title and template fields.")
+    which = parser.add_mutually_exclusive_group(required=True)
+    which.add_argument("--pr", type=int, help="label one pull request")
+    which.add_argument("--all", action="store_true", help="label every open pull request")
+    parser.add_argument("--dry-run", action="store_true", help="print the planned changes and write nothing")
+    args = parser.parse_args()
+    repo = repository()
+    if args.pr is not None:
+        prs = [pull(json.loads(gh("api", f"repos/{repo}/pulls/{args.pr}")))]
+    else:
+        prs = [pull(data) for data in decode_pages(gh("api", "--paginate", f"repos/{repo}/pulls?state=open&base=main&per_page=100"))]
+    failed = []
+    for pr in prs:
+        try:
+            add, remove, post = process(repo, pr, args.dry_run, comment=args.pr is not None)
+        except subprocess.CalledProcessError as error:
+            failed.append(pr["number"])
+            print(f"cannot label #{pr['number']}: {(error.stderr or '').strip()}", file=sys.stderr)
+            continue
+        if add or remove or post:
+            plan = [f"+{label}" for label in sorted(add)] + [f"-{label}" for label in sorted(remove)] + (["comment"] if post else [])
+            print(f"{'would change' if args.dry_run else 'changed'} #{pr['number']}: {' '.join(plan)}")
+            if args.all and not args.dry_run:
+                time.sleep(WRITE_PAUSE)
+    if failed:
+        sys.exit(f"{len(failed)} pull requests not labeled: {', '.join(f'#{number}' for number in failed)}")
+
+
+if __name__ == "__main__":
+    main()

tools/tests/test_pr_labels.py

@@ -0,0 +1,181 @@
+import copy
+import json
+import sys
+import unittest
+from pathlib import Path
+from unittest.mock import patch
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
+import pr_labels
+
+SUMMARY = {
+    "commit": "abc123", "dirty": False, "env": {"APS5_TRACE_FRAMES": "600:4"},
+    "gpu": "GPU", "driver": "Driver 1", "os": "OS 1", "resolution": "1920x1080",
+    "window": {"spec": "frames 600:4", "first_present": 600, "ms": 80.0}, "presents": 4,
+    "frame_ms": {"p50": 20.0, "p90": 40.0, "p99": 40.0, "max": 40.0},
+    "gpu_busy_ms_per_present": 5.0, "worker_busy_pct": {"queue worker 0x0": 50.0},
+    "top_phases_ms_per_frame": {"Driver.Packet": 12.0, "Driver.Present": 2.0},
+    "trace_dropped": False, "trace": "aps5-trace-abc123-7.json.gz",
+}
+TEMPLATE = ("- [ ] Unblocks: <!-- title ID and Game bring-up discussion, e.g. PPSA01325 #3232, or none -->\n"
+            "- [ ] Depends on: none\n")
+
+
+def summary(commit):
+    record = copy.deepcopy(SUMMARY)
+    record["commit"] = commit
+    return json.dumps(record)
+
+
+def pr(title="fix: x", body="", labels=()):
+    return {"number": 7, "title": title, "body": body, "labels": list(labels), "state": "open"}
+
+
+class TitleIdTests(unittest.TestCase):
+    def test_forms(self):
+        cases = [
+            (TEMPLATE, set()),
+            ("- [x] Unblocks: PPSA01325 #3232\n", {"PPSA01325"}),
+            ("- [ ] Unblocks: **PPSA01325 #3232, CUSA12345 #1**", {"PPSA01325", "CUSA12345"}),
+            ("- [ ] Unblocks: none\nSeen on PPSA01325.\n", set()),
+            ("- [ ] Unblocks:\n  - PPSA01325 #3232\n  - PPSA00001\n- [ ] Depends on: none\n", {"PPSA01325", "PPSA00001"}),
+            ("- [ ] Unblocks:\n- [ ] Depends on: PPSA01325\n", set()),
+            ("- [ ] Unblocks: <!-- PPSA01325 -->\n", set()),
+            ("- [x] Unblocks: PPSA01325_00 #3232\n", {"PPSA01325"}),
+            ("- [x] Unblocks: PPSA013250, XPPSA01325\n", set()),
+            ("", set()),
+        ]
+        for body, expected in cases:
+            with self.subTest(body=body):
+                self.assertEqual(pr_labels.title_ids(body), expected)
+
+    def test_none_body(self):
+        self.assertEqual(pr_labels.title_ids(None), set())
+
+
+class PerfTests(unittest.TestCase):
+    def test_titles(self):
+        for title, expected in [("perf(agc): x", True), ("perf: x", True), ("perf!: x", True), ("perf(agc)!: x", True),
+                                ("fix(perf): x", False), ("feat: perf", False), ("performance: x", False)]:
+            with self.subTest(title=title):
+                self.assertEqual(pr_labels.is_perf(title), expected)
+
+
+class NumbersTests(unittest.TestCase):
+    def test_counts(self):
+        cases = [
+            ("", False),
+            (summary("a"), False),
+            (summary("a") + "\n" + summary("b"), True),
+            ("before\n" + summary("a") + "\nafter\n" + summary("b") + "\n", True),
+            (summary("a") + "\n" + summary("a"), False),
+            (summary("a") + "\n{not json", False),
+            ("{}\n{}", False),
+            ("```json\n" + json.dumps(json.loads(summary("a")), indent=2) + "\n```\n```json\n" + summary("b") + "\n```", True),
+            ("<!-- " + summary("a") + " -->\n" + summary("b"), False),
+            (summary("a") + "\r\n" + summary("b") + "\r\n", True),
+            (summary("a") + "\n{" + "[" * 100000 + "\n" + summary("b"), True),
+        ]
+        for body, expected in cases:
+            with self.subTest(body=body[:40]):
+                self.assertEqual(pr_labels.has_numbers(body), expected)
+
+    def test_invalid_summary_not_counted(self):
+        broken = json.loads(summary("b"))
+        broken["presents"] = 0
+        self.assertFalse(pr_labels.has_numbers(summary("a") + "\n" + json.dumps(broken)))
+
+
+class LabelTests(unittest.TestCase):
+    def test_labels(self):
+        self.assertEqual(pr_labels.labels_for(pr()), (set(), set()))
+        self.assertEqual(pr_labels.labels_for(pr("perf: x", TEMPLATE)), ({"perf", "perf numbers missing"}, set()))
+        both = TEMPLATE + summary("a") + "\n" + summary("b")
+        self.assertEqual(pr_labels.labels_for(pr("perf: x", both, ["perf", "perf numbers missing"])), (set(), {"perf numbers missing"}))
+        self.assertEqual(pr_labels.labels_for(pr("fix: x", "Unblocks: PPSA01325", ["perf", "other"])), ({"unblocks title"}, {"perf"}))
+        self.assertEqual(pr_labels.labels_for(pr("fix: x", "Unblocks: none", ["unblocks title", "other"])), (set(), {"unblocks title"}))
+
+
+class ProcessTests(unittest.TestCase):
+    def run_process(self, pull, comments=0, dry_run=False, comment=True):
+        calls = []
+
+        def fake(*args):
+            calls.append(args)
+            return ""
+
+        with patch.object(pr_labels, "gh", side_effect=fake), patch.object(pr_labels, "commented", return_value=comments) as counted:
+            result = pr_labels.process("o/r", pull, dry_run, comment)
+        self.comment_lookups = counted.call_count
+        return result, calls
+
+    def test_backfill_labels_without_commenting(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE), comment=False)
+        self.assertEqual(result, ({"perf", "perf numbers missing"}, set(), False))
+        self.assertEqual(calls, [("pr", "edit", "7", "-R", "o/r", "--add-label", "perf,perf numbers missing")])
+        self.assertEqual(self.comment_lookups, 0)
+
+    def test_no_edit_when_labels_match(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"]), comments=1)
+        self.assertEqual(result, (set(), set(), False))
+        self.assertEqual(calls, [])
+
+    def test_edit_and_single_comment(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE))
+        self.assertEqual(result, ({"perf", "perf numbers missing"}, set(), True))
+        self.assertEqual(calls[0], ("pr", "edit", "7", "-R", "o/r", "--add-label", "perf,perf numbers missing"))
+        self.assertIn("-f", calls[1])
+        self.assertIn(pr_labels.MARKER, calls[1][-1])
+        self.assertEqual(len(calls), 2)
+
+    def test_comment_not_posted_twice(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"]), comments=1)
+        self.assertFalse(result[2])
+        result, calls = self.run_process(pr("perf: x", TEMPLATE), comments=1)
+        self.assertEqual(len(calls), 1)
+
+    def test_comment_restored_for_labeled_pr_without_one(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"]))
+        self.assertTrue(result[2])
+        self.assertEqual(len(calls), 1)
+
+    def test_numbers_present_removes_label_keeps_comment(self):
+        body = summary("a") + "\n" + summary("b")
+        result, calls = self.run_process(pr("perf: x", body, ["perf", "perf numbers missing"]), comments=1)
+        self.assertEqual(result, (set(), {"perf numbers missing"}, False))
+        self.assertEqual(calls, [("pr", "edit", "7", "-R", "o/r", "--remove-label", "perf numbers missing")])
+
+    def test_dry_run_writes_nothing(self):
+        result, calls = self.run_process(pr("perf: x", TEMPLATE), dry_run=True)
+        self.assertEqual(result[2], True)
+        self.assertEqual(calls, [])
+
+    def test_closed_pull_request_ignored(self):
+        pull = pr("perf: x", TEMPLATE)
+        pull["state"] = "closed"
+        self.assertEqual(self.run_process(pull)[1], [])
+
+
+class MainTests(unittest.TestCase):
+    def test_failed_pull_request_does_not_stop_the_others(self):
+        listed = [{"number": number, "title": "perf: x", "body": "", "labels": [], "state": "open"} for number in (1, 2)]
+        edited = []
+
+        def fake(*args):
+            if args[0] == "api":
+                return json.dumps(listed)
+            edited.append(args[2])
+            if args[2] == "1":
+                raise pr_labels.subprocess.CalledProcessError(1, "gh", stderr="HTTP 403")
+            return ""
+
+        with patch.object(pr_labels, "gh", side_effect=fake), patch.object(pr_labels, "WRITE_PAUSE", 0), \
+                patch.object(sys, "argv", ["pr_labels.py", "--all"]), patch.dict(pr_labels.os.environ, {"GITHUB_REPOSITORY": "o/r"}):
+            with self.assertRaises(SystemExit) as raised:
+                pr_labels.main()
+        self.assertEqual(edited, ["1", "2"])
+        self.assertIn("#1", str(raised.exception.code))
+
+
+if __name__ == "__main__":
+    unittest.main()