From b23dcfe1124f4a2ab2361551bb8326ed20f96fdf Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Thu, 29 Oct 2026 10:57:43 -0400 Subject: [PATCH 1/2] ci(rccl): build the coco PR baseline at the PR's merge-base The coco PR gate built its develop baseline at develop tip on every run, so changes merged after a PR branched read as the PR's own. Resolve the PR's merge-base with its base branch and pass it as --baseline-sha when the coco harness supports the flag. Otherwise warn, or fail the setup when the run requires the pin. Signed-off-by: speriaswamy-amd --- .../actions/rccl-resolve-coco-run/action.yml | 12 + .github/scripts/rccl_resolve_coco_run.py | 176 +++++++- .../tests/rccl_resolve_coco_run_test.py | 419 +++++++++++++++++- 3 files changed, 597 insertions(+), 10 deletions(-) diff --git a/.github/actions/rccl-resolve-coco-run/action.yml b/.github/actions/rccl-resolve-coco-run/action.yml index bf84931001a1..7833f6a1cba8 100644 --- a/.github/actions/rccl-resolve-coco-run/action.yml +++ b/.github/actions/rccl-resolve-coco-run/action.yml @@ -33,6 +33,13 @@ inputs: description: 'Monorepo slug. Required when pr_number is set.' required: false default: '' + require_baseline_sha: + description: >- + Fail a PR run whose coco harness cannot build the baseline at the PR's + merge-base (coco pr --baseline-sha), instead of warning and measuring + against the baseline's configured ref. + required: false + default: 'false' client-id: description: 'GitHub App client ID used to mint the harness checkout token.' required: true @@ -80,10 +87,15 @@ runs: working-directory: _coco env: DRY_RUN: ${{ inputs.dry_run }} + # Where the "Checkout the coco harness" step put it; the probe reads its CLI. + HARNESS_DIR: ${{ github.workspace }}/_coco + REQUIRE_BASELINE_SHA: ${{ inputs.require_baseline_sha }} PR_NUMBER: ${{ github.event_name == 'pull_request' && github.event.pull_request.number || inputs.pr_number }} PR_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || inputs.pr_sha }} PR_REPO: ${{ github.event_name == 'pull_request' && github.repository || inputs.pr_repo }} EVENT_NAME: ${{ github.event_name }} + # Reads the PR's base branch and merge-base from the REST API. + GITHUB_TOKEN: ${{ github.token }} PERIODIC_MODE: ${{ inputs.periodic_mode }} SCHEDULE: ${{ inputs.schedule }} run: python3 "${GITHUB_WORKSPACE}/.github/scripts/rccl_resolve_coco_run.py" diff --git a/.github/scripts/rccl_resolve_coco_run.py b/.github/scripts/rccl_resolve_coco_run.py index 24e223641e15..ec1e011b6c3c 100644 --- a/.github/scripts/rccl_resolve_coco_run.py +++ b/.github/scripts/rccl_resolve_coco_run.py @@ -6,18 +6,25 @@ from __future__ import annotations +import http.client +import json import os +import re import subprocess import sys +import urllib.error +import urllib.parse +import urllib.request from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path +from typing import Callable _SCRIPT_DIR = Path(__file__).resolve().parent if str(_SCRIPT_DIR) not in sys.path: sys.path.insert(0, str(_SCRIPT_DIR)) -from ci_utils import set_github_output +from ci_utils import retry, set_github_output # Must stay in step with the cron entries in rccl-coco-scheduled.yml. WEEKDAY_NIGHTLY_CRON = "47 3 * * 1-5" @@ -25,6 +32,18 @@ VALID_MODES = frozenset({"pr", "nightly", "weekly", "monthly"}) +FULL_SHA = re.compile(r"^[0-9a-f]{40}$") + +# The coco harness CLI sources searched for the --baseline-sha option. Any file +# and either quote style counts, so moving the declaration does not hide it. +HARNESS_CLI_DIR = Path("coco") / "cli" +BASELINE_SHA_OPTION = re.compile(r"""["']--baseline-sha["']""") + +# Besides server errors, a rate limit is worth waiting out. +RETRYABLE_STATUSES = frozenset({403, 429}) + +GitHubGet = Callable[[str], dict] + @dataclass(frozen=True) class ResolvedRun: @@ -70,12 +89,14 @@ def resolve_mode_and_trigger( return periodic_mode, "manual" -def build_coco_args(*, mode: str, dry_run: bool) -> str: +def build_coco_args(*, mode: str, dry_run: bool, baseline_sha: str = "") -> str: args = "-vv --skip-failed-builds" if mode == "pr": # No --prune-build: the build and test phases share a directory and # prune would delete trees the later phase still needs. args += " --skip-baselines" + if baseline_sha: + args += f" --baseline-sha {baseline_sha}" else: args += " --prune-build" if dry_run: @@ -83,6 +104,95 @@ def build_coco_args(*, mode: str, dry_run: bool) -> str: return args +def harness_supports_baseline_sha(harness_dir: Path) -> bool: + """True when the coco harness in *harness_dir* declares ``--baseline-sha``.""" + return any( + BASELINE_SHA_OPTION.search(source.read_text(encoding="utf-8")) + for source in (harness_dir / HARNESS_CLI_DIR).rglob("*.py") + ) + + +class GitHubApiError(Exception): + """A GitHub REST call failed; the message names the request and the reason.""" + + +class TransientGitHubError(GitHubApiError): + """A failure worth retrying: a server error, a rate limit or a lost connection.""" + + +@retry(max_attempts=3, delay_seconds=2, exceptions=(TransientGitHubError,)) +def github_get(url: str) -> dict: + """GET a GitHub REST API URL as JSON, authenticated by ``GITHUB_TOKEN``. + + Raises ``GitHubApiError`` naming *url* and the reason. Only server errors, + rate limits and transport failures (timeouts, refused or reset connections, + a truncated response) are retried; any other HTTP error, such as a 404 for + a mistyped PR, and a malformed URL fail at once. + """ + token = os.environ.get("GITHUB_TOKEN", "") + if token == "": + raise GitHubApiError("GITHUB_TOKEN is not set; cannot call the GitHub API.") + request = urllib.request.Request( + url, + headers={ + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {token}", + "X-GitHub-Api-Version": "2022-11-28", + }, + ) + try: + with urllib.request.urlopen(request, timeout=30) as response: + return json.load(response) + except urllib.error.HTTPError as exc: + failure = ( + TransientGitHubError + if exc.code >= 500 or exc.code in RETRYABLE_STATUSES + else GitHubApiError + ) + raise failure(f"GET {url} failed: HTTP {exc.code} {exc.reason}") from exc + except http.client.InvalidURL as exc: + raise GitHubApiError(f"GET {url} is not a valid URL: {exc}") from exc + except (urllib.error.URLError, http.client.HTTPException, OSError) as exc: + raise TransientGitHubError(f"GET {url} failed: {exc!r}") from exc + except ValueError as exc: + raise GitHubApiError(f"GET {url} returned invalid JSON: {exc}") from exc + + +def resolve_merge_base( + *, pr_repo: str, pr_number: str, pr_sha: str, get: GitHubGet = github_get +) -> str: + """The merge-base of the PR head *pr_sha* with the PR's base branch. + + The baseline arm is built at this commit, so a PR is measured only + against code it contains. Raises ``ValueError`` when it cannot be + resolved, or when it is the PR head itself, and ``GitHubApiError`` when a + GitHub call fails. + """ + api = f"https://api.github.com/repos/{pr_repo}" + pr_sha = pr_sha.lower() + try: + base_ref = get(f"{api}/pulls/{pr_number}")["base"]["ref"] + compare = get( + f"{api}/compare/{urllib.parse.quote(base_ref, safe='/')}...{pr_sha}" + ) + merge_base = compare["merge_base_commit"]["sha"] + except (KeyError, TypeError) as exc: + raise ValueError( + f"GitHub's response for PR #{pr_number} lacks {exc}; " + "cannot resolve its merge-base." + ) from exc + if not FULL_SHA.match(merge_base or ""): + raise ValueError( + f"Could not resolve the merge-base of PR #{pr_number} with {base_ref!r}." + ) + if merge_base == pr_sha: + raise ValueError( + f"PR #{pr_number}'s head {pr_sha} is its merge-base with {base_ref!r}; " + "there is no change to measure." + ) + return merge_base + + def resolve_run( *, pr_number: str, @@ -94,6 +204,7 @@ def resolve_run( dry_run: bool, day_of_month: int | None = None, coco_sha: str | None = None, + baseline_sha: str = "", ) -> ResolvedRun: mode, trigger_kind = resolve_mode_and_trigger( pr_number=pr_number, @@ -117,26 +228,77 @@ def resolve_run( mode=mode, trigger_kind=trigger_kind, coco_ref=coco_sha, - coco_args=build_coco_args(mode=mode, dry_run=dry_run), + coco_args=build_coco_args( + mode=mode, dry_run=dry_run, baseline_sha=baseline_sha + ), pr_number=pr_number, pr_sha=pr_sha, pr_repo=pr_repo, ) +def resolve_baseline_sha( + *, + pr_number: str, + pr_sha: str, + pr_repo: str, + harness_dir: Path, + require: bool, + get: GitHubGet = github_get, +) -> str: + """The commit to build a PR run's baseline at, or ``""`` to keep its configured ref. + + Empty for a periodic run. A harness without ``--baseline-sha`` leaves the + baseline on its configured ref with a workflow warning, or raises + ``ValueError`` when *require* is set. + """ + if not pr_number: + return "" + if not harness_supports_baseline_sha(harness_dir): + if require: + raise ValueError( + "This coco harness has no --baseline-sha, but this run requires the " + "PR baseline to be built at the PR's merge-base." + ) + print( + "::warning::This coco harness has no --baseline-sha; the PR is measured " + "against the baseline's configured ref, not its merge-base." + ) + return "" + return resolve_merge_base( + pr_repo=pr_repo, pr_number=pr_number, pr_sha=pr_sha, get=get + ) + + def main() -> None: dry_run = os.environ.get("DRY_RUN", "false").lower() == "true" + pr_number = os.environ.get("PR_NUMBER", "") + pr_sha = os.environ.get("PR_SHA", "") + pr_repo = os.environ.get("PR_REPO", "") try: + harness_dir = os.environ.get("HARNESS_DIR", "") + if harness_dir == "": + raise ValueError( + "HARNESS_DIR is not set; it names the checked-out coco harness." + ) resolved = resolve_run( - pr_number=os.environ.get("PR_NUMBER", ""), - pr_sha=os.environ.get("PR_SHA", ""), - pr_repo=os.environ.get("PR_REPO", ""), + pr_number=pr_number, + pr_sha=pr_sha, + pr_repo=pr_repo, event_name=os.environ.get("EVENT_NAME", ""), schedule=os.environ.get("SCHEDULE", ""), periodic_mode=os.environ.get("PERIODIC_MODE", "nightly"), dry_run=dry_run, + baseline_sha=resolve_baseline_sha( + pr_number=pr_number, + pr_sha=pr_sha, + pr_repo=pr_repo, + harness_dir=Path(harness_dir), + require=os.environ.get("REQUIRE_BASELINE_SHA", "false").lower() + == "true", + ), ) - except ValueError as exc: + except (ValueError, GitHubApiError) as exc: print(f"::error::{exc}") raise SystemExit(1) from exc diff --git a/.github/scripts/tests/rccl_resolve_coco_run_test.py b/.github/scripts/tests/rccl_resolve_coco_run_test.py index 04bd5f8adcc6..0a8781d554cc 100644 --- a/.github/scripts/tests/rccl_resolve_coco_run_test.py +++ b/.github/scripts/tests/rccl_resolve_coco_run_test.py @@ -3,11 +3,24 @@ """Tests for rccl_resolve_coco_run.py.""" +import contextlib +import http.client +import io +import json import os import re +import shutil +import socket +import subprocess import sys +import threading +import tempfile import unittest +import urllib.error +import urllib.parse +import urllib.request from pathlib import Path +from unittest import mock sys.path.insert(0, os.fspath(Path(__file__).parent.parent)) import rccl_resolve_coco_run as resolve @@ -23,9 +36,7 @@ def test_workflow_crons_are_all_recognised(self): text = SCHEDULED_WORKFLOW.read_text(encoding="utf-8") crons = set(re.findall(r"^\s*-\s*cron:\s*'([^']+)'", text, re.MULTILINE)) self.assertTrue(crons, f"no cron entries found in {SCHEDULED_WORKFLOW}") - self.assertEqual( - crons, {resolve.WEEKDAY_NIGHTLY_CRON, resolve.SATURDAY_CRON} - ) + self.assertEqual(crons, {resolve.WEEKDAY_NIGHTLY_CRON, resolve.SATURDAY_CRON}) class ScheduleToModeTest(unittest.TestCase): @@ -109,5 +120,407 @@ def test_rejects_unknown_manual_mode(self): ) +HEAD = "a" * 40 +MERGE_BASE = "b" * 40 +API = "https://api.github.com/repos/ROCm/rocm-systems" + + +def fake_github(merge_base=MERGE_BASE, base_ref="develop", head=HEAD): + """A GitHub REST stub serving one PR and its compare against the base branch.""" + compare = f"{API}/compare/{urllib.parse.quote(base_ref, safe='/')}...{head}" + responses = { + f"{API}/pulls/42": {"base": {"ref": base_ref}}, + compare: {"merge_base_commit": {"sha": merge_base}}, + } + return responses.__getitem__ + + +def merge_base_of(**kwargs): + return resolve.resolve_merge_base( + pr_repo="ROCm/rocm-systems", + pr_number="42", + pr_sha=kwargs.pop("pr_sha", HEAD), + get=kwargs.pop("get", fake_github()), + ) + + +class BaselineShaArgsTest(unittest.TestCase): + def test_pr_args_pin_the_baseline_to_the_given_commit(self): + args = resolve.build_coco_args( + mode="pr", dry_run=False, baseline_sha=MERGE_BASE + ) + self.assertIn(f"--baseline-sha {MERGE_BASE}", args) + + def test_pr_args_without_a_commit_keep_the_configured_baseline(self): + self.assertNotIn( + "--baseline-sha", resolve.build_coco_args(mode="pr", dry_run=False) + ) + + def test_periodic_args_never_pin(self): + args = resolve.build_coco_args( + mode="nightly", dry_run=False, baseline_sha=MERGE_BASE + ) + self.assertNotIn("--baseline-sha", args) + + +class ResolveMergeBaseTest(unittest.TestCase): + def test_returns_the_merge_base_with_the_prs_base_branch(self): + """The baseline arm is built here, so a PR is measured only against code it contains.""" + self.assertEqual(merge_base_of(), MERGE_BASE) + + def test_uses_the_prs_own_base_branch(self): + self.assertEqual( + merge_base_of(get=fake_github(base_ref="rccl-develop")), MERGE_BASE + ) + + def test_a_base_branch_with_reserved_characters_is_quoted(self): + """An unquoted ``#`` would start a URL fragment and swallow the head SHA.""" + requested = [] + serve = fake_github(base_ref="release/foo#1") + + def get(url): + requested.append(url) + return serve(url) + + self.assertEqual(merge_base_of(get=get), MERGE_BASE) + self.assertIn("/compare/release/foo%231...", requested[-1]) + + def test_a_head_that_is_its_own_merge_base_is_refused(self): + """Both arms on one commit would compare the PR with itself and always pass.""" + with self.assertRaisesRegex(ValueError, "no change to measure"): + merge_base_of(get=fake_github(merge_base=HEAD)) + + def test_an_uppercase_head_is_compared_in_lowercase(self): + """The workflow accepts an uppercase head SHA; GitHub returns lowercase ones.""" + with self.assertRaisesRegex(ValueError, "no change to measure"): + merge_base_of(pr_sha=HEAD.upper(), get=fake_github(merge_base=HEAD)) + + def test_a_malformed_merge_base_is_refused(self): + with self.assertRaises(ValueError): + merge_base_of(get=fake_github(merge_base="deadbeef")) + + def test_an_unexpected_response_names_the_pr_and_the_missing_field(self): + with self.assertRaisesRegex(ValueError, r"PR #42 lacks 'base'"): + merge_base_of(get=lambda url: {}) + + +def http_error(code): + return urllib.error.HTTPError( + f"{API}/pulls/42", code, "reason", {}, io.BytesIO(b"") + ) + + +def direct_urlopen(request, timeout): + """``urlopen`` through a fresh opener with no proxy. + + ``urllib.request.urlopen`` builds its default opener once, from whatever + proxy variables the environment had then, so a test that talks to a local + socket must not depend on it. + """ + opener = urllib.request.build_opener(urllib.request.ProxyHandler({})) + return opener.open(request, timeout=timeout) + + +def response(payload): + """A urlopen result: a context manager whose body is *payload* as JSON.""" + body = mock.MagicMock() + body.__enter__.return_value = io.BytesIO(json.dumps(payload).encode()) + return body + + +@mock.patch.dict(os.environ, {"GITHUB_TOKEN": "test-token"}) +@mock.patch("time.sleep") +class GithubGetTest(unittest.TestCase): + URL = f"{API}/pulls/42" + + def get(self, side_effect): + with mock.patch("urllib.request.urlopen", side_effect=side_effect) as urlopen: + try: + return resolve.github_get(self.URL), urlopen + except resolve.GitHubApiError as exc: + return exc, urlopen + + def test_a_missing_pr_fails_at_once_naming_the_request(self, _sleep): + """A 404 cannot recover: no retries, and the message says what was requested.""" + error, urlopen = self.get([http_error(404)]) + self.assertIsInstance(error, resolve.GitHubApiError) + self.assertNotIsInstance(error, resolve.TransientGitHubError) + self.assertEqual(urlopen.call_count, 1) + self.assertIn(f"GET {self.URL}", str(error)) + self.assertIn("HTTP 404", str(error)) + + def test_a_server_error_is_retried_until_it_succeeds(self, _sleep): + payload, urlopen = self.get([http_error(502), response({"ok": True})]) + self.assertEqual(payload, {"ok": True}) + self.assertEqual(urlopen.call_count, 2) + + def test_a_persistent_server_error_gives_up_with_the_request_named(self, _sleep): + error, urlopen = self.get([http_error(500)] * 3) + self.assertEqual(urlopen.call_count, 3) + self.assertIn(f"GET {self.URL}", str(error)) + self.assertIn("HTTP 500", str(error)) + + def test_a_rate_limit_is_retried(self, _sleep): + payload, urlopen = self.get([http_error(429), response({"ok": True})]) + self.assertEqual(payload, {"ok": True}) + self.assertEqual(urlopen.call_count, 2) + + def test_a_timeout_is_retried_and_reported_as_an_api_error(self, _sleep): + """``TimeoutError`` is not a ``URLError``, so it once escaped as a traceback.""" + error, urlopen = self.get([TimeoutError("timed out")] * 3) + self.assertEqual(urlopen.call_count, 3) + self.assertIsInstance(error, resolve.GitHubApiError) + self.assertIn("timed out", str(error)) + + def test_a_connection_reset_is_retried_until_it_succeeds(self, _sleep): + """urllib only converts errors raised while sending the request. + + A server that resets the connection after the request was sent raises + ``RemoteDisconnected`` from ``getresponse``, which is neither a + ``URLError`` nor a ``TimeoutError``; it once escaped as a traceback. + """ + payload, urlopen = self.get( + [http.client.RemoteDisconnected("closed"), response({"ok": True})] + ) + self.assertEqual(payload, {"ok": True}) + self.assertEqual(urlopen.call_count, 2) + + def test_a_persistent_reset_is_an_api_error_naming_the_request(self, _sleep): + error, urlopen = self.get([ConnectionResetError("reset")] * 3) + self.assertEqual(urlopen.call_count, 3) + self.assertIsInstance(error, resolve.GitHubApiError) + self.assertIn(f"GET {self.URL}", str(error)) + self.assertIn("ConnectionResetError", str(error)) + + def test_a_truncated_body_is_retried(self, _sleep): + """``IncompleteRead`` is raised while reading the body, after the status.""" + truncated = mock.MagicMock() + truncated.__enter__.return_value.read.side_effect = http.client.IncompleteRead( + b"{", 100 + ) + payload, urlopen = self.get([truncated, response({"ok": True})]) + self.assertEqual(payload, {"ok": True}) + self.assertEqual(urlopen.call_count, 2) + + def test_a_server_that_resets_after_the_request_is_an_api_error(self, _sleep): + """The real path: urllib's own handlers talk to a real socket. + + The default opener would route through any proxy in the environment + instead of the local server, so this goes through a direct one. + """ + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + listener.listen(5) + self.addCleanup(listener.close) + + def reset_every_request(): + while True: + try: + connection, _ = listener.accept() + except OSError: + return + connection.recv(65536) + connection.close() + + threading.Thread(target=reset_every_request, daemon=True).start() + url = f"http://127.0.0.1:{listener.getsockname()[1]}/pulls/42" + + with mock.patch("urllib.request.urlopen", side_effect=direct_urlopen): + with self.assertRaises(resolve.GitHubApiError) as raised: + resolve.github_get(url) + + self.assertIn(f"GET {url}", str(raised.exception)) + self.assertIn("RemoteDisconnected", str(raised.exception)) + + def test_a_malformed_url_fails_at_once_without_retries(self, sleep): + """``InvalidURL`` is an ``HTTPException``, but retrying cannot fix an input. + + A ``pr_number`` of ``14 105`` once slept 2 s and 4 s before failing as + a transient error, which reads like a flaky network. + """ + url = f"{API}/pulls/14 105" + + with mock.patch("urllib.request.urlopen", side_effect=direct_urlopen): + with self.assertRaises(resolve.GitHubApiError) as raised: + resolve.github_get(url) + + self.assertNotIsInstance(raised.exception, resolve.TransientGitHubError) + self.assertIn(f"GET {url} is not a valid URL", str(raised.exception)) + sleep.assert_not_called() + + def test_an_invalid_url_from_the_transport_is_not_retried(self, _sleep): + error, urlopen = self.get([http.client.InvalidURL("bad")]) + self.assertNotIsInstance(error, resolve.TransientGitHubError) + self.assertEqual(urlopen.call_count, 1) + + def test_an_unreadable_body_is_an_api_error(self, _sleep): + body = mock.MagicMock() + body.__enter__.return_value = io.BytesIO(b"") + error, _ = self.get([body]) + self.assertIsInstance(error, resolve.GitHubApiError) + self.assertIn("invalid JSON", str(error)) + + def test_a_missing_token_fails_before_any_request(self, _sleep): + with mock.patch.dict(os.environ, {"GITHUB_TOKEN": ""}): + error, urlopen = self.get([response({})]) + self.assertIn("GITHUB_TOKEN is not set", str(error)) + urlopen.assert_not_called() + + +class HarnessProbeTest(unittest.TestCase): + def harness(self, **sources): + root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + for relative, text in sources.items(): + path = root / "coco" / "cli" / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return root + + def test_a_harness_declaring_the_option_supports_it(self): + harness = self.harness( + **{"commands/pr.py": '@click.option(\n "--baseline-sha",'} + ) + self.assertTrue(resolve.harness_supports_baseline_sha(harness)) + + def test_single_quotes_and_a_multi_line_declaration_are_found(self): + harness = self.harness( + **{ + "commands/pr.py": "@click.option(\n '--baseline-sha',\n default='',\n)" + } + ) + self.assertTrue(resolve.harness_supports_baseline_sha(harness)) + + def test_an_option_moved_to_a_shared_module_is_found(self): + harness = self.harness( + **{"commands/pr.py": "", "options.py": 'BASELINE = "--baseline-sha"'} + ) + self.assertTrue(resolve.harness_supports_baseline_sha(harness)) + + def test_a_harness_with_only_baseline_stack_does_not_support_it(self): + """What ``coco`` 1.3.x declares: a neighbouring option must not match.""" + harness = self.harness( + **{"commands/pr.py": '"--baseline-stack", "--skip-baselines"'} + ) + self.assertFalse(resolve.harness_supports_baseline_sha(harness)) + + def test_a_directory_without_a_cli_does_not_support_it(self): + self.assertFalse(resolve.harness_supports_baseline_sha(self.harness())) + + +class ResolveBaselineShaTest(unittest.TestCase): + def _harness(self, pr_command): + root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + path = root / "coco" / "cli" / "commands" / "pr.py" + path.parent.mkdir(parents=True) + path.write_text(pr_command, encoding="utf-8") + return root + + def resolve(self, pr_number="42", pr_command='"--baseline-sha"', require=False): + return resolve.resolve_baseline_sha( + pr_number=pr_number, + pr_sha=HEAD, + pr_repo="ROCm/rocm-systems", + harness_dir=self._harness(pr_command), + require=require, + get=fake_github(), + ) + + def test_periodic_runs_keep_the_configured_baseline(self): + self.assertEqual(self.resolve(pr_number=""), "") + + def test_a_pr_run_pins_to_the_merge_base_when_the_harness_can(self): + self.assertEqual(self.resolve(), MERGE_BASE) + + def test_an_older_harness_is_not_passed_a_flag_it_would_reject(self): + """The harness ref (1.3.x) may predate --baseline-sha; passing it would fail every gate.""" + out = io.StringIO() + with contextlib.redirect_stdout(out): + sha = self.resolve(pr_command='"--skip-baselines"') + self.assertEqual(sha, "") + self.assertIn("::warning::", out.getvalue()) + + def test_a_required_pin_fails_on_a_harness_without_the_flag(self): + """Once the gate runs a harness that has the flag, losing it must not unpin quietly.""" + with self.assertRaisesRegex(ValueError, "requires the PR baseline"): + self.resolve(pr_command='"--skip-baselines"', require=True) + + def test_a_required_pin_still_pins_when_the_harness_can(self): + self.assertEqual(self.resolve(require=True), MERGE_BASE) + + +class MainTest(unittest.TestCase): + """``main`` reads its inputs from the environment, as the composite action sets them.""" + + def run_main(self, **env): + outputs = {} + completed = subprocess.CompletedProcess([], 0, stdout="cafe\n") + out = io.StringIO() + with ( + mock.patch.dict(os.environ, env, clear=True), + mock.patch.object(resolve, "set_github_output", outputs.update), + mock.patch.object(resolve.subprocess, "run", return_value=completed), + contextlib.redirect_stdout(out), + ): + try: + resolve.main() + code = 0 + except SystemExit as exc: + code = exc.code + return code, outputs, out.getvalue() + + def harness(self, pr_command): + root = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + path = root / "coco" / "cli" / "commands" / "pr.py" + path.parent.mkdir(parents=True) + path.write_text(pr_command, encoding="utf-8") + return os.fspath(root) + + def test_a_missing_harness_dir_fails_instead_of_silently_unpinning(self): + """The probe once read ``Path.cwd()``; a dropped working-directory unpinned every run.""" + code, outputs, out = self.run_main(PR_NUMBER="42", PR_SHA=HEAD) + self.assertEqual(code, 1) + self.assertEqual(outputs, {}) + self.assertIn("::error::HARNESS_DIR is not set", out) + + def test_a_periodic_run_needs_no_api_call(self): + code, outputs, _ = self.run_main( + HARNESS_DIR=self.harness(""), + EVENT_NAME="workflow_dispatch", + PERIODIC_MODE="nightly", + ) + self.assertEqual(code, 0) + self.assertEqual(outputs["mode"], "nightly") + self.assertNotIn("--baseline-sha", outputs["coco_args"]) + + def test_a_required_pin_on_an_old_harness_fails_the_setup(self): + code, outputs, out = self.run_main( + HARNESS_DIR=self.harness('"--skip-baselines"'), + PR_NUMBER="42", + PR_SHA=HEAD, + PR_REPO="ROCm/rocm-systems", + REQUIRE_BASELINE_SHA="true", + ) + self.assertEqual(code, 1) + self.assertEqual(outputs, {}) + self.assertIn("::error::This coco harness has no --baseline-sha", out) + + def test_an_api_failure_fails_the_setup_with_the_request_named(self): + """End to end: a missing PR ends setup with the request in the annotation.""" + with mock.patch("urllib.request.urlopen", side_effect=[http_error(404)]): + code, outputs, out = self.run_main( + HARNESS_DIR=self.harness('"--baseline-sha"'), + PR_NUMBER="42", + PR_SHA=HEAD, + PR_REPO="ROCm/rocm-systems", + GITHUB_TOKEN="test-token", + ) + self.assertEqual(code, 1) + self.assertEqual(outputs, {}) + self.assertIn(f"::error::GET {API}/pulls/42 failed: HTTP 404", out) + + if __name__ == "__main__": unittest.main() From 683f8ca15d533e4ba22b14c7adfdf6110dcdfbfa Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Thu, 29 Oct 2026 10:57:43 -0400 Subject: [PATCH 2/2] ci(rccl): require the PR baseline pin in the coco PR gate The coco PR gate now runs the 1.4.x harness, which declares --baseline-sha. Require the pin, so a harness that loses the flag fails setup instead of measuring against develop tip behind a warning. Rolling coco_ref back to 1.3.x must turn the requirement off as well. Signed-off-by: speriaswamy-amd --- .../tests/rccl_resolve_coco_run_test.py | 29 +++++++++++++++++++ .../rccl-cluster-runner-allowlist.yml | 2 ++ .github/workflows/rccl-coco-pr.yml | 3 ++ 3 files changed, 34 insertions(+) diff --git a/.github/scripts/tests/rccl_resolve_coco_run_test.py b/.github/scripts/tests/rccl_resolve_coco_run_test.py index 0a8781d554cc..5c2f53d2a1f6 100644 --- a/.github/scripts/tests/rccl_resolve_coco_run_test.py +++ b/.github/scripts/tests/rccl_resolve_coco_run_test.py @@ -39,6 +39,35 @@ def test_workflow_crons_are_all_recognised(self): self.assertEqual(crons, {resolve.WEEKDAY_NIGHTLY_CRON, resolve.SATURDAY_CRON}) +class BaselinePinWiringTest(unittest.TestCase): + """The PR workflow must reach the action's ``require_baseline_sha`` input. + + GitHub only warns about an input the action does not declare, so a + misspelt name would silently turn the requirement off. + """ + + WORKFLOWS = Path(__file__).resolve().parents[2] / "workflows" + ACTION = ( + Path(__file__).resolve().parents[2] + / "actions" + / "rccl-resolve-coco-run" + / "action.yml" + ) + + def test_the_action_declares_the_input_and_defaults_it_off(self): + text = self.ACTION.read_text(encoding="utf-8") + self.assertRegex( + text, + r"(?m)^ require_baseline_sha:\n(?: .*\n)*? default: 'false'\n", + ) + self.assertIn("REQUIRE_BASELINE_SHA: ${{ inputs.require_baseline_sha }}", text) + + def test_the_pr_workflow_requires_the_pin(self): + """Only PR runs pin the baseline, so only this workflow needs to require it.""" + text = (self.WORKFLOWS / "rccl-coco-pr.yml").read_text(encoding="utf-8") + self.assertRegex(text, r"(?m)^\s+require_baseline_sha: 'true'$") + + class ScheduleToModeTest(unittest.TestCase): def test_weekday_cron_is_nightly(self): self.assertEqual( diff --git a/.github/workflows/rccl-cluster-runner-allowlist.yml b/.github/workflows/rccl-cluster-runner-allowlist.yml index e1b0aafec6e5..5d2bdc79e491 100644 --- a/.github/workflows/rccl-cluster-runner-allowlist.yml +++ b/.github/workflows/rccl-cluster-runner-allowlist.yml @@ -11,6 +11,7 @@ on: paths: - '.github/workflows/**' - '.github/scripts/**' + - '.github/actions/rccl-resolve-coco-run/**' workflow_dispatch: permissions: @@ -37,6 +38,7 @@ jobs: sparse-checkout: | .github/workflows .github/scripts + .github/actions/rccl-resolve-coco-run - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: diff --git a/.github/workflows/rccl-coco-pr.yml b/.github/workflows/rccl-coco-pr.yml index d3c836b06681..6086137dd24f 100644 --- a/.github/workflows/rccl-coco-pr.yml +++ b/.github/workflows/rccl-coco-pr.yml @@ -119,6 +119,9 @@ jobs: pr_number: ${{ inputs.pr_number }} pr_sha: ${{ inputs.pr_sha }} pr_repo: ${{ inputs.pr_repo }} + # The harness must declare --baseline-sha (1.4.x and later); rolling + # coco_ref back to 1.3.x needs this turned off as well. + require_baseline_sha: 'true' client-id: ${{ secrets.APP_ID }} app-private-key: ${{ secrets.APP_PRIVATE_KEY }}