feat(llm): resolve provider-specific runtime metadata for routed models - #4423
feat(llm): resolve provider-specific runtime metadata for routed models#4423neubig wants to merge 2 commits into
Conversation
The static model catalog (LiteLLM metadata) describes a model's nominal limits, which can overstate the limit of the route a configured provider actually serves. On OpenRouter, for example, deepseek-v4-flash advertises a 1M-token context while the CoreWeave endpoint limits requests to 262k. This makes LLM.effective_max_input_tokens wrong for routed models and feeds the wrong value into context management and telemetry. Add LLM.resolve_runtime_metadata()/aresolve_runtime_metadata(), which resolve route-aware limits lazily, cache them with a TTL (positive and negative), deduplicate concurrent lookups, and always fall back to model metadata on any error. The OpenRouter adapter queries the per-endpoint catalog and applies `litellm_extra_body.provider` routing semantics (only/ignore/order/allow_fallbacks): exact for a pinned route, a conservative lower bound when multiple eligible routes differ, and None when routing is not safely interpretable. Provider transport is confined to a private module so it can be retired when LiteLLM gains equivalent support. effective_max_input_tokens now consults the cached route-aware value without any network I/O in the property, preserving explicit- max_input_tokens precedence. Closes: #4421 Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
…e an event loop resolve_provider_metadata_sync() called asyncio.run(), which raises RuntimeError when the caller already owns a running event loop (e.g. the agent-server async path). Detect that case up front and return None so the caller falls back to model-level metadata instead of crashing, matching the module's fallback-on-any-error contract. Co-authored-by: openhands <openhands@all-hands.dev>
|
Pushed a65af7375e92a8d8987081aa704cf76de5a (a8af737) which resets the head SHA and clears the two stale This comment was created by an AI agent (OpenHands) on behalf of the automating user. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Overview
This PR adds provider-aware runtime metadata resolution for routed models (OpenRouter), addressing issue #4421 where the static LiteLLM catalog overstates context limits for specific endpoints. The design is clean: lazy resolution, TTL caching (positive + negative), concurrent-call deduplication, and graceful fallback to model metadata on any error. The OpenRouter adapter correctly implements only/ignore/order/allow_fallbacks routing semantics with conservative lower bounds when multiple eligible routes differ.
Risk Assessment: Medium
The code is well-structured and the fallback strategy is safe (any error → None → model metadata). However, there are two material gaps that prevent the PR from actually fixing the issue it claims to close, plus a thread-safety concern.
Material Findings
1. The feature is not wired up — the fix is inert (Correctness)
effective_max_input_tokens consults _runtime_metadata if it has been cached, but nothing in the codebase ever calls resolve_runtime_metadata() or aresolve_runtime_metadata(). The cache is never populated unless an external caller explicitly invokes these methods. This means effective_max_input_tokens will still return the model-level metadata value (the buggy behavior described in #4421) because _runtime_metadata stays None.
The PR description says "Closes #4421," but without a caller — e.g., in conversation startup, Agent.step(), or the condenser path — the bug is not actually fixed. At minimum, the agent-server or LocalConversation startup should call aresolve_runtime_metadata() before the first LLM completion. If the intent is to wire this up in a follow-up PR, the PR description should clarify that and not claim to close the issue.
2. effective_max_output_tokens does not consult runtime metadata (Consistency)
ModelRuntimeMetadata has a max_output_tokens field populated by the OpenRouter adapter, but effective_max_output_tokens (unchanged in this PR) does not consult it. For joint-budget providers, the output token limit feeds into _clamp_max_tokens_for_joint_budget() alongside effective_max_input_tokens. If the input context is route-aware but the output limit is not, the clamping math can be inconsistent. Either wire max_output_tokens into effective_max_output_tokens or remove the field from the model to avoid implying it is used.
3. Thread-safety of sync path instance mutation (Concurrency)
resolve_runtime_metadata() (sync) mutates _runtime_metadata, _runtime_metadata_fetched_at, and _runtime_metadata_negative_until without any lock. The async path has _inflight for deduplication, but the sync path has no dedup or locking. Since LLM is documented as an immutable Pydantic model that may be shared across conversations (and potentially threads), concurrent sync calls could race on these PrivateAttr fields. The worst case is duplicate upstream requests rather than corruption, but a lock or a note restricting the sync path to single-threaded use would be prudent.
Minor Notes
selected_providerforsafe_lower_boundconfidence: Whenallow_fallbacks=Trueand multiple eligible routes have different context limits,selected_provideris set to the first entry inorder. But the router may use any eligible provider at runtime. Reporting a singleselected_providerwhen any could be used is slightly misleading for telemetry. Consider setting it toNoneforsafe_lower_bound.- Missing test for
onlyrouting positive case: There is a test foronlyreturningNonewhen no match, but no test verifyingonlymatching and returningconfidence="exact"with the correct endpoint. The logic is straightforward but worth covering. detect_providerprefix check:model.startswith("openrouter")would match a hypothetical model likeopenrouterx/foo. Considermodel.startswith("openrouter/")for precision.
What Looks Good
- The fallback-on-any-error strategy (
except Exception→None) is the right call for metadata discovery that must never block a completion. - The negative cache prevents hammering a flaky endpoint.
- The
force=Truebypass for cache refresh is a clean escape hatch. - The sync-resolver-inside-running-loop guard is thoughtful.
- Serialization correctly excludes runtime cache fields (
test_serialization_excludes_runtime_cache). - The
httpx.MockTransporttest approach exercises real resolver/merge paths without live network calls.
| ) | ||
| if cached is not None and cached.max_input_tokens is not None: | ||
| return cached.max_input_tokens | ||
| return self._effective_max_input_tokens |
There was a problem hiding this comment.
Feature is not wired up. effective_max_input_tokens consults _runtime_metadata if cached, but no code anywhere in the codebase calls resolve_runtime_metadata() or aresolve_runtime_metadata(). The cache is never populated, so this property still returns the model-level metadata value — the bug from #4421 persists. A caller (e.g., conversation/agent startup) must invoke aresolve_runtime_metadata() before the first LLM completion for this fix to take effect. If wiring is deferred to a follow-up, the PR should not claim to close #4421.
| return None | ||
|
|
||
| metadata = resolve_provider_metadata_sync(self) | ||
| self._store_runtime_metadata(metadata) |
There was a problem hiding this comment.
Thread-safety concern (sync path). resolve_runtime_metadata() mutates _runtime_metadata, _runtime_metadata_fetched_at, and _runtime_metadata_negative_until without a lock. The async path has _inflight for dedup, but the sync path has none. Since LLM is an immutable Pydantic model that may be shared across conversations/threads, concurrent sync calls could race on these PrivateAttr fields. Consider adding a lock or documenting that the sync path is single-threaded only.
| max_output_tokens=min(completion_limits) if completion_limits else None, | ||
| source="openrouter_endpoints_api", | ||
| candidate_providers=provider_names, | ||
| selected_provider=reordered[0].get("provider_name"), |
There was a problem hiding this comment.
selected_provider is misleading for safe_lower_bound. With allow_fallbacks=True and multiple eligible routes, the router may use any provider at runtime, but selected_provider is set to the first entry in order. Consider setting selected_provider=None when confidence="safe_lower_bound" to avoid implying a specific provider was selected.
2 participants
HUMAN:
Validated the runtime metadata changes with the listed unit and lint checks.
AGENT:
Why
The static model catalog (LiteLLM metadata) describes a model's nominal limits, which can overstate the limit of the route a configured provider actually serves. On OpenRouter,
deepseek/deepseek-v4-flash-0731advertises a 1M-token context, but the CoreWeave endpoint limits requests to 262k tokens. Because LiteLLM imports the model-level catalog value,LLM.effective_max_input_tokensreports the wrong context for routed models, feeding the wrong value into context validation, token-aware condensation, and telemetry.See #4421.
Summary
LLM.resolve_runtime_metadata()/aresolve_runtime_metadata(): resolve route-aware limits lazily, cache with a TTL (positive and negative), deduplicate concurrent lookups, and fall back to model metadata on any error.openhands/sdk/llm/utils/providers/openrouter.py) that queries the per-endpoint catalog and applieslitellm_extra_body.providerrouting semantics (only/ignore/order/allow_fallbacks): exact for a pinned route, conservative lower bound when multiple eligible routes differ in context,Nonewhen routing is not safely interpretable.effective_max_input_tokensnow consults the cached route-aware value with no network I/O in the property, preserving explicit-max_input_tokensprecedence and pre-resolution behavior.ModelRuntimeMetadatafromopenhands.sdk.llm.Issue Number
Closes #4421
How to Test
Unit tests use
httpx.MockTransportagainst the real resolver/merge paths (realLLMconstruction, caching, precedence, routing semantics):Existing LLM coverage is unchanged (all pass):
Lint/type/import checks via
uv run --project . pre-commit run --files <changed files>: Ruff format, Ruff lint, pycodestyle, pyright, import-rules, and tool-registration all pass.A live OpenRouter call is not required: the adapter's HTTP is exercised through
httpx.MockTransport, and the parsing/routing logic is covered directly.This pull request was created by an AI agent (OpenHands) on behalf of Graham Neubig.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:a8af737-pythonRun
All tags pushed for this build
About Multi-Architecture Support
a8af737-python) is a multi-arch manifest supporting both amd64 and arm64a8af737-python-amd64) are also available if neededCloses #4428