From 515d1c865038b305b107336dc470fcd7fa3106ba Mon Sep 17 00:00:00 2001 From: moe-berri Date: Sat, 5 Sep 2026 15:43:03 -0700 Subject: [PATCH] address review: trim remaining comment verbosity --- .../auto_router/auto_router.py | 28 ++++--------------- .../router_strategy/test_auto_router.py | 16 ++--------- 2 files changed, 9 insertions(+), 35 deletions(-) diff --git a/litellm/router_strategy/auto_router/auto_router.py b/litellm/router_strategy/auto_router/auto_router.py index 7b91fb6db1a..d08afa8c1f6 100644 --- a/litellm/router_strategy/auto_router/auto_router.py +++ b/litellm/router_strategy/auto_router/auto_router.py @@ -119,12 +119,7 @@ class AutoRouter(CustomLogger): return auto_router_routes def _build_routelayer(self) -> "SemanticRouter": - """Build (once) the SemanticRouter for this alias's static route config. - - `auto_sync="local"` embeds every route's utterances against the encoder, so - this does a synchronous embedding call and must never run directly on the - event loop; see `_ensure_routelayer`. - """ + """Synchronous (embeds every route's utterances); run only via `_ensure_routelayer`.""" if self.routelayer is not None: return self.routelayer @@ -139,27 +134,16 @@ class AutoRouter(CustomLogger): return routelayer def _clear_build_task_on_failure(self, build_task: "asyncio.Task[SemanticRouter]") -> None: - """Done-callback: drop a failed build so the next caller gets a fresh attempt. - - Runs whether or not any caller is still awaiting `build_task` (that's the point: - a caller cancelled via `cancel_on_disconnect` mid-build must not leave a later - failure cached with nothing left to clear it), and `not build_task.cancelled()` - guards `.exception()`, which raises on a cancelled task instead of returning one. - """ + """Runs even with no caller left awaiting, so a failure never stays cached forever.""" if build_task is self._routelayer_build_task and not build_task.cancelled() and build_task.exception(): self._routelayer_build_task = None async def _ensure_routelayer(self) -> "SemanticRouter": - """Return the cached route layer, building it once under a lock if needed. + """Build the route layer once, off the event loop, shared across concurrent callers. - The build runs in a worker thread (it embeds the static route utterances via the - encoder's synchronous path, so it must never run directly on the event loop) as a - task stored on `self`, not a bare `asyncio.to_thread` awaited inline: a disconnected - caller cancelled via `cancel_on_disconnect` would otherwise release `_routelayer_lock` - while the thread keeps running, letting a second concurrent request see no lock held - and start (and bill) a duplicate build. Every caller awaits the same stored task - through `asyncio.shield`, so cancelling one caller's wait never cancels the build - itself or lets another caller start a second one. + A shared task (not a bare `asyncio.to_thread` awaited under the lock) survives one + caller's cancellation, so `cancel_on_disconnect` can't free a second caller into + starting a duplicate build. """ if self.routelayer is not None: return self.routelayer diff --git a/tests/test_litellm/router_strategy/test_auto_router.py b/tests/test_litellm/router_strategy/test_auto_router.py index e8ff99fa7e5..01bf0c2a5ad 100644 --- a/tests/test_litellm/router_strategy/test_auto_router.py +++ b/tests/test_litellm/router_strategy/test_auto_router.py @@ -612,13 +612,7 @@ class TestAutoRouterAttributesItsEmbeddingSpend: class ThreadTrackingEmbeddingRouter(StubEmbeddingRouter): - """Records which OS thread called the sync `embedding()` path, and how many times. - - `auto_sync="local"` route-layer construction embeds every route's utterances through - this exact method (the encoder's synchronous path), so instrumenting it - an already - dependency-injected collaborator - observes the real build without reaching into - AutoRouter's own internals. - """ + """Records which OS thread and how many times `embedding()` was called during a build.""" def __init__(self) -> None: super().__init__() @@ -675,9 +669,7 @@ class TestAutoRouterColdStartDoesNotBlockTheEventLoop: @pytest.mark.asyncio async def test_should_not_duplicate_the_build_when_a_caller_is_cancelled_mid_build(self): - """Regression: cancel_on_disconnect cancels the awaiting request, not the worker thread - actually doing the build. A second caller arriving before that thread finishes must - reuse the same in-flight build rather than starting a duplicate one.""" + """A caller arriving while the first is cancelled mid-build must reuse it, not duplicate it.""" import threading class BlockingEmbeddingRouter(ThreadTrackingEmbeddingRouter): @@ -724,9 +716,7 @@ class TestAutoRouterColdStartDoesNotBlockTheEventLoop: @pytest.mark.asyncio async def test_should_clear_a_failed_build_even_with_no_caller_left_to_observe_it(self): - """Regression: a build that fails after its only caller was already cancelled must - still clear the slot, so the next request gets a fresh attempt instead of replaying - the same stale failure forever.""" + """A build failing after its only caller was cancelled must still clear, not stay cached.""" import threading class FailsOnFirstAttemptEmbeddingRouter(ThreadTrackingEmbeddingRouter):