diff --git a/docs/Architecture.md b/docs/Architecture.md index c78b665..6184447 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -888,7 +888,9 @@ else) towards better changes. - **A work item per PR,** scoped to its repository (L1). It closes when the PR merges or closes. - **What starts a run:** the PR becoming ready, a new push, or a new comment or - review on it. + review on it. A new head's review waits for its checks to finish (passed or + failed), so it is judged with the CI result and stays off a GPU that CI is + using; up to a tuning limit, and not at all where there is no CI. - **The first review** is given the PR (title, description, its commits and diff, its comments, and the issue it closes); the reviewer reads the rest as it needs it (R3). diff --git a/docs/Tuning-Guide.md b/docs/Tuning-Guide.md index aca2cbc..38c4532 100644 --- a/docs/Tuning-Guide.md +++ b/docs/Tuning-Guide.md @@ -638,3 +638,25 @@ reads file by file. Failed review runs on one PR, at one head, before the reviewer says on the PR that it couldn't review it, and stops trying until the next push. + +### `tuning.review.ci_wait_s` — default 3600 + +How long a new head's review waits while its checks are pending. A review +judges the CI result (R1), and waiting keeps it off a GPU that CI is using +(the `nisse` project's end-to-end tests). Finished checks, passed or failed, +end the wait; after this long it reviews anyway and the CI result reads as +pending. + +- **Too low:** reviews run beside CI, slower on a shared GPU, without its result. +- **Too high:** a stuck runner delays reviews that long. +- **Deciding:** a little longer than the repository's slowest CI run (the full + end-to-end suite takes 25 minutes or more). + +### `tuning.review.ci_grace_s` — default 120 + +How long a new head with no checks at all waits for some to appear: Gitea +Actions reports a push's checks a moment after it. Past this, a repository +without CI is reviewed at once. + +- **Too low:** a review starts before CI has reported, and runs beside it. +- **Too high:** repositories without CI wait that long for every review. diff --git a/nisse-review/src/nisse_review/build.py b/nisse-review/src/nisse_review/build.py index 485da5e..b8d153c 100644 --- a/nisse-review/src/nisse_review/build.py +++ b/nisse-review/src/nisse_review/build.py @@ -142,6 +142,8 @@ def build( scratch=Path(scratch), poll_s=tuning.review.poll_s, attempts=tuning.review.attempts, + ci_wait_s=tuning.review.ci_wait_s, + ci_grace_s=tuning.review.ci_grace_s, patterns=config.repositories, ) return Built(scheduler, account, repos, store, gitea) diff --git a/nisse-review/src/nisse_review/config.py b/nisse-review/src/nisse_review/config.py index 011f392..e69bd5f 100644 --- a/nisse-review/src/nisse_review/config.py +++ b/nisse-review/src/nisse_review/config.py @@ -55,6 +55,10 @@ class ReviewTuning: max_turns: int = 40 # a review run's turn cap diff_chars: int = 60000 # the diff in the review's context; files on demand attempts: int = 2 # failed runs on one PR before saying so on it + # A new head waits for its CI to finish: at most ci_wait_s while checks + # are pending, and ci_grace_s for checks to appear at all. + ci_wait_s: float = 3600.0 + ci_grace_s: float = 120.0 @dataclass(frozen=True) diff --git a/nisse-review/src/nisse_review/scheduler.py b/nisse-review/src/nisse_review/scheduler.py index d51aee0..61c1861 100644 --- a/nisse-review/src/nisse_review/scheduler.py +++ b/nisse-review/src/nisse_review/scheduler.py @@ -14,6 +14,8 @@ item waits rather than risk a second review. import re import tempfile import threading +import time +from collections.abc import Callable from pathlib import Path from nisse_core.gitea import GiteaClient, GiteaError, PullRequest @@ -56,6 +58,9 @@ class Scheduler: poll_s: float = 60.0, attempts: int = 2, patterns: frozenset[str] | None = None, + ci_wait_s: float = 3600.0, + ci_grace_s: float = 120.0, + clock: Callable[[], float] = time.time, ) -> None: self._store = store self._gitea = gitea @@ -66,6 +71,10 @@ class Scheduler: self._attempts = attempts # The repositories reviewed, as patterns; None: every one it was given. self._patterns = patterns + self._ci_wait_s = ci_wait_s + self._ci_grace_s = ci_grace_s + self._clock = clock + self._said_waiting: set[str] = set() # "item@head", logged once self._stop = threading.Event() # -- the loop --------------------------------------------------------------- @@ -131,13 +140,21 @@ class Scheduler: title=pr.title, scope=repo, position=position, - data=data(PR(repo, pr.number, head=pr.head_sha)), + data=data(PR(repo, pr.number, head=pr.head_sha, seen_at=self._clock())), cause="ready for review" if position == READY else "not ready", ) return known = pr_of(item) moved = known.head != pr.head_sha - new = PR(known.repo, known.number, pr.head_sha, known.reviewed, known.attempts) + seen_at = self._clock() if moved else known.seen_at + new = PR( + known.repo, + known.number, + pr.head_sha, + known.reviewed, + known.attempts, + seen_at, + ) if item.position in (READY, WAITING): if position != item.position or moved: self._store.update_work_item( @@ -145,7 +162,7 @@ class Scheduler: ) elif item.position == FAILED and moved: # It never got a review: new code is worth another try. - new = PR(known.repo, known.number, pr.head_sha, known.reviewed, 0) + new = PR(known.repo, known.number, pr.head_sha, known.reviewed, 0, seen_at) self._store.update_work_item( item.id, position=position, data=data(new), cause="new push" ) @@ -165,8 +182,35 @@ class Scheduler: ) def _next(self) -> WorkItem | None: + """The oldest ready item whose CI has settled.""" ready_items = [i for i in self._store.work_items("open") if i.position == READY] - return min(ready_items, key=lambda i: i.opened_at, default=None) + for item in sorted(ready_items, key=lambda i: i.opened_at): + if self._ci_settled(item): + return item + return None + + def _ci_settled(self, item: WorkItem) -> bool: + """R1: a review judges the CI result, so a new head waits for its + checks: reviewed once they've finished (passed or not), at once with + no CI at all after a short grace for checks to appear, and anyway + once they've been pending ``ci_wait_s``. Waiting also keeps reviews + off a GPU that CI is using.""" + known = pr_of(item) + waited = self._clock() - known.seen_at + try: + state = self._gitea.checks(known.repo, known.head) + except GiteaError as exc: + self._oplog.emit("review.checks_unknown", cause=str(exc)[:300]) + return False # asked again next round + if state == "pending" and waited < self._ci_wait_s: + key = f"{item.id}@{known.head}" + if key not in self._said_waiting: + self._said_waiting.add(key) + self._oplog.emit( + "review.waiting_for_ci", cause="checks pending", work_item=item.id + ) + return False + return not (state == "none" and waited < self._ci_grace_s) # -- a review --------------------------------------------------------------- diff --git a/nisse-review/src/nisse_review/state.py b/nisse-review/src/nisse_review/state.py index e3e10ee..19487d2 100644 --- a/nisse-review/src/nisse_review/state.py +++ b/nisse-review/src/nisse_review/state.py @@ -33,6 +33,7 @@ class PR: head: str = "" # the commit last seen at the PR's head reviewed: str = "" # the commit its review was posted on attempts: int = 0 # failed runs at this head + seen_at: float = 0.0 # when this head was first seen (the CI wait) def item_id(repo: str, number: int) -> str: diff --git a/nisse-review/tests/review_fakes.py b/nisse-review/tests/review_fakes.py index 5014320..9df8de3 100644 --- a/nisse-review/tests/review_fakes.py +++ b/nisse-review/tests/review_fakes.py @@ -73,6 +73,7 @@ class FakeGitea: # Readable but not given (public, no grant): missing from /user/repos. public: set[str] = field(default_factory=set) unlisted: set[str] = field(default_factory=set) # its PRs can't be listed + ci: dict[str, str] = field(default_factory=dict) # head -> checks; success def _up(self) -> None: if self.down: @@ -124,7 +125,7 @@ class FakeGitea: return [] def checks(self, repo: str, sha: str) -> str: - return "success" + return self.ci.get(sha, "success") def archive(self, repo: str, ref: str) -> bytes: return archive(self.files) diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index b5fa65e..8785ff4 100644 --- a/nisse-review/tests/test_review.py +++ b/nisse-review/tests/test_review.py @@ -342,6 +342,58 @@ def test_a_repository_whose_prs_cannot_be_listed_holds_up_no_other( assert '"intake.repo_failed"' in out.getvalue() and "a/empty" in out.getvalue() +class Clock: + def __init__(self) -> None: + self.now = 1000.0 + + def __call__(self) -> float: + return self.now + + +def test_a_new_head_waits_for_its_ci_to_finish(tmp_path: Path) -> None: + # Follow-up to #154: judged with CI's result, and off CI's GPU. + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + head = pr().head_sha + gitea.ci[head] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock) + assert w["scheduler"].round() is False # pending: not yet + clock.now += 600 + assert w["scheduler"].round() is False + gitea.ci[head] = "failure" # finished: failing CI is worth a review too + assert w["scheduler"].round() is True + assert item(w).position == REVIEWED + + +def test_ci_pending_too_long_is_reviewed_anyway(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + gitea.ci[pr().head_sha] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock, ci_wait_s=3600) + assert w["scheduler"].round() is False + clock.now += 3600 + assert w["scheduler"].round() is True # a stuck runner blocks nothing + + +def test_no_ci_waits_only_a_grace_for_checks_to_appear(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr()]}) + gitea.ci[pr().head_sha] = "none" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock, ci_grace_s=120) + assert w["scheduler"].round() is False + clock.now += 120 + assert w["scheduler"].round() is True + + +def test_a_pr_waiting_on_ci_holds_up_no_other(tmp_path: Path) -> None: + clock = Clock() + gitea = FakeGitea(repos={REPO: [pr(7), pr(8, head_sha="c8")]}) + gitea.ci[pr(7).head_sha] = "pending" + w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock) + assert w["scheduler"].round() is True # 8, though 7 is older + assert (item(w, 7).position, item(w, 8).position) == (READY, REVIEWED) + + def test_a_configured_repository_it_cannot_read_is_left_out() -> None: gitea = FakeGitea(repos={REPO: []}) out = io.StringIO()