fix(routing): wire cheapest_upstream into embedding member resolution - #965
Conversation
cheapest_upstream was a headline-documented, __all__-exported "cost optimiser" export that CostRoutingCoordinator never called anywhere in production -- its only real invocations were the two unit tests that exercise the function directly (confirmed by grep against the full repo). Wire it into a real routing decision: CostRoutingCoordinator's _resolve_embedding_target previously picked candidates[0] (an arbitrary, price-blind first-of-ranked-list pick) whenever an embedding batch request left the model member unspecified among several capability-matched candidates (e.g. operator-managed model-group members). It now uses a new _cheapest_capability_candidate helper, backed by cheapest_upstream and the coordinator's own PriceBook, so the cheapest priced member wins instead. Ties/unpriced candidates keep the original order, so behavior is unchanged whenever the price table has nothing to optimise. Adds a regression test proving the wiring overrides the naturally higher-ranked (by priority) but more expensive candidate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough임베딩 capability 후보 선택이 가격표 기반으로 변경되었습니다. 가격이 없거나 통화가 다른 후보는 비교에서 제외됩니다. 미지정 비-ZDR 임베딩 요청도 최저가 후보를 선택합니다. 관련 테스트와 변경 기록이 추가되었습니다. Changes비용 인지 임베딩 라우팅
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to Unspecified embedding requests now select the cheapest eligible configured provider member while preserving existing policy and eligibility controls. The change is mergeable with owner follow-up to correct documentation and test wording that currently misstates how the price-based selection is implemented; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant complete_embeddings_batch
participant CostRoutingCoordinator
participant capability_agents
participant PriceBook
complete_embeddings_batch->>CostRoutingCoordinator: 임베딩 배치 전달
CostRoutingCoordinator->>capability_agents: capability 후보 조회
capability_agents-->>CostRoutingCoordinator: 후보 목록 반환
CostRoutingCoordinator->>PriceBook: 후보별 가격 조회
PriceBook-->>CostRoutingCoordinator: 비교 가능한 가격 반환
CostRoutingCoordinator-->>complete_embeddings_batch: 최저가 임베딩 대상 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 65.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review found three real bugs in the embedding-target cheapest-price helper added in this PR (all at cost_router.py:158): - An unpriced or invalid PriceBook entry priced at 0.0 and could beat every paid candidate. get_price() is now checked directly so a missing/invalid entry stays "unknown" and is excluded from the price comparison instead of winning as a false zero-cost candidate. - A candidate priced in a currency other than PriceBook.default_currency was compared to same-currency prices by face value. Only entries whose currency matches default_currency are now compared; there is no exchange-rate conversion source anywhere in this repo to convert with. - cheapest_upstream() was called with its default assumed_completion_tokens (1000), pricing 1,000 nonexistent completion tokens for what is strictly an input-only embedding request. The embedding call site now passes assumed_completion_tokens=0. Candidates with no comparable (known, same-currency) price keep the orchestrator's existing ranked order, matching the pre-965 fallback and the documented "unchanged when there's nothing to optimise" behavior. No other production caller of cheapest_upstream exists, so its default signature and other call sites are untouched. Adds four regression tests in tests/test_cost_router.py exercising each fixed case directly against _cheapest_capability_candidate, plus the all-unpriced ranked-order-preserved case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
This happened while waiting on the local Generated by Claude Code |
Devin Review found two more real bugs on PR #965's embedding cheapest-price routing (both cost_router.py, after commit 0223811): - _cheapest_capability_candidate's currency-comparability check used an exact string match, so a lowercase or whitespace-padded same-currency code (e.g. "usd" vs "USD") was wrongly treated as incomparable and lost to a costlier candidate. Now reuses model_discovery._currency_is_comparable (non-empty, trimmed, case-insensitive) for the same normalization already used elsewhere in this codebase, instead of inventing a second one. - _resolve_embedding_target returned before _cheapest_capability_candidate ever ran whenever zdr_only=False, so only ZDR embedding batches were cost-aware; ordinary unspecified (auto/group model) embedding batches always kept the orchestrator's ranked-not-price order. The early-return passthrough is now scoped to only the two cases it must protect - an explicit agent_id and an explicit model outside the configured pool (regardless of zdr_only) - so an unspecified model with no agent_id now runs the same cheapest-comparable-member selection non-ZDR requests were missing. Regression tests added in tests/test_cost_router.py: - test_cheapest_capability_candidate_compares_equivalent_currency_spellings - test_non_zdr_embedding_batch_selects_cheapest_capability_candidate_when_unspecified - test_non_zdr_complete_embeddings_batch_selects_cheapest_capability_candidate (through complete_embeddings_batch, the public coordinator entry point) - test_non_zdr_embedding_batch_preserves_explicit_model_outside_the_pool tests/test_cost_router_boundaries.py's shared _coordinator() fixture agent gains an "embedding" tag: its embedding-document lifecycle tests submit unspecified-model batches that now resolve through the real capability-agent lookup instead of bypassing it. CHANGELOG.md cross-references ADR 0003 (cost-aware sync/batch routing), which already covers cheapest_upstream table-driven load balancing as design grounding for this bugfix rather than needing new citations. Full validation: pytest tests -q (2861 passed, same 2 pre-existing/ unrelated environment failures as before - missing mcp.Client test double and missing fast_mlsirm module); interrogate (100%); tests/test_conventions.py (passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ded cost Devin review round 3 on #965 (contextual_orchestrator/cost_router.py:181-183): `_cheapest_capability_candidate` ranked comparable embedding candidates by delegating to `cheapest_upstream`, which compares `PriceBook.compute_cost` results quantized to six decimal places for ledger reporting. Two genuinely different low per-1K prices (e.g. 0.00000049 and 0.00000001) can both round to the same 0.0 ledger cost for the assumed 1,000-token request, collapsing a real price difference into a tie and letting the ranked-first (possibly costlier) candidate win. Ranking now compares each comparable candidate's raw, unrounded PriceEntry.prompt_price_per_1k directly instead of routing through cheapest_upstream's rounded compute_cost output (embedding requests carry zero completion tokens, so completion price stays out of the comparison, matching the round-2 fix). cheapest_upstream itself is untouched -- its rounded cost remains correct for its own ledger-reporting callers -- as are currency filtering, the unpriced-exclusion behavior, and ranked-order tie-breaking on a true price tie. Adds a regression test with two candidates whose raw prices are distinct but both quantize to the same rounded ledger amount, asserting the actually cheaper one now wins. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Round 3 changed CostRoutingCoordinator._cheapest_capability_candidate to compare PriceEntry.prompt_price_per_1k directly via PriceBook.get_price(), removing its earlier call to cheapest_upstream. Three docs/comments still described the old behavior; update them to describe the current direct price-lookup implementation, noting the two helpers play a similar role without one calling the other. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review follow-up on PR #965 (after the concurrent fix(routing): preserve embedding intent and health order, ec0d7a4, already fixed the earlier two findings — premature model resolution before cost routing, and unconditional cheapest-first ignoring health evidence): an omitted-model request to /v1/embeddings can now resolve to a cheaper or failed-over candidate than the highest-ranked model _validate_embeddings_model guessed before candidate discovery ran, but the sync response still reported that pre-failover guess instead of who actually served the request. /v1/embeddings now reports the completed document's own served model for an omitted-model request (explicit-model requests are unchanged). /v1/batch/embeddings needed no change: its response is the raw batch document, whose model field is already derived from the actually- submitted request. Verified the cost ledger's own model_name attribution dimension was never affected either: CostLedger .record_usage deliberately strips and overwrites any caller-supplied attribution["model_name"] with the real served model argument ("execution identity always wins"), so ledger/spend rollups were already correct independent of this bug. Adds one parametrized regression test (sync + batch) covering an omitted-model request where the cost-selected served member differs from the initially-ranked model, asserting both the response model and the ledger's recorded model_name reflect who actually served it. Verification: python -m pytest tests -q --ignore=tests/test_psychometric_routing.py -> 2867 passed, 1 skipped (the ignored file needs numpy, gated to Python >=3.12 per pyproject.toml; this sandbox runs 3.11, matching the PR's own documented pre-existing environment gap). interrogate: PASSED 100.0%. tests/test_conventions.py: passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Required
|
Required
|
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Checked this PR's changed-file list against #971's for the cross-PR Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
…ile compute_cost 3-tuple
| embedding_agents = coordinator._cost_ordered_capability_candidates( | ||
| embedding_agents | ||
| ) |
There was a problem hiding this comment.
🟡 Unattributed embedding costs disappear
When provider metadata is omitted, _cost_ordered_capability_candidates selects a provider without recording its identity. The ledger prices unknown, losing configured cost and provider attribution.
Prompt for agents
The HTTP embedding paths in contextual_orchestrator/server.py now choose a concrete ModelAgent, but the attribution passed to CostRoutingCoordinator.complete_embeddings_batch still lacks the gateway-selected provider. embeddings_batch_document derives provider only from request attribution and ignores EmbeddingBatchRequest.agent_id, so ordinary clients that do not spoof/provide a provider dimension get unknown provider, unknown price, and incorrect provider rollups. Carry the selected agent's actual provider identity through a trusted execution-identity path for both /v1/embeddings and /v1/batch/embeddings. Preserve CostLedger.record_usage's rule that caller-supplied provider/model attribution cannot override execution identity.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.orchestrator._group_router.member_report(candidate.id)[ | ||
| "success_posterior_mean" | ||
| ] | ||
| >= 0.5 |
There was a problem hiding this comment.
🟡 One failure permanently demotes cheapest model
Without external health observations, one failure makes _cost_ordered_capability_candidates demote the cheapest model. Successful fallbacks prevent further probes, so transient outages permanently raise routing cost.
Prompt for agents
The health gate in CostRoutingCoordinator._cost_ordered_capability_candidates uses posterior_mean >= 0.5. Under the router's Beta(1,1) prior, one failure immediately makes a member unhealthy. The server then appends it after healthy candidates, and a successful fallback stops iteration, so an embedding-only member without external telemetry receives no future success observation and cannot recover. Introduce a bounded recovery/probing or time-decay policy, or use routing evidence that permits transiently failed members to be retried without putting them first on every request. Add a test that fails the cheapest member once, restores it, keeps the fallback healthy, and verifies eventual automatic readmission without directly calling observe_success.
Was this helpful? React with 👍 or 👎 to provide feedback.
| embedding_agents = orchestrator._capability_agents( | ||
| "embedding", | ||
| TaskOrchestrator.AUTO_MODEL if model_was_omitted else model_name, | ||
| ) | ||
| embedding_agents = coordinator._cost_ordered_capability_candidates( | ||
| embedding_agents | ||
| ) |
There was a problem hiding this comment.
📝 Info: Free-model filtering remains intact
Explicit orchestrator/free requests retain _capability_agents zero-price filtering before cost ordering. The omitted-model AUTO expansion does not affect this contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
| comparable: list[tuple[Any, PriceEntry]] = [] | ||
| for candidate in candidates: | ||
| provider, model = self._agent_provider_model(candidate, candidate.model) | ||
| entry = self.price_book.get_price(provider, model) | ||
| if entry is None or not _currency_is_comparable( | ||
| entry.currency_code, self.price_book.default_currency | ||
| ): | ||
| continue | ||
| comparable.append((candidate, entry)) | ||
| if not comparable: | ||
| return candidates[0] | ||
| best_candidate, best_entry = comparable[0] | ||
| for candidate, entry in comparable[1:]: | ||
| if entry.prompt_price_per_1k < best_entry.prompt_price_per_1k: | ||
| best_candidate, best_entry = candidate, entry | ||
| return best_candidate |
There was a problem hiding this comment.
📝 Info: Unknown prices preserve prior ranking
_cheapest_capability_candidate excludes unknown and cross-currency prices. Empty comparable sets and true price ties preserve the orchestrator’s existing order.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Devin follow-up: an omitted model can resolve to a | ||
| # different (cheaper or failed-over) member than the | ||
| # pre-failover, price-blind ``model_name`` validation | ||
| # picked, so report the completed document's own | ||
| # served model instead — it already carries the | ||
| # actually-used agent's model (see | ||
| # ``embeddings_batch_document``). An explicit model | ||
| # (a concrete pool model or a group alias) still | ||
| # reports exactly what the client asked for. | ||
| model=document.get("model") if model_was_omitted else model_name, |
There was a problem hiding this comment.
📝 Info: Explicit aliases retain response identity
Explicit group aliases remain the response model while execution uses a concrete member. Omitted requests alone expose the actually served model.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Hollow-path finding
contextual_orchestrator/batch_routing.py:cheapest_upstreamwas advertised as one of three headline exports of the "LiteLLM-plus cost optimiser" in the module's own top docstring, and re-exported throughcontextual_orchestrator/__init__.py's__all__— butCostRoutingCoordinator(the actual router) never called it in production. Its only real invocations anywhere in the repo were the two unit tests that exercise the function directly (tests/test_batch_routing.py,tests/test_batch_routing_boundaries.py), confirmed by grep against the full repo before making any change.Re-verified against current
origin/main(c6c3a0c9) before touching anything — the finding was accurate and current.Fix
Chose "wire it into the real router" (the finding's first suggested direction) over "demote the docstring/
__all__", becausedocs/adr/0003-cost-aware-sync-batch-routing.mdandAGENTS.mdboth already describecheapest_upstream-style "upstream load balancing among priced candidates" as one of this repo's three headline capabilities (cost optimiser + upstream load balancing + batch routing) — the intent was real, the wiring was just missing.CostRoutingCoordinator._resolve_embedding_targetpreviously did:— an arbitrary, price-blind pick of the first (highest quality/throughput-ranked) capability-matched candidate whenever an embedding batch request left the model member unspecified (e.g. resolving one member of an operator-managed embedding model group). It now calls a new
_cheapest_capability_candidatehelper, backed bycheapest_upstreamand the coordinator's ownPriceBook, so the cheapest priced member wins instead. Unpriced or tied candidates keep the original ranked order, so behavior is unchanged whenever the price table has nothing to optimise (verified this doesn't regress any existing single-candidate or explicit-agent_idtest).Also added a one-line cross-reference in
cheapest_upstream's own docstring pointing at the new caller, and aCHANGELOG.mdentry under the existing## [0.2.0] - Unreleased/### Fixedsection.Scope is deliberately narrow: only the embedding-target resolution path (a simple, already price-blind
candidates[0]pick) was touched. The chat/ZDR batch path (_resolve_batch_request) and the general capability-agent selector (_ranked_agents/select_capability_agent, used well beyond routing) were left alone — those already run a considered quality/throughput ranking algorithm, and folding cost into that broader ranking is a larger design decision outside this finding's scope.Verification
test_embedding_batch_selects_cheapest_capability_candidate_when_unspecifiedintests/test_cost_router.py: constructs two ZDR-tagged embedding candidates where the orchestrator's own priority-based ranking would return the more expensive one first (asserted directly against_capability_agents), then asserts the coordinator resolves to the cheaper one once a price table is configured.python3 -m pytest tests -q: 2857 passed, 2 pre-existing failures, both reproduced identically on a clean, unmodified checkout of this sameorigin/maincommit and unrelated to this change (a pinned-vs-installedmcppackage version mismatch intest_privacy_policy_analysis.py, and the optionalfast_mlsirmdependency requiring Python ≥3.12 while this sandbox runs 3.11 — seepyproject.toml's environment marker on that dependency).interrogate(repo-wide, matching the[tool.interrogate]fail-under = 100gate inpyproject.toml): PASSED, 100.0%.coverage run/reportscoped to the two changed files: 100% statement coverage on bothcontextual_orchestrator/cost_router.pyandcontextual_orchestrator/batch_routing.pyfrom the dedicated batch-routing/cost-router/embeddings test files alone.tests/test_conventions.py(object-naming gate): passed.No new dependency added (Ponytail gate: n/a). No PR template exists in this repo (checked
.github/), so this description follows the shape of recent merged PRs' commit/PR messages instead.Generated by Claude Code
Summary by CodeRabbit
새 기능
버그 수정