Tuning: the CI waits may be zero
The live review test sets ci_grace_s: 0, which the shared tuning parser
refused (every number had to be positive). A field now opts in to zero
with metadata {"zero": True}; ci_wait_s and ci_grace_s do.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
This commit is contained in:
1 parent
096eeea56b
commit
59fba3bb76
5 files changed
+42
-9
No files matched your search
@@ -645,7 +645,7 @@ 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
|
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,
|
(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
|
end the wait; after this long it reviews anyway and the CI result reads as
|
||||||
pending.
|
pending. 0 never waits.
|
||||||
|
|
||||||
- **Too low:** reviews run beside CI, slower on a shared GPU, without its result.
|
- **Too low:** reviews run beside CI, slower on a shared GPU, without its result.
|
||||||
- **Too high:** a stuck runner delays reviews that long.
|
- **Too high:** a stuck runner delays reviews that long.
|
||||||
@@ -656,7 +656,8 @@ pending.
|
|||||||
|
|
||||||
How long a new head with no checks at all waits for some to appear: Gitea
|
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
|
Actions reports a push's checks a moment after it. Past this, a repository
|
||||||
without CI is reviewed at once.
|
without CI is reviewed at once. 0 reviews such a head at once (the live
|
||||||
|
review test, whose fresh repository has no CI).
|
||||||
|
|
||||||
- **Too low:** a review starts before CI has reported, and runs beside it.
|
- **Too low:** a review starts before CI has reported, and runs beside it.
|
||||||
- **Too high:** repositories without CI wait that long for every review.
|
- **Too high:** repositories without CI wait that long for every review.
|
||||||
@@ -81,13 +81,14 @@ def read_file(path: Path, errors: list[str]) -> dict[str, Any]:
|
|||||||
return loaded
|
return loaded
|
||||||
|
|
||||||
|
|
||||||
def positive(value: Any, kind: type) -> int | float | None:
|
def positive(value: Any, kind: type, *, zero: bool = False) -> int | float | None:
|
||||||
|
"""``value`` as ``kind`` if above 0 (or 0 itself, given ``zero``)."""
|
||||||
# bool is an int in Python; `true` for a turn limit is a mistake.
|
# bool is an int in Python; `true` for a turn limit is a mistake.
|
||||||
if isinstance(value, bool) or not isinstance(value, int | float):
|
if isinstance(value, bool) or not isinstance(value, int | float):
|
||||||
return None
|
return None
|
||||||
if kind is int and not float(value).is_integer():
|
if kind is int and not float(value).is_integer():
|
||||||
return None
|
return None
|
||||||
if value <= 0:
|
if value < 0 or (value == 0 and not zero):
|
||||||
return None
|
return None
|
||||||
return kind(value)
|
return kind(value)
|
||||||
|
|
||||||
@@ -132,9 +133,13 @@ def parse_tuning(
|
|||||||
if spec is None:
|
if spec is None:
|
||||||
errors.append(f"tuning.{name}.{key}: unknown setting")
|
errors.append(f"tuning.{name}.{key}: unknown setting")
|
||||||
continue
|
continue
|
||||||
number = positive(value, float if spec.type in (float, "float") else int)
|
# A field with metadata {"zero": True} may also be 0 ("don't wait").
|
||||||
|
zero = bool(spec.metadata.get("zero"))
|
||||||
|
kind = float if spec.type in (float, "float") else int
|
||||||
|
number = positive(value, kind, zero=zero)
|
||||||
if number is None:
|
if number is None:
|
||||||
errors.append(f"tuning.{name}.{key}: must be a positive number")
|
need = "zero or more" if zero else "a positive number"
|
||||||
|
errors.append(f"tuning.{name}.{key}: must be {need}")
|
||||||
continue
|
continue
|
||||||
values[key] = number
|
values[key] = number
|
||||||
sections[name] = cls(**values)
|
sections[name] = cls(**values)
|
||||||
|
|||||||
@@ -19,6 +19,7 @@ from nisse_core.config import (
|
|||||||
class Review:
|
class Review:
|
||||||
max_rounds: int = 3
|
max_rounds: int = 3
|
||||||
wait_s: float = 60.0
|
wait_s: float = 60.0
|
||||||
|
grace_s: float = field(default=5.0, metadata={"zero": True})
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
@@ -45,6 +46,17 @@ def test_an_agents_own_tuning_sections() -> None:
|
|||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_field_may_opt_in_to_zero() -> None:
|
||||||
|
errors: list[str] = []
|
||||||
|
tuning = parse_tuning({"review": {"grace_s": 0}}, Tuning, errors)
|
||||||
|
assert tuning.review.grace_s == 0 and errors == []
|
||||||
|
parse_tuning({"review": {"grace_s": -1, "wait_s": 0}}, Tuning, errors)
|
||||||
|
assert errors == [
|
||||||
|
"tuning.review.grace_s: must be zero or more",
|
||||||
|
"tuning.review.wait_s: must be a positive number",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_references_and_people() -> None:
|
def test_references_and_people() -> None:
|
||||||
errors: list[str] = []
|
errors: list[str] = []
|
||||||
env = {"TOKEN": "s3cret"}
|
env = {"TOKEN": "s3cret"}
|
||||||
|
|||||||
@@ -56,9 +56,9 @@ class ReviewTuning:
|
|||||||
diff_chars: int = 60000 # the diff in the review's context; files on demand
|
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
|
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
|
# 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.
|
# are pending, and ci_grace_s for checks to appear at all. 0: don't wait.
|
||||||
ci_wait_s: float = 3600.0
|
ci_wait_s: float = field(default=3600.0, metadata={"zero": True})
|
||||||
ci_grace_s: float = 120.0
|
ci_grace_s: float = field(default=120.0, metadata={"zero": True})
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
|
|||||||
@@ -431,6 +431,21 @@ def test_a_repository_pattern_must_be_owner_slash_name(tmp_path: Path) -> None:
|
|||||||
ReviewConfig.from_tree({**tree, "repositories": [bad]}, {})
|
ReviewConfig.from_tree({**tree, "repositories": [bad]}, {})
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_ci_waits_may_be_zero(tmp_path: Path) -> None:
|
||||||
|
# The live test's repository has no CI: it reviews with no grace at all.
|
||||||
|
from nisse_review.config import ReviewConfig
|
||||||
|
|
||||||
|
tree = {
|
||||||
|
"state_db": str(tmp_path / "s.db"),
|
||||||
|
"llm": {"base_url": "http://llm", "token": "k"},
|
||||||
|
"roles": {"reviewer": "local", "expert": "frontier"},
|
||||||
|
"gitea": {"url": "https://g", "token": "t", "operators": ["don"]},
|
||||||
|
"tuning": {"review": {"ci_wait_s": 0, "ci_grace_s": 0}},
|
||||||
|
}
|
||||||
|
review = ReviewConfig.from_tree(tree, {}).tuning.review
|
||||||
|
assert (review.ci_wait_s, review.ci_grace_s) == (0, 0)
|
||||||
|
|
||||||
|
|
||||||
def test_diff_lines_are_the_heads_lines_a_diff_shows() -> None:
|
def test_diff_lines_are_the_heads_lines_a_diff_shows() -> None:
|
||||||
diff = (
|
diff = (
|
||||||
"--- a/x.py\n+++ b/x.py\n@@ -1,3 +1,4 @@\n a\n-b\n+c\n+d\n e\n"
|
"--- a/x.py\n+++ b/x.py\n@@ -1,3 +1,4 @@\n a\n-b\n+c\n+d\n e\n"
|
||||||
|
|||||||
Reference in new issue
Block a user