Skip to content

Commit d77cc3c

Browse files
committed
refactor: simplify RouteHealthCheckFilter and clean up health-check docs
- ``RouteHealthCheckFilter.health_check_required`` becomes a plain ``bool`` (default ``False``): ``False`` skips the filter, ``True`` keeps only routes whose revision has a resolved health-check block. The previous ``Optional[bool]`` carried a third "keep only opt-out routes" mode no caller used and whose meaning was easy to misread off the field name. - Drop ``_passes_health_check`` helper; inline the single remaining predicate at the call site in ``get_routes_by_statuses``. - Remove the ``health_check_required=False`` repository test that exercised the dropped semantic. - Strip the ``hc-`` shorthand from AppProxy sync handler comments and the repository test docstring — the abbreviation reads as jargon to anyone outside this branch. - Trim verbose docstrings on ``RouteData`` (back to main's one-liner) and ``RouteSessionData`` (single sentence, no FK invariant note).
1 parent 600e107 commit d77cc3c

6 files changed

Lines changed: 17 additions & 59 deletions

File tree

src/ai/backend/manager/data/deployment/types.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -265,12 +265,11 @@ class RouteTargetStatuses:
265265
class RouteHealthCheckFilter:
266266
"""In-memory gating on a revision's ``health_check_config``.
267267
268-
``True`` keeps only routes whose revision has a config; ``False``
269-
keeps only routes without; ``None`` skips the filter. Anything more
270-
specific belongs in the calling handler.
268+
``health_check_required=True`` keeps only routes whose revision has
269+
a resolved health-check block; ``False`` (default) skips the filter.
271270
"""
272271

273-
health_check_required: bool | None = None
272+
health_check_required: bool = False
274273

275274

276275
@dataclass(frozen=True)

src/ai/backend/manager/repositories/deployment/db_source/db_source.py

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -232,20 +232,6 @@ def _project_health_check_config(
232232
return model_definition.health_check_config()
233233

234234

235-
def _passes_health_check(
236-
has_health_check_config: bool,
237-
health_check_required: bool | None,
238-
) -> bool:
239-
"""Whether the revision's ``health_check_config`` presence matches
240-
``health_check_required`` (``None`` skips the check).
241-
"""
242-
if health_check_required is True:
243-
return has_health_check_config
244-
if health_check_required is False:
245-
return not has_health_check_config
246-
return True
247-
248-
249235
def _project_preset_slots(
250236
preset_row: DeploymentRevisionPresetRow | None,
251237
slot_entries: list[tuple[str, Decimal]],
@@ -1666,8 +1652,7 @@ async def get_routes_by_statuses(
16661652
routes: list[RouteData] = []
16671653
for row, model_definition, session_status in result.all():
16681654
health_check_config = _project_health_check_config(model_definition)
1669-
has_config = health_check_config is not None
1670-
if not _passes_health_check(has_config, health_check_filter.health_check_required):
1655+
if health_check_filter.health_check_required and health_check_config is None:
16711656
continue
16721657
routes.append(
16731658
RouteData(

src/ai/backend/manager/repositories/deployment/types/endpoint.py

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -62,26 +62,15 @@ class EndpointData:
6262

6363
@dataclass(frozen=True)
6464
class RouteSessionData:
65-
"""Session id + status snapshot attached to a RouteData.
66-
67-
``RoutingRow.session`` has ``ondelete=RESTRICT`` so a route cannot
68-
reference a missing session row; ``status`` therefore is always set
69-
when ``session_id`` is.
70-
"""
65+
"""Session id paired with its current status."""
7166

7267
session_id: SessionId
7368
status: SessionStatus
7469

7570

7671
@dataclass
7772
class RouteData:
78-
"""Data structure for model service route.
79-
80-
``health_check_config`` is the resolved ``ModelHealthCheck`` from the
81-
revision's ``model_definition`` (or ``None`` when the revision opted
82-
out). It is loaded eagerly with the route, so consumers do not need
83-
to re-query the revision row to know how (or whether) to probe.
84-
"""
73+
"""Data structure for model service route."""
8574

8675
route_id: uuid.UUID
8776
deployment_id: DeploymentID

src/ai/backend/manager/sokovan/deployment/route/handlers/appproxy_sync.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,11 @@ def category(cls) -> RouteHandlerCategory:
5858

5959
@classmethod
6060
def target_statuses(cls) -> RouteTargetStatuses:
61-
# NOT_CHECKED rows pass through for hc-disabled revisions; ``execute``
62-
# rejects hc-enabled NOT_CHECKED rows whose probe is still pending.
61+
# NOT_CHECKED is included because revisions that omit
62+
# ``service.health_check`` stay NOT_CHECKED for life and must
63+
# still receive traffic. ``execute`` filters out NOT_CHECKED
64+
# rows whose revision DID declare a probe (still waiting on the
65+
# first result).
6366
return RouteTargetStatuses(
6467
lifecycle=[RouteStatus.RUNNING],
6568
health=[RouteHealthStatus.HEALTHY, RouteHealthStatus.NOT_CHECKED],
@@ -80,7 +83,8 @@ def status_transitions(cls) -> RouteStatusTransitions:
8083
)
8184

8285
async def execute(self, routes: Sequence[RouteData]) -> RouteExecutionResult:
83-
# Skip NOT_CHECKED hc-enabled rows: their first probe hasn't run yet.
86+
# Routes with a configured probe must reach HEALTHY before sync;
87+
# routes whose revision declared no probe sync as soon as RUNNING.
8488
eligible = [
8589
r
8690
for r in routes

tests/unit/manager/repositories/deployment/test_deployment_repository.py

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3986,7 +3986,7 @@ async def test_traffic_status_filter(
39863986
populated_routes["running_hc_on_healthy_inactive"],
39873987
}
39883988

3989-
async def test_health_check_required_true_keeps_only_configured_revisions(
3989+
async def test_health_check_required_keeps_only_configured_revisions(
39903990
self,
39913991
deployment_repository: DeploymentRepository,
39923992
populated_routes: dict[str, uuid.UUID],
@@ -4009,34 +4009,15 @@ async def test_health_check_required_true_keeps_only_configured_revisions(
40094009
populated_routes["running_hc_on_healthy_inactive"],
40104010
}
40114011

4012-
async def test_health_check_required_false_keeps_only_unconfigured_revisions(
4013-
self,
4014-
deployment_repository: DeploymentRepository,
4015-
populated_routes: dict[str, uuid.UUID],
4016-
) -> None:
4017-
"""``health_check_required=False`` keeps only routes whose revision
4018-
has no ``ModelHealthCheck``.
4019-
"""
4020-
result = await deployment_repository.get_routes_by_statuses(
4021-
RouteTargetStatuses(
4022-
lifecycle=[RouteStatus.RUNNING],
4023-
health=list(RouteHealthStatus),
4024-
),
4025-
RouteHealthCheckFilter(health_check_required=False),
4026-
)
4027-
4028-
assert {r.route_id for r in result} == {
4029-
populated_routes["running_hc_off_not_checked_active"],
4030-
}
4031-
40324012
async def test_health_check_config_projected_from_model_definition(
40334013
self,
40344014
deployment_repository: DeploymentRepository,
40354015
populated_routes: dict[str, uuid.UUID],
40364016
) -> None:
40374017
"""``RouteData.health_check_config`` mirrors the revision's
40384018
``model_definition.health_check_config()``: a populated
4039-
:class:`ModelHealthCheck` for hc-enabled revisions, ``None`` otherwise.
4019+
:class:`ModelHealthCheck` when the revision declares
4020+
``service.health_check``, ``None`` otherwise.
40404021
"""
40414022
result = await deployment_repository.get_routes_by_statuses(
40424023
RouteTargetStatuses(

tests/unit/manager/sokovan/deployment/route/handlers/test_appproxy_sync_handler.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ def test_health_check_filter_imposes_no_repo_level_gating(self) -> None:
7272
case that AppProxy must skip is rejected in ``execute`` instead.
7373
"""
7474
filt = AppProxySyncRouteHandler.health_check_filter()
75-
assert filt.health_check_required is None
75+
assert filt.health_check_required is False
7676

7777
async def test_execute_keeps_healthy_and_unprobed_routes(self) -> None:
7878
"""HEALTHY rows pass through; NOT_CHECKED rows pass only when their

0 commit comments

Comments
 (0)