diff --git a/.github/workflows/run-ci-command.yml b/.github/workflows/run-ci-command.yml index 747831c7d4..3435148ea4 100644 --- a/.github/workflows/run-ci-command.yml +++ b/.github/workflows/run-ci-command.yml @@ -21,11 +21,17 @@ jobs: (github.event.comment.body == '/ci run' || github.event.comment.body == '/ci run all' || github.event.comment.body == '/ci run nightly' || + github.event.comment.body == '/ci run --allow-stale' || + github.event.comment.body == '/ci run all --allow-stale' || + github.event.comment.body == '/ci run nightly --allow-stale' || github.event.comment.body == '/ci retry' || github.event.comment.body == '/ci cancel' || github.event.comment.body == '/amd-ci run' || github.event.comment.body == '/amd-ci run all' || github.event.comment.body == '/amd-ci run nightly' || + github.event.comment.body == '/amd-ci run --allow-stale' || + github.event.comment.body == '/amd-ci run all --allow-stale' || + github.event.comment.body == '/amd-ci run nightly --allow-stale' || github.event.comment.body == '/amd-ci retry' || github.event.comment.body == '/amd-ci cancel') runs-on: [self-hosted, linux, x64, vllm-runners] diff --git a/.github/workflows/scripts/run_ci_command.py b/.github/workflows/scripts/run_ci_command.py index 6454f041ff..530bd697d0 100644 --- a/.github/workflows/scripts/run_ci_command.py +++ b/.github/workflows/scripts/run_ci_command.py @@ -1,6 +1,7 @@ # SPDX-License-Identifier: Apache-2.0 # SPDX-FileCopyrightText: Copyright contributors to the vLLM project +import http.client import json import os import random @@ -22,6 +23,7 @@ COMMAND_RUN_AMD_CI_ALL = "/amd-ci run all" COMMAND_RUN_AMD_CI_NIGHTLY = "/amd-ci run nightly" COMMAND_RETRY_AMD_FAILED = "/amd-ci retry" COMMAND_CANCEL_AMD_CI = "/amd-ci cancel" +ALLOW_STALE_SUFFIX = " --allow-stale" RUN_CI_COMMAND_ENV = { COMMAND_RUN_CI: {}, COMMAND_RUN_CI_ALL: {"RUN_ALL": "1"}, @@ -30,20 +32,25 @@ RUN_CI_COMMAND_ENV = { COMMAND_RUN_AMD_CI_ALL: {"RUN_ALL": "1"}, COMMAND_RUN_AMD_CI_NIGHTLY: {"RUN_ALL": "1", "NIGHTLY": "1"}, } +ALLOW_STALE_COMMANDS = frozenset( + f"{command}{ALLOW_STALE_SUFFIX}" for command in RUN_CI_COMMAND_ENV +) +RUN_CI_COMMAND_ENV.update( + { + command: RUN_CI_COMMAND_ENV[command.removesuffix(ALLOW_STALE_SUFFIX)] + for command in ALLOW_STALE_COMMANDS + } +) UPSTREAM_CI_COMMANDS = frozenset( { - COMMAND_RUN_CI, - COMMAND_RUN_CI_ALL, - COMMAND_RUN_CI_NIGHTLY, + *(command for command in RUN_CI_COMMAND_ENV if command.startswith("/ci ")), COMMAND_RETRY_FAILED, COMMAND_CANCEL_CI, } ) AMD_CI_COMMANDS = frozenset( { - COMMAND_RUN_AMD_CI, - COMMAND_RUN_AMD_CI_ALL, - COMMAND_RUN_AMD_CI_NIGHTLY, + *(command for command in RUN_CI_COMMAND_ENV if command.startswith("/amd-ci ")), COMMAND_RETRY_AMD_FAILED, COMMAND_CANCEL_AMD_CI, } @@ -79,6 +86,10 @@ class ApiError(RuntimeError): self.status = status +class CiPreparationError(RuntimeError): + pass + + def rate_limit_jitter() -> float: return random.uniform(1, 5) @@ -137,6 +148,8 @@ class HttpTransport: None, f"API request failed: {error.reason}", ) from error + except (OSError, http.client.HTTPException) as error: + raise ApiError(None, f"API request failed: {error}") from error if not response_body: return None @@ -234,6 +247,17 @@ class GitHubClient: def get_pr(self, number: int) -> dict[str, Any]: return self._request(self._repo_path(f"/pulls/{number}")) + def get_commits_behind_base(self, base_ref: str, head_sha: str) -> int: + base_ref = urllib.parse.quote(f"refs/heads/{base_ref}", safe="") + head_sha = urllib.parse.quote(head_sha, safe="") + response = self._request( + self._repo_path(f"/compare/{base_ref}...{head_sha}?per_page=1") + ) + behind = response.get("behind_by") if isinstance(response, dict) else None + if type(behind) is not int or behind < 0: + raise ApiError(None, "GitHub returned an invalid commits-behind count.") + return behind + def list_pulls_for_commit(self, commit: str) -> list[dict[str, Any]]: commit = urllib.parse.quote(commit, safe="") response = self._request(self._repo_path(f"/commits/{commit}/pulls")) @@ -747,6 +771,10 @@ def notify_authorized( ( f"✅ @{author}, CI is now available for this PR.\n\n" "- `/ci run` starts upstream CI; `/amd-ci run` starts AMD CI only.\n" + "- Your branch must contain every commit currently on its upstream " + "target branch. Merge or rebase onto the latest target branch, then " + "rerun the command. Append `--allow-stale` to a run command to test " + "an outdated branch at your own risk.\n" "- `/ci retry` retries failed jobs in the CI build for the current " "PR head. If the current head has no CI build, it starts a new CI " "build for the current head containing only jobs that failed in " @@ -794,6 +822,56 @@ def resolve_workflow_run_pr( return find_matching_pr(github.list_pulls_for_commit(head_sha)) +def prepare_pr_for_ci( + github: GitHubClient, + pr: Mapping[str, Any], + command: str, +) -> tuple[dict[str, Any], str]: + base_ref = pr["base"]["ref"] + no_action = "No new CI build was started." + try: + behind = github.get_commits_behind_base(base_ref, pr["head"]["sha"]) + current_pr = github.get_pr(pr["number"]) + except ApiError as error: + raise CiPreparationError( + f"Could not check the PR against upstream `{base_ref}`. " + f"{no_action} Comment `{command}` again. {error}" + ) from error + if ( + current_pr["state"] != "open" + or current_pr["head"]["sha"] != pr["head"]["sha"] + or current_pr["base"]["ref"] != pr["base"]["ref"] + ): + raise CiPreparationError( + f"The PR changed while checking its branch. {no_action} " + f"Comment `{command}` again." + ) + commits = "commit" if behind == 1 else "commits" + lag = f"This PR is {behind} {commits} behind upstream `{base_ref}`." + if command in ALLOW_STALE_COMMANDS: + warning = "" + if behind: + run_command = command.removesuffix(ALLOW_STALE_SUFFIX) + warning = ( + f"⚠️ {lag} Running CI at your own risk because " + "`--allow-stale` was requested; outdated CI configuration may " + "cause failures. Before merging, merge or rebase onto the latest " + f"`{base_ref}`, then rerun `{run_command}` on the latest PR commit." + ) + print(warning) + return current_pr, warning + + if behind: + raise CiPreparationError( + f"{lag} Your branch must contain every commit currently on upstream " + f"`{base_ref}`. {no_action} " + f"Merge or rebase onto the latest `{base_ref}`, then rerun `{command}`." + " To test this branch at your own risk, " + f"use `{command}{ALLOW_STALE_SUFFIX}`." + ) + return current_pr, "" + + def handle_run_ci( *, actor: str, @@ -828,12 +906,7 @@ def handle_run_ci( f"{ci_name} is already running for this commit: {active_build['web_url']}" ) - current_pr = github.get_pr(pr["number"]) - if current_pr["state"] != "open" or current_pr["head"]["sha"] != pr["head"]["sha"]: - return ( - "The PR head changed while processing the command. " - f"Comment `{command}` again." - ) + current_pr, warning = prepare_pr_for_ci(github, pr, command) build = buildkite.create_build( create_build_payload( @@ -846,6 +919,7 @@ def handle_run_ci( return ( f"Triggered [Buildkite {ci_name} #{build['number']}]({build['web_url']}) " f"for commit `{current_pr['head']['sha'][:12]}`." + + (f"\n\n{warning}" if warning else "") ) @@ -1099,6 +1173,12 @@ def run( raise ValueError(f"Unsupported CI command: {command}") add_reaction_safely(github, comment_id, "rocket") github.add_comment(issue_number, f"✅ {message}") + except CiPreparationError as error: + add_reaction_safely(github, comment_id, "-1") + github.add_comment( + issue_number, + f"❌ {error}\n\n{command_comment_marker(comment_id)}", + ) except Exception: add_reaction_safely(github, comment_id, "confused") raise diff --git a/.github/workflows/scripts/test_run_ci_command.py b/.github/workflows/scripts/test_run_ci_command.py index ef93ea8559..9750cefaa3 100644 --- a/.github/workflows/scripts/test_run_ci_command.py +++ b/.github/workflows/scripts/test_run_ci_command.py @@ -22,8 +22,10 @@ from run_ci_command import ( COMMAND_RUN_CI_ALL, COMMAND_RUN_CI_NIGHTLY, RETRY_STATES, + RUN_CI_COMMAND_ENV, ApiError, BuildkiteClient, + GitHubClient, HttpTransport, authorize, create_build_payload, @@ -85,6 +87,7 @@ class FakeGitHub: comments: list[str] | None = None, pulls_for_commit: list[dict[str, Any]] | None = None, prs: dict[int, dict[str, Any]] | None = None, + behind: int = 0, ) -> None: self.comments = comments or [] self.permission = permission @@ -95,6 +98,12 @@ class FakeGitHub: self.reviews = reviews or [] self.pulls_for_commit = pulls_for_commit or [] self.prs = prs or {self.pr["number"]: self.pr} + self.behind = behind + self.lag_queries: list[tuple[str, str]] = [] + + def get_commits_behind_base(self, base_ref: str, head_sha: str) -> int: + self.lag_queries.append((base_ref, head_sha)) + return self.behind def get_pr(self, number: int) -> dict[str, Any]: try: @@ -322,6 +331,11 @@ class RunCiCommandTest(unittest.TestCase): for command in commands: with self.subTest(command=command): self.assertEqual(parse_command(command), command) + override = f"{command} --allow-stale" + if " run" in command: + self.assertEqual(parse_command(override), override) + else: + self.assertIsNone(parse_command(override)) self.assertIsNone(parse_command("/ci run please")) self.assertIsNone(parse_command("/ci run all please")) @@ -331,6 +345,8 @@ class RunCiCommandTest(unittest.TestCase): self.assertIsNone(parse_command("/amd-ci retry ")) self.assertIsNone(parse_command("/AMD-CI run")) self.assertIsNone(parse_command("/amdci run")) + self.assertIsNone(parse_command("/ci run --allow-stale all")) + self.assertIsNone(parse_command("/amd-ci run --allow-stale please")) def test_commands_select_only_their_configured_pipeline(self) -> None: cases = ( @@ -530,6 +546,177 @@ class RunCiCommandTest(unittest.TestCase): self.assertTrue(github.comments[0].startswith("✅ ")) self.assertIn("Buildkite CI #123", github.comments[0]) + def test_run_variants_require_current_base_without_changing_commit(self) -> None: + for command in RUN_CI_COMMAND_ENV: + if command.endswith(" --allow-stale"): + continue + for behind in (0, 1, 5): + with self.subTest(command=command, behind=behind): + github = FakeGitHub( + behind=behind, pr=make_pr(base={"ref": "release/1.0"}) + ) + buildkite = FakeBuildkite([[], []]) + run(make_event(command), github, buildkite) + + self.assertEqual( + github.lag_queries, + [("release/1.0", github.pr["head"]["sha"])], + ) + if behind == 0: + self.assertEqual( + buildkite.created_builds[0]["commit"], + github.pr["head"]["sha"], + ) + self.assertIn( + "](https://buildkite.example/builds/123)", + github.comments[0], + ) + else: + self.assertEqual(buildkite.created_builds, []) + self.assertTrue(github.comments[0].startswith("❌ ")) + self.assertIn(str(behind), github.comments[0]) + self.assertIn("upstream `release/1.0`", github.comments[0]) + self.assertIn("every commit", github.comments[0]) + self.assertIn( + "No new CI build was started.", github.comments[0] + ) + self.assertIn(command, github.comments[0]) + + def test_allow_stale_preserves_author_permissions_and_run_configuration( + self, + ) -> None: + cases = ( + (COMMAND_RUN_CI, "ci", {}), + (COMMAND_RUN_CI_ALL, "ci", {"RUN_ALL": "1"}), + (COMMAND_RUN_CI_NIGHTLY, "ci", {"RUN_ALL": "1", "NIGHTLY": "1"}), + (COMMAND_RUN_AMD_CI, "amd-ci", {}), + (COMMAND_RUN_AMD_CI_ALL, "amd-ci", {"RUN_ALL": "1"}), + ( + COMMAND_RUN_AMD_CI_NIGHTLY, + "amd-ci", + {"RUN_ALL": "1", "NIGHTLY": "1"}, + ), + ) + for original_command, pipeline, expected_env in cases: + command = f"{original_command} --allow-stale" + for ready, behind in ((False, 100), (True, 0), (True, 100)): + with self.subTest(command=command, ready=ready, behind=behind): + github = FakeGitHub( + permission="read", + behind=behind, + pr=make_pr( + base={"ref": "feature/stack"}, + labels=[{"name": "ready"}] if ready else [], + ), + ) + buildkite = FakeBuildkite([[], []]) + + run(make_event(command, "author"), github, buildkite) + + self.assertEqual(pipeline_for_command(command), pipeline) + if not ready: + self.assertEqual(buildkite.created_builds, []) + self.assertEqual(buildkite.list_calls, []) + self.assertEqual(github.lag_queries, []) + self.assertTrue(github.comments[0].startswith("❌ ")) + continue + self.assertEqual(len(buildkite.created_builds), 1) + payload = buildkite.created_builds[0] + self.assertEqual(payload["commit"], github.pr["head"]["sha"]) + self.assertEqual( + payload["pull_request_base_branch"], "feature/stack" + ) + self.assertEqual(payload["message"], f"PR #42 {command} by @author") + self.assertEqual( + payload["env"], + { + "VLLM_CI_GITHUB_COMMENT_ID": "99", + "VLLM_CI_TRIGGERED_BY": "author", + **expected_env, + }, + ) + self.assertEqual( + github.lag_queries, + [("feature/stack", github.pr["head"]["sha"])], + ) + self.assertIn( + "Buildkite AMD CI #123" + if pipeline == "amd-ci" + else "Buildkite CI #123", + github.comments[0], + ) + if behind: + self.assertIn("100 commits behind", github.comments[0]) + self.assertIn("upstream `feature/stack`", github.comments[0]) + self.assertIn("Before merging", github.comments[0]) + self.assertIn("latest PR commit", github.comments[0]) + self.assertIn(original_command, github.comments[0]) + + def test_base_lag_query_qualifies_branch_refs_and_validates_count(self) -> None: + for base_ref, encoded_ref in ( + ("main", "main"), + ("release/1.0", "release%2F1.0"), + ("feature/stack", "feature%2Fstack"), + ): + for count in (0, 5, None, -1, True, "5"): + with self.subTest(base_ref=base_ref, count=count): + transport = FakeTransport( + {} if count is None else {"behind_by": count} + ) + github = GitHubClient("token", "vllm-project/vllm", transport) + if type(count) is int and count >= 0: + self.assertEqual( + github.get_commits_behind_base(base_ref, "head-sha"), count + ) + else: + with self.assertRaises(ApiError): + github.get_commits_behind_base(base_ref, "head-sha") + self.assertEqual( + transport.calls[0]["url"], + "https://api.github.com/repos/vllm-project/vllm/compare/" + f"refs%2Fheads%2F{encoded_ref}...head-sha?per_page=1", + ) + self.assertEqual(transport.calls[0]["method"], "GET") + + def test_unknown_lag_or_changed_pr_prevents_runs(self) -> None: + for command in ( + COMMAND_RUN_CI, + COMMAND_RUN_AMD_CI, + f"{COMMAND_RUN_CI} --allow-stale", + f"{COMMAND_RUN_AMD_CI} --allow-stale", + ): + for fault in ("api", "head", "closed", "base"): + with self.subTest(command=command, fault=fault): + github = FakeGitHub() + buildkite = FakeBuildkite([[], []]) + + def check( + base_ref: str, + sha: str, + fault: str = fault, + github: FakeGitHub = github, + ) -> int: + if fault == "api": + raise ApiError(503, "Unavailable") + changed = make_pr() + if fault == "head": + changed["head"]["sha"] = "another-head" + elif fault == "closed": + changed["state"] = "closed" + else: + changed["base"]["ref"] = "another-base" + github.prs[42] = changed + return 0 + + with patch.object( + github, "get_commits_behind_base", side_effect=check + ): + run(make_event(command), github, buildkite) + self.assertEqual(buildkite.created_builds, []) + self.assertEqual(buildkite.retry_calls, []) + self.assertTrue(github.comments[0].startswith("❌ ")) + self.assertNotIn("Triggered", github.comments[0]) + def test_amd_run_ignores_blocked_builds(self) -> None: for metadata in ({}, {"github-comment-id": "98"}): with self.subTest(metadata=metadata): @@ -627,6 +814,7 @@ class RunCiCommandTest(unittest.TestCase): run(make_event(COMMAND_RUN_CI, "author"), github, buildkite) self.assertEqual(buildkite.list_calls, []) + self.assertEqual(github.lag_queries, []) self.assertEqual(github.reactions, ["eyes"]) self.assertTrue(github.comments[0].startswith("❌ ")) self.assertIn("approve the PR", github.comments[0]) @@ -848,6 +1036,7 @@ class RunCiCommandTest(unittest.TestCase): def test_ci_retry_retries_failed_jobs_while_build_is_running(self) -> None: github = FakeGitHub( + behind=100, permission="read", pr=make_pr(labels=[{"name": "ready"}]), ) @@ -867,10 +1056,12 @@ class RunCiCommandTest(unittest.TestCase): run(make_event(COMMAND_RETRY_FAILED, "author"), github, buildkite) self.assertEqual(buildkite.retry_calls, [(123, RETRY_STATES)]) + self.assertEqual(github.lag_queries, []) self.assertIn("Queued 3 failed job", github.comments[0]) def test_amd_ci_retry_retries_only_the_current_head_build(self) -> None: github = FakeGitHub( + behind=100, permission="read", pr=make_pr(labels=[{"name": "ready"}]), ) @@ -891,6 +1082,7 @@ class RunCiCommandTest(unittest.TestCase): run(make_event(COMMAND_RETRY_AMD_FAILED, "author"), github, buildkite) self.assertEqual(buildkite.retry_calls, [(321, RETRY_STATES)]) + self.assertEqual(github.lag_queries, []) self.assertIn("Buildkite AMD CI #321", github.comments[0]) def test_amd_ci_retry_requires_a_build_for_the_current_head(self) -> None: @@ -912,6 +1104,7 @@ class RunCiCommandTest(unittest.TestCase): def test_ci_retry_creates_filtered_build_for_new_head(self) -> None: github = FakeGitHub( + behind=100, permission="read", pr=make_pr(labels=[{"name": "ready"}]), ) @@ -953,6 +1146,7 @@ class RunCiCommandTest(unittest.TestCase): self.assertEqual(len(buildkite.created_builds), 1) payload = buildkite.created_builds[0] self.assertEqual(payload["commit"], "0123456789abcdef") + self.assertEqual(github.lag_queries, []) self.assertEqual(payload["message"], "PR #42 /ci retry by @author") self.assertEqual( json.loads(payload["env"]["VLLM_CI_ONLY_STEP_KEYS"]), @@ -1104,6 +1298,7 @@ class RunCiCommandTest(unittest.TestCase): run(make_event(COMMAND_CANCEL_CI), github, buildkite) self.assertEqual(buildkite.cancel_calls, [123, 124, 126]) + self.assertEqual(github.lag_queries, []) self.assertEqual( [request["branch"] for request in buildkite.list_requests], ["contributor:feature", "feature"], @@ -1152,6 +1347,7 @@ class RunCiCommandTest(unittest.TestCase): run(make_event(COMMAND_CANCEL_AMD_CI), github, buildkite) self.assertEqual(buildkite.cancel_calls, [322, 321]) + self.assertEqual(github.lag_queries, []) self.assertEqual( [request["branch"] for request in buildkite.list_requests], ["contributor:feature", "feature"], diff --git a/docs/contributing/README.md b/docs/contributing/README.md index 55966f2854..7a1af9c1a7 100644 --- a/docs/contributing/README.md +++ b/docs/contributing/README.md @@ -315,6 +315,34 @@ review process: the `ready` label, the PR author can use `/ci run`, `/ci retry`, `/ci cancel`, or the corresponding `/amd-ci` variants. New commits do not start upstream CI automatically. +- Before creating a Buildkite build, `/ci run` and `/amd-ci run`, including + `all` and `nightly`, check the requested PR commit against its target branch + in the upstream repository. Your branch must contain every commit currently + on that branch: **zero commits behind**. This also applies to PRs targeting + release branches or another PR's branch. +- If the PR is behind, the bot reports the count without creating a new build. + Merge or rebase onto the latest target branch, then rerun the command. If the + target branch advances in the meantime, update your branch and try again. +- To run CI on an outdated branch at your own risk, append `--allow-stale` to + any run command, for example `/ci run --allow-stale` or + `/amd-ci run all --allow-stale`. The same authorization requirements apply. + The bot reports the lag and warns that outdated CI configuration may cause + failures. Before merging, merge or rebase onto the latest target branch and + rerun CI without `--allow-stale` on the latest PR commit. +- Retrying or cancelling builds does not check branch freshness. +- These commands do not modify your branch or enforce merge requirements. + Direct Buildkite launches are outside this workflow. No additional token is + needed. + +Maintainers must configure required Buildkite status checks on the latest PR +commit and enable GitHub's +[Require branches to be up to date before merging](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging) +protection on the relevant target branches to enforce the merge policy. Select +statuses that confirm the required tests completed, using Buildkite as their +expected source. An aggregate status that reports "passed and blocked" does not +by itself guarantee those tests ran; verify the pipeline's required and optional +blocked steps before choosing statuses or changing blocked-build reporting. +The CI command workflow does not configure these repository or pipeline settings. ### Pull Request Limits and Escalation