From e40803d7083400b371a81375bb46a5d185ea6bd2 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Fri, 18 Apr 2025 01:58:05 +0000 Subject: [PATCH 1/3] ci: Allow skipping extensive tests with `ci: skip-extensive` Sometimes we do refactoring that moves things around and triggers an extensive test, even though the implementation didn't change. There isn't any need to run full extensive CI in these cases, so add a way to skip it from the PR message. --- .github/workflows/main.yaml | 15 ++++--- ci/ci-util.py | 88 ++++++++++++++++++++++++++----------- 2 files changed, 73 insertions(+), 30 deletions(-) diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 93c56c9d4..2b2891ab2 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -239,6 +239,9 @@ jobs: name: Calculate job matrix runs-on: ubuntu-24.04 timeout-minutes: 10 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} outputs: matrix: ${{ steps.script.outputs.matrix }} steps: @@ -267,7 +270,7 @@ jobs: # this is not currently possible https://github.com/actions/runner/issues/1985. include: ${{ fromJSON(needs.calculate_extensive_matrix.outputs.matrix).matrix }} env: - CHANGED: ${{ matrix.changed }} + TO_TEST: ${{ matrix.to_test }} steps: - uses: actions/checkout@v4 with: @@ -279,16 +282,18 @@ jobs: - uses: Swatinem/rust-cache@v2 - name: Run extensive tests run: | - echo "Changed: '$CHANGED'" - if [ -z "$CHANGED" ]; then + echo "Tests to run: '$TO_TEST'" + if [ -z "$TO_TEST" ]; then echo "No tests to run, exiting." exit fi + set -x + # Run the non-extensive tests first to catch any easy failures - cargo t --profile release-checked -- "$CHANGED" + cargo t --profile release-checked -- "$TO_TEST" - LIBM_EXTENSIVE_TESTS="$CHANGED" cargo t \ + LIBM_EXTENSIVE_TESTS="$TO_TEST" cargo t \ --features build-mpfr,unstable,force-soft-floats \ --profile release-checked \ -- extensive diff --git a/ci/ci-util.py b/ci/ci-util.py index 7464fd425..8b07dde31 100755 --- a/ci/ci-util.py +++ b/ci/ci-util.py @@ -6,6 +6,7 @@ """ import json +import os import subprocess as sp import sys from dataclasses import dataclass @@ -13,7 +14,7 @@ from inspect import cleandoc from os import getenv from pathlib import Path -from typing import TypedDict +from typing import TypedDict, Self USAGE = cleandoc( """ @@ -51,6 +52,8 @@ ARTIFACT_GLOB = "baseline-icount*" # Place this in a PR body to skip regression checks (must be at the start of a line). REGRESSION_DIRECTIVE = "ci: allow-regressions" +# Place this in a PR body to skip extensive tests +SKIP_EXTENSIVE_DIRECTIVE = "ci: skip-extensive" # Don't run exhaustive tests if these files change, even if they contaiin a function # definition. @@ -68,6 +71,39 @@ def eprint(*args, **kwargs): print(*args, file=sys.stderr, **kwargs) +@dataclass +class PrInfo: + """GitHub response for PR query""" + + body: str + commits: list[str] + created_at: str + number: int + + @classmethod + def load(cls, pr_number: int | str) -> Self: + """For a given PR number, query the body and commit list""" + pr_info = sp.check_output( + [ + "gh", + "pr", + "view", + str(pr_number), + "--json=number,commits,body,createdAt", + # Flatten the commit list to only hashes, change a key to snake naming + "--jq=.commits |= map(.oid) | .created_at = .createdAt | del(.createdAt)", + ], + text=True, + ) + eprint("PR info:", json.dumps(pr_info, indent=4)) + return cls(**json.loads(pr_info)) + + def contains_directive(self, directive: str) -> bool: + """Return true if the provided directive is on a line in the PR body""" + lines = self.body.splitlines() + return any(line.startswith(directive) for line in lines) + + class FunctionDef(TypedDict): """Type for an entry in `function-definitions.json`""" @@ -149,7 +185,7 @@ def changed_routines(self) -> dict[str, list[str]]: eprint(f"changed files for {name}: {changed}") routines.add(name) - ret = {} + ret: dict[str, list[str]] = {} for r in sorted(routines): ret.setdefault(self.defs[r]["type"], []).append(r) @@ -159,13 +195,27 @@ def make_workflow_output(self) -> str: """Create a JSON object a list items for each type's changed files, if any did change, and the routines that were affected by the change. """ + + pr_number = os.environ.get("PR_NUMBER") + skip_tests = False + + if pr_number is not None: + pr = PrInfo.load(pr_number) + skip_tests = pr.contains_directive(SKIP_EXTENSIVE_DIRECTIVE) + + if skip_tests: + eprint("Skipping all extensive tests") + changed = self.changed_routines() ret = [] for ty in TYPES: ty_changed = changed.get(ty, []) + changed_str = ",".join(ty_changed) + item = { "ty": ty, - "changed": ",".join(ty_changed), + "changed": changed_str, + "to_test": "" if skip_tests else changed_str, } ret.append(item) output = json.dumps({"matrix": ret}, separators=(",", ":")) @@ -266,13 +316,13 @@ def check_iai_regressions(args: list[str]): found. """ - iai_home = "iai-home" - pr_number = False + iai_home_str = "iai-home" + pr_number = None while len(args) > 0: match args: case ["--home", home, *rest]: - iai_home = home + iai_home_str = home args = rest case ["--allow-pr-override", pr_num, *rest]: pr_number = pr_num @@ -281,10 +331,10 @@ def check_iai_regressions(args: list[str]): eprint(USAGE) exit(1) - iai_home = Path(iai_home) + iai_home = Path(iai_home_str) found_summaries = False - regressions = [] + regressions: list[dict] = [] for summary_path in iglob("**/summary.json", root_dir=iai_home, recursive=True): found_summaries = True with open(iai_home / summary_path, "r") as f: @@ -292,7 +342,9 @@ def check_iai_regressions(args: list[str]): summary_regs = [] run = summary["callgrind_summary"]["callgrind_run"] - name_entry = {"name": f"{summary["function_name"]}.{summary["id"]}"} + fname = summary["function_name"] + id = summary["id"] + name_entry = {"name": f"{fname}.{id}"} for segment in run["segments"]: summary_regs.extend(segment["regressions"]) @@ -312,22 +364,8 @@ def check_iai_regressions(args: list[str]): eprint("Found regressions:", json.dumps(regressions, indent=4)) if pr_number is not None: - pr_info = sp.check_output( - [ - "gh", - "pr", - "view", - str(pr_number), - "--json=number,commits,body,createdAt", - "--jq=.commits |= map(.oid)", - ], - text=True, - ) - pr = json.loads(pr_info) - eprint("PR info:", json.dumps(pr, indent=4)) - - lines = pr["body"].splitlines() - if any(line.startswith(REGRESSION_DIRECTIVE) for line in lines): + pr = PrInfo.load(pr_number) + if pr.contains_directive(REGRESSION_DIRECTIVE): eprint("PR allows regressions, returning") return From 3a63974eeed91b4fb95af73b885337baeef296bd Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Fri, 18 Apr 2025 02:20:21 +0000 Subject: [PATCH 2/3] ci: Require `ci: allow-many-extensive` if a threshold is exceeded Error out when too many extensive tests would be run unless `ci: allow-many-extensive` is in the PR description. This allows us to set a much higher CI timeout with less risk that a 4+ hour job gets started by accident. --- ci/ci-util.py | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/ci/ci-util.py b/ci/ci-util.py index 8b07dde31..aae791d0f 100755 --- a/ci/ci-util.py +++ b/ci/ci-util.py @@ -54,6 +54,11 @@ REGRESSION_DIRECTIVE = "ci: allow-regressions" # Place this in a PR body to skip extensive tests SKIP_EXTENSIVE_DIRECTIVE = "ci: skip-extensive" +# Place this in a PR body to allow running a large number of extensive tests. If not +# set, this script will error out if a threshold is exceeded in order to avoid +# accidentally spending huge amounts of CI time. +ALLOW_MANY_EXTENSIVE_DIRECTIVE = "ci: allow-many-extensive" +MANY_EXTENSIVE_THRESHOLD = 20 # Don't run exhaustive tests if these files change, even if they contaiin a function # definition. @@ -198,28 +203,45 @@ def make_workflow_output(self) -> str: pr_number = os.environ.get("PR_NUMBER") skip_tests = False + error_on_many_tests = False if pr_number is not None: pr = PrInfo.load(pr_number) skip_tests = pr.contains_directive(SKIP_EXTENSIVE_DIRECTIVE) + error_on_many_tests = not pr.contains_directive( + ALLOW_MANY_EXTENSIVE_DIRECTIVE + ) if skip_tests: eprint("Skipping all extensive tests") changed = self.changed_routines() ret = [] + total_to_test = 0 + for ty in TYPES: ty_changed = changed.get(ty, []) - changed_str = ",".join(ty_changed) + ty_to_test = [] if skip_tests else ty_changed + total_to_test += len(ty_to_test) item = { "ty": ty, - "changed": changed_str, - "to_test": "" if skip_tests else changed_str, + "changed": ",".join(ty_changed), + "to_test": ",".join(ty_to_test), } + ret.append(item) output = json.dumps({"matrix": ret}, separators=(",", ":")) eprint(f"output: {output}") + eprint(f"total extensive tests: {total_to_test}") + + if error_on_many_tests and total_to_test > MANY_EXTENSIVE_THRESHOLD: + eprint( + f"More than {MANY_EXTENSIVE_THRESHOLD} tests would be run; add" + f" `{ALLOW_MANY_EXTENSIVE_DIRECTIVE}` to the PR body if this is intentional" + ) + exit(1) + return output From 773d6dcd374b549288b8aa20034a4237ac2b4644 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Fri, 18 Apr 2025 01:24:38 +0000 Subject: [PATCH 3/3] ci: Increase the timeout for extensive tests The reorganization PR has caused this to fail once before because every file shows up as changed. Increase the timeout so this doesn't happen. We now cancel the job if too many extensive tests are run unless `ci: allow-many-extensive` is in the PR description, so this helps prevent the limit being hit by accident. --- .github/workflows/main.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index 2b2891ab2..c925e63aa 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -261,7 +261,7 @@ jobs: - clippy - calculate_extensive_matrix runs-on: ubuntu-24.04 - timeout-minutes: 180 + timeout-minutes: 240 # 4 hours strategy: matrix: # Use the output from `calculate_extensive_matrix` to calculate the matrix