fix(platform): drop temperature for models that reject it #1834
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| """Per-model parameter support, resolved from the LLM Gateway discovery API. | ||
|
|
||
| Newer models reject parameters that older ones accept: passing `temperature` to | ||
| a model that dropped it fails the whole request with HTTP 400. The gateway | ||
| publishes these constraints per model under `modelDetails`, so callers can strip | ||
| the parameter instead of guessing from the model name. | ||
| """ | ||
|
|
||
| import logging | ||
| from typing import Any | ||
|
|
||
| from ..common._base_service import BaseService | ||
| from ..common._endpoints_manager import EndpointManager | ||
| from ..common._models import Endpoint | ||
| from ..constants import HEADER_AGENTHUB_CONFIG | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| CacheKey = tuple[str, str | None] | ||
|
|
||
| # One fetch per (base_url, agenthub_config); an empty mapping caches "couldn't read | ||
| # discovery". Deliberately unlocked: a racing duplicate GET is cheaper than an | ||
| # asyncio.Lock pinned to whichever event loop touched it first. | ||
| _model_details: dict[CacheKey, dict[str, dict[str, Any]]] = {} | ||
|
|
||
|
|
||
| async def should_skip_temperature( | ||
| service: BaseService, model: str, agenthub_config: str | None | ||
| ) -> bool: | ||
| """Whether `model` rejects the `temperature` parameter. | ||
|
|
||
| Args: | ||
| service: Service used to call discovery; supplies base URL and auth. | ||
| model: Model name as sent to the gateway. | ||
| agenthub_config: AgentHub config scoping which models are visible. | ||
|
|
||
| Returns: | ||
| True only when discovery explicitly reports the model skips temperature. | ||
| Unknown models, unreachable discovery, and any unexpected failure return | ||
| False, leaving the caller's request untouched. | ||
| """ | ||
| try: | ||
| details = await _details_for(service, model, agenthub_config) | ||
| except Exception as e: | ||
| logger.warning( | ||
| "Could not resolve parameter support for model %s (%s); " | ||
| "sending model parameters unchanged", | ||
| model, | ||
| e, | ||
| ) | ||
| return False | ||
|
|
||
| return bool(details.get("shouldSkipTemperature", False)) | ||
|
|
||
|
|
||
| async def _details_for( | ||
| service: BaseService, model: str, agenthub_config: str | None | ||
| ) -> dict[str, Any]: | ||
| key = (service._config.base_url, agenthub_config) | ||
|
|
||
| if key not in _model_details: | ||
|
There was a problem hiding this comment. This caches a discovery failure the same way it caches a real result. One transient discovery blip at process startup (timeout, 5xx, network hiccup) poisons This is the exact regression called out before this PR was cut: must not cache discovery failures, only successes, or add a TTL. Suggest not writing to |
||
| _model_details[key] = await _fetch_model_details(service, agenthub_config) | ||
|
Comment on lines
+61
to
+62
There was a problem hiding this comment.
When the first discovery request for a cache key is transiently unavailable or returns an invalid payload, Useful? React with 👍 / 👎. |
||
|
|
||
| return _model_details[key].get(model, {}) | ||
|
|
||
|
|
||
| async def _fetch_model_details( | ||
| service: BaseService, agenthub_config: str | None | ||
| ) -> dict[str, dict[str, Any]]: | ||
| headers = {HEADER_AGENTHUB_CONFIG: agenthub_config} if agenthub_config else {} | ||
| endpoint = Endpoint("/" + EndpointManager.get_discovery_endpoint()) | ||
|
|
||
| try: | ||
| response = await service.request_async("GET", endpoint, headers=headers) | ||
| models = response.json() | ||
| except Exception as e: | ||
| # Falling back to the caller's parameters is the pre-existing behaviour; | ||
| # failing the LLM call because discovery is down would be worse. | ||
| logger.warning( | ||
| "LLM Gateway discovery unavailable (%s); sending model parameters unchanged", | ||
| e, | ||
| ) | ||
| return {} | ||
|
|
||
| if not isinstance(models, list): | ||
| logger.warning("Unexpected LLM Gateway discovery payload; expected a list") | ||
| return {} | ||
|
|
||
| return { | ||
| model["modelName"]: model.get("modelDetails") or {} | ||
| for model in models | ||
| if isinstance(model, dict) and model.get("modelName") | ||
| } | ||
|
|
||
|
|
||
| def _reset_cache() -> None: | ||
| """Clear the discovery cache. For tests.""" | ||
| _model_details.clear() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove is_reasining_model check here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, but I'd keep it — and it can't be deleted outright anyway.
Two reasons to keep it in the
or:should_skip_temperaturefails open by design. It returnsFalsefor an unknown model, unreachable discovery, or any unexpected exception. Ifis_reasoning_modelcomes out of the expression, a discovery outage means o1/o3/o4 start receivingtemperatureagain — reintroducing exactly the 400 this PR exists to fix, for models that were previously safe. It gets worse combined with the caching behaviour I flagged separately:_details_forcaches the failure, so one blip at process start would keep reasoning models 400ing for the whole process lifetime.It short-circuits. Python
ormeans a known reasoning model never makes the discovery call at all — no round-trip, no latency, no dependency.There's also an unverified assumption buried in the removal: that gateway discovery actually reports
shouldSkipTemperature: truefor o1/o3/o4. Worth checking empirically, but even if it does, point 1 still argues for keeping the local guard as the deterministic floor.And separately — the variable itself is load-bearing elsewhere: line 584,
if not is_claude_model and not is_reasoning_model:, gatesn,frequency_penalty,presence_penaltyandtop_p. Sois_reasoning_modelhas to stay regardless; the only question was whether to drop it from theskip_temperatureexpression.Net: the local check is a cheap deterministic floor, discovery is the extension for models we can't name ahead of time. Belt and braces is right here.