mirror of
https://github.com/affaan-m/ECC.git
synced 2026-10-19 14:31:50 +02:00
fix(llm): honor selected models in provider resolution
This commit is contained in:
@@ -26,8 +26,14 @@ def _uses_adaptive_thinking_only(model: str) -> bool:
|
||||
class ClaudeProvider(LLMProvider):
|
||||
provider_type = ProviderType.CLAUDE
|
||||
|
||||
def __init__(self, api_key: str | None = None, base_url: str | None = None) -> None:
|
||||
def __init__(
|
||||
self,
|
||||
api_key: str | None = None,
|
||||
base_url: str | None = None,
|
||||
default_model: str | None = None,
|
||||
) -> None:
|
||||
self.client = Anthropic(api_key=api_key or os.environ.get("ANTHROPIC_API_KEY"), base_url=base_url)
|
||||
self.default_model = default_model or _DEFAULT_MODEL
|
||||
self._models = [
|
||||
ModelInfo(
|
||||
name="claude-opus-4-8",
|
||||
@@ -57,7 +63,7 @@ class ClaudeProvider(LLMProvider):
|
||||
|
||||
def generate(self, input: LLMInput) -> LLMOutput:
|
||||
try:
|
||||
model = input.model or _DEFAULT_MODEL
|
||||
model = input.model or self.default_model
|
||||
system_parts = [msg.content for msg in input.messages if msg.role == Role.SYSTEM]
|
||||
api_messages = [
|
||||
msg.to_dict() for msg in input.messages if msg.role not in (Role.SYSTEM,)
|
||||
@@ -139,4 +145,4 @@ class ClaudeProvider(LLMProvider):
|
||||
return bool(self.client.api_key)
|
||||
|
||||
def get_default_model(self) -> str:
|
||||
return _DEFAULT_MODEL
|
||||
return self.default_model
|
||||
|
||||
@@ -28,12 +28,18 @@ from llm.providers.openai_compat import to_openai_message_param
|
||||
class OpenAIProvider(LLMProvider):
|
||||
provider_type = ProviderType.OPENAI
|
||||
|
||||
def __init__(self, api_key: str | None = None, base_url: str | None = None) -> None:
|
||||
def __init__(
|
||||
self,
|
||||
api_key: str | None = None,
|
||||
base_url: str | None = None,
|
||||
default_model: str | None = None,
|
||||
) -> None:
|
||||
self.client = OpenAI(
|
||||
api_key=api_key or os.environ.get("OPENAI_API_KEY"),
|
||||
base_url=base_url,
|
||||
_enforce_credentials=False,
|
||||
)
|
||||
self.default_model = default_model or "gpt-4o-mini"
|
||||
self._models = [
|
||||
ModelInfo(
|
||||
name="gpt-4o",
|
||||
@@ -72,7 +78,7 @@ class OpenAIProvider(LLMProvider):
|
||||
def generate(self, input: LLMInput) -> LLMOutput:
|
||||
try:
|
||||
params: dict[str, Any] = {
|
||||
"model": input.model or "gpt-4o-mini",
|
||||
"model": input.model or self.default_model,
|
||||
"messages": [to_openai_message_param(msg) for msg in input.messages],
|
||||
"temperature": input.temperature,
|
||||
}
|
||||
@@ -129,4 +135,4 @@ class OpenAIProvider(LLMProvider):
|
||||
return bool(self.client.api_key)
|
||||
|
||||
def get_default_model(self) -> str:
|
||||
return "gpt-4o-mini"
|
||||
return self.default_model
|
||||
|
||||
@@ -22,6 +22,16 @@ _PROVIDER_MAP: dict[ProviderType, type[LLMProvider]] = {
|
||||
ProviderType.OLLAMA: OllamaProvider,
|
||||
}
|
||||
|
||||
# Only built-in adapters have a known default_model constructor contract.
|
||||
_MODEL_ENV_VARS: dict[type[LLMProvider], tuple[str, ...]] = {
|
||||
ClaudeProvider: (),
|
||||
OpenAIProvider: (),
|
||||
OllamaProvider: ("OLLAMA_MODEL",),
|
||||
AtlasProvider: (AtlasProvider.model_env,),
|
||||
AstraflowProvider: (AstraflowProvider.model_env,),
|
||||
AstraflowCNProvider: (AstraflowCNProvider.model_env, AstraflowProvider.model_env),
|
||||
}
|
||||
|
||||
LLM_ENV_FILE = ".llm.env"
|
||||
|
||||
|
||||
@@ -47,20 +57,48 @@ def _read_saved_llm_config(env_path: str | Path = LLM_ENV_FILE) -> dict[str, str
|
||||
return config
|
||||
|
||||
|
||||
def _resolve_provider_type(provider_type: ProviderType | str | None) -> ProviderType | str:
|
||||
def _resolve_provider_config(
|
||||
provider_type: ProviderType | str | None,
|
||||
) -> tuple[ProviderType | str, str | None]:
|
||||
if provider_type is not None:
|
||||
return provider_type
|
||||
return provider_type, None
|
||||
|
||||
env_provider = os.environ.get("LLM_PROVIDER")
|
||||
if env_provider:
|
||||
return _strip_env_value(env_provider).lower()
|
||||
return _strip_env_value(env_provider).lower(), None
|
||||
|
||||
saved_config = _read_saved_llm_config()
|
||||
return saved_config.get("LLM_PROVIDER", "claude").lower()
|
||||
saved_provider = saved_config.get("LLM_PROVIDER", "claude").lower()
|
||||
# A saved model belongs to its saved provider, never to an override.
|
||||
saved_model = saved_config.get("LLM_MODEL") if saved_config.get("LLM_PROVIDER") else None
|
||||
return saved_provider, saved_model
|
||||
|
||||
|
||||
def _resolve_default_model(
|
||||
provider_type: ProviderType, provider_cls: type[LLMProvider], saved_model: str | None,
|
||||
) -> str | None:
|
||||
for name in _MODEL_ENV_VARS[provider_cls]:
|
||||
model = os.environ.get(name)
|
||||
if model:
|
||||
return model
|
||||
|
||||
env_provider = _strip_env_value(os.environ.get("LLM_PROVIDER", "")).lower()
|
||||
if not env_provider or env_provider == provider_type.value:
|
||||
env_model = _strip_env_value(os.environ.get("LLM_MODEL", ""))
|
||||
if env_model:
|
||||
return env_model
|
||||
return saved_model or None
|
||||
|
||||
|
||||
def get_provider(provider_type: ProviderType | str | None = None, **kwargs: str) -> LLMProvider:
|
||||
provider_type = _resolve_provider_type(provider_type)
|
||||
"""Create a provider, honoring the model selected by llm-select.
|
||||
|
||||
Built-in model defaults prefer explicit kwargs, then existing provider-specific
|
||||
environment variables, LLM_MODEL, and the saved provider/model pair. Saved
|
||||
config is only read when neither an explicit nor an environment provider wins.
|
||||
Custom providers retain their own constructor and configuration semantics.
|
||||
"""
|
||||
provider_type, saved_model = _resolve_provider_config(provider_type)
|
||||
|
||||
if isinstance(provider_type, str):
|
||||
try:
|
||||
@@ -72,6 +110,11 @@ def get_provider(provider_type: ProviderType | str | None = None, **kwargs: str)
|
||||
if not provider_cls:
|
||||
raise ValueError(f"No provider registered for type: {provider_type}")
|
||||
|
||||
if "default_model" not in kwargs and provider_cls in _MODEL_ENV_VARS:
|
||||
model = _resolve_default_model(provider_type, provider_cls, saved_model)
|
||||
if model:
|
||||
kwargs = {**kwargs, "default_model": model}
|
||||
|
||||
return provider_cls(**kwargs)
|
||||
|
||||
|
||||
|
||||
@@ -38,6 +38,41 @@ def make_response(content: list[SimpleNamespace], stop_reason: str = "tool_use")
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
("default_model", "request_model", "expected_model"),
|
||||
[
|
||||
(None, None, "claude-sonnet-4-6"),
|
||||
(None, "claude-haiku-4-5", "claude-haiku-4-5"),
|
||||
("claude-haiku-4-5", None, "claude-haiku-4-5"),
|
||||
("claude-opus-4-8", None, "claude-opus-4-8"),
|
||||
("claude-haiku-4-5", "claude-opus-4-8", "claude-opus-4-8"),
|
||||
],
|
||||
)
|
||||
def test_generate_respects_configured_default_and_request_model(
|
||||
default_model: str | None, request_model: str | None, expected_model: str
|
||||
) -> None:
|
||||
provider_options = {"api_key": "test-key"}
|
||||
if default_model is not None:
|
||||
provider_options["default_model"] = default_model
|
||||
provider = ClaudeProvider(**provider_options)
|
||||
client = FakeClient(make_response([SimpleNamespace(type="text", text="ok")]))
|
||||
provider.client = client
|
||||
expected_default = default_model or "claude-sonnet-4-6"
|
||||
|
||||
assert provider.get_default_model() == expected_default
|
||||
|
||||
provider.generate(
|
||||
LLMInput(messages=[Message(role=Role.USER, content="Hi")], model=request_model)
|
||||
)
|
||||
|
||||
assert client.messages.last_params["model"] == expected_model
|
||||
if expected_model == "claude-opus-4-8":
|
||||
assert client.messages.last_params["thinking"] == {"type": "adaptive"}
|
||||
assert "temperature" not in client.messages.last_params
|
||||
assert provider.get_default_model() == expected_default
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_generate_collects_text_and_tool_use_blocks() -> None:
|
||||
provider = make_provider(
|
||||
|
||||
@@ -73,6 +73,38 @@ def _openai_response(**overrides) -> SimpleNamespace:
|
||||
return SimpleNamespace(**defaults)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
("default_model", "request_model", "expected_model"),
|
||||
[
|
||||
(None, None, "gpt-4o-mini"),
|
||||
(None, "gpt-4o", "gpt-4o"),
|
||||
("gpt-4o", None, "gpt-4o"),
|
||||
("gpt-4o", "gpt-4-turbo", "gpt-4-turbo"),
|
||||
],
|
||||
)
|
||||
def test_openai_generate_respects_configured_default_and_request_model(
|
||||
default_model: str | None, request_model: str | None, expected_model: str
|
||||
) -> None:
|
||||
provider_options = {"api_key": "test"}
|
||||
if default_model is not None:
|
||||
provider_options["default_model"] = default_model
|
||||
provider = OpenAIProvider(**provider_options)
|
||||
client = _OpenAIClient()
|
||||
provider.client = client
|
||||
expected_default = default_model or "gpt-4o-mini"
|
||||
|
||||
assert provider.get_default_model() == expected_default
|
||||
|
||||
provider.generate(
|
||||
LLMInput(messages=[Message(role=Role.USER, content="hi")], model=request_model)
|
||||
)
|
||||
|
||||
assert client.completions.params is not None
|
||||
assert client.completions.params["model"] == expected_model
|
||||
assert provider.get_default_model() == expected_default
|
||||
|
||||
|
||||
def test_openai_provider_serializes_tools_for_chat_completions():
|
||||
provider = OpenAIProvider(api_key="test")
|
||||
client = _OpenAIClient()
|
||||
|
||||
+181
-1
@@ -1,6 +1,11 @@
|
||||
import io
|
||||
import json
|
||||
from unittest.mock import Mock
|
||||
|
||||
import pytest
|
||||
|
||||
from llm.core.types import ProviderType
|
||||
from llm.cli.selector import save_config
|
||||
from llm.core.types import LLMInput, Message, ProviderType, Role
|
||||
from llm.providers import (
|
||||
AstraflowCNProvider,
|
||||
AstraflowProvider,
|
||||
@@ -9,6 +14,7 @@ from llm.providers import (
|
||||
OllamaProvider,
|
||||
OpenAIProvider,
|
||||
get_provider,
|
||||
resolver,
|
||||
)
|
||||
|
||||
|
||||
@@ -36,6 +42,7 @@ class TestGetProvider:
|
||||
def test_get_astraflow_cn_provider(self):
|
||||
provider = get_provider("astraflow_cn")
|
||||
assert isinstance(provider, AstraflowCNProvider)
|
||||
|
||||
assert provider.provider_type == ProviderType.ASTRAFLOW_CN
|
||||
|
||||
def test_get_atlas_provider(self):
|
||||
@@ -100,3 +107,176 @@ class TestGetProvider:
|
||||
provider = get_provider()
|
||||
|
||||
assert isinstance(provider, AstraflowCNProvider)
|
||||
|
||||
|
||||
class TestSelectedModel:
|
||||
@pytest.fixture(autouse=True)
|
||||
def isolated_config(self, monkeypatch, tmp_path):
|
||||
monkeypatch.chdir(tmp_path)
|
||||
for name in (
|
||||
"LLM_PROVIDER", "LLM_MODEL", "OLLAMA_MODEL", "ATLAS_MODEL",
|
||||
"ASTRAFLOW_MODEL", "ASTRAFLOW_CN_MODEL",
|
||||
):
|
||||
# save_config(persist=True) writes os.environ directly; restore it after each test.
|
||||
monkeypatch.setenv(name, "")
|
||||
monkeypatch.delenv(name)
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key")
|
||||
monkeypatch.setenv("OPENAI_API_KEY", "test-key")
|
||||
|
||||
@pytest.mark.parametrize("persist", [False, True])
|
||||
def test_selector_model_reaches_actual_request(self, monkeypatch, persist):
|
||||
save_config("ollama", "mistral", persist=persist)
|
||||
urlopen = Mock(side_effect=lambda *args, **kwargs: io.BytesIO(
|
||||
b'{"message":{"content":"ok"},"model":"mistral"}'
|
||||
))
|
||||
monkeypatch.setattr("urllib.request.urlopen", urlopen)
|
||||
|
||||
provider = get_provider()
|
||||
provider.generate(LLMInput(messages=[Message(Role.USER, "hello")]))
|
||||
|
||||
assert provider.get_default_model() == "mistral"
|
||||
assert json.loads(urlopen.call_args.args[0].data)["model"] == "mistral"
|
||||
|
||||
provider.generate(LLMInput(messages=[], model="codellama"))
|
||||
assert json.loads(urlopen.call_args.args[0].data)["model"] == "codellama"
|
||||
|
||||
@pytest.mark.parametrize("provider_type", list(ProviderType))
|
||||
@pytest.mark.parametrize("source", ["saved", "environment"])
|
||||
def test_selected_model_applies_to_every_builtin(self, monkeypatch, provider_type, source):
|
||||
save_config(provider_type.value, "Selected/MixedCase-model", persist=source == "environment")
|
||||
|
||||
provider = get_provider()
|
||||
|
||||
assert provider.provider_type == provider_type
|
||||
assert provider.get_default_model() == "Selected/MixedCase-model"
|
||||
|
||||
def test_environment_model_overrides_saved_model(self, monkeypatch):
|
||||
save_config("ollama", "mistral")
|
||||
monkeypatch.setenv("LLM_MODEL", "codellama")
|
||||
|
||||
assert get_provider().get_default_model() == "codellama"
|
||||
|
||||
@pytest.mark.parametrize("provider_type,model_env", [
|
||||
("ollama", "OLLAMA_MODEL"),
|
||||
("atlas", "ATLAS_MODEL"),
|
||||
("astraflow", "ASTRAFLOW_MODEL"),
|
||||
("astraflow_cn", "ASTRAFLOW_CN_MODEL"),
|
||||
("astraflow_cn", "ASTRAFLOW_MODEL"),
|
||||
])
|
||||
def test_provider_environment_retains_precedence(self, monkeypatch, provider_type, model_env):
|
||||
save_config(provider_type, "saved-model")
|
||||
monkeypatch.setenv("LLM_MODEL", "global-model")
|
||||
monkeypatch.setenv(model_env, "provider-model")
|
||||
|
||||
assert get_provider().get_default_model() == "provider-model"
|
||||
|
||||
def test_cn_specific_environment_overrides_shared_fallback(self, monkeypatch):
|
||||
save_config("astraflow_cn", "saved-model")
|
||||
monkeypatch.setenv("ASTRAFLOW_MODEL", "shared-model")
|
||||
monkeypatch.setenv("ASTRAFLOW_CN_MODEL", "cn-model")
|
||||
|
||||
assert get_provider().get_default_model() == "cn-model"
|
||||
|
||||
@pytest.mark.parametrize("provider_type", list(ProviderType))
|
||||
def test_explicit_default_model_overrides_selection(self, monkeypatch, provider_type):
|
||||
save_config(provider_type.value, "saved-model", persist=True)
|
||||
monkeypatch.setenv("OLLAMA_MODEL", "provider-model")
|
||||
|
||||
assert get_provider(default_model="explicit-model").get_default_model() == "explicit-model"
|
||||
|
||||
@pytest.mark.parametrize("default_model", [None, ""])
|
||||
def test_explicit_empty_default_retains_constructor_fallback(self, monkeypatch, default_model):
|
||||
save_config("ollama", "saved-model", persist=True)
|
||||
monkeypatch.setenv("OLLAMA_MODEL", "provider-model")
|
||||
|
||||
assert get_provider(default_model=default_model).get_default_model() == "provider-model"
|
||||
|
||||
@pytest.mark.parametrize("explicit", [False, True])
|
||||
def test_provider_override_does_not_inherit_saved_model(self, monkeypatch, explicit):
|
||||
save_config("openai", "gpt-selected")
|
||||
if explicit:
|
||||
provider = get_provider("ollama")
|
||||
else:
|
||||
monkeypatch.setenv("LLM_PROVIDER", "ollama")
|
||||
provider = get_provider()
|
||||
|
||||
assert provider.get_default_model() == "llama3.2"
|
||||
|
||||
def test_explicit_provider_does_not_inherit_another_environment_model(self, monkeypatch):
|
||||
monkeypatch.setenv("LLM_PROVIDER", "openai")
|
||||
monkeypatch.setenv("LLM_MODEL", "gpt-selected")
|
||||
|
||||
assert get_provider("ollama").get_default_model() == "llama3.2"
|
||||
|
||||
@pytest.mark.parametrize("env_provider", [None, " 'OLLAMA' "])
|
||||
def test_environment_model_applies_to_matching_explicit_provider(self, monkeypatch, env_provider):
|
||||
if env_provider is not None:
|
||||
monkeypatch.setenv("LLM_PROVIDER", env_provider)
|
||||
monkeypatch.setenv("LLM_MODEL", ' "MixedCase/Model" ')
|
||||
|
||||
assert get_provider("ollama").get_default_model() == "MixedCase/Model"
|
||||
|
||||
@pytest.mark.parametrize("source", ["saved", "environment"])
|
||||
@pytest.mark.parametrize("model", ["", " ", '""'])
|
||||
def test_empty_selected_model_retains_default(self, monkeypatch, source, model):
|
||||
save_config("ollama", model, persist=source == "environment")
|
||||
|
||||
assert get_provider().get_default_model() == "llama3.2"
|
||||
|
||||
def test_empty_environment_model_falls_back_to_saved_model(self, monkeypatch):
|
||||
save_config("ollama", "mistral")
|
||||
monkeypatch.setenv("LLM_MODEL", " ")
|
||||
|
||||
assert get_provider().get_default_model() == "mistral"
|
||||
|
||||
def test_quoted_saved_model_preserves_case(self, tmp_path):
|
||||
tmp_path.joinpath(".llm.env").write_text(
|
||||
"LLM_PROVIDER='OLLAMA'\nLLM_MODEL='MixedCase/Model'\n"
|
||||
)
|
||||
|
||||
assert get_provider().get_default_model() == "MixedCase/Model"
|
||||
|
||||
def test_unpaired_saved_model_does_not_override_default_provider(self, tmp_path):
|
||||
tmp_path.joinpath(".llm.env").write_text("LLM_MODEL=orphan-model\n")
|
||||
|
||||
assert get_provider().get_default_model() == "claude-sonnet-4-6"
|
||||
|
||||
@pytest.mark.parametrize("explicit", [False, True])
|
||||
def test_provider_override_does_not_read_invalid_saved_file(self, monkeypatch, tmp_path, explicit):
|
||||
tmp_path.joinpath(".llm.env").write_bytes(b"\xff\xfe")
|
||||
monkeypatch.setenv("LLM_MODEL", "mistral")
|
||||
if explicit:
|
||||
provider = get_provider("ollama")
|
||||
else:
|
||||
monkeypatch.setenv("LLM_PROVIDER", "ollama")
|
||||
provider = get_provider()
|
||||
|
||||
assert provider.get_default_model() == "mistral"
|
||||
|
||||
def test_registered_provider_keeps_its_constructor_contract(self, monkeypatch):
|
||||
class CustomProvider(OllamaProvider):
|
||||
def __init__(self, base_url=None):
|
||||
super().__init__(base_url=base_url, default_model="custom-default")
|
||||
|
||||
monkeypatch.setattr(resolver, "_PROVIDER_MAP", resolver._PROVIDER_MAP.copy())
|
||||
resolver.register_provider(ProviderType.OLLAMA, CustomProvider)
|
||||
save_config("ollama", "mistral", persist=True)
|
||||
|
||||
provider = get_provider(base_url="http://localhost:1234")
|
||||
|
||||
assert isinstance(provider, CustomProvider)
|
||||
assert provider.base_url == "http://localhost:1234"
|
||||
assert provider.get_default_model() == "custom-default"
|
||||
|
||||
def test_registered_provider_receives_explicit_default_model(self, monkeypatch):
|
||||
class CustomProvider(OllamaProvider):
|
||||
pass
|
||||
|
||||
monkeypatch.setattr(resolver, "_PROVIDER_MAP", resolver._PROVIDER_MAP.copy())
|
||||
resolver.register_provider(ProviderType.OLLAMA, CustomProvider)
|
||||
save_config("ollama", "mistral", persist=True)
|
||||
|
||||
provider = get_provider(default_model="custom-explicit")
|
||||
|
||||
assert isinstance(provider, CustomProvider)
|
||||
assert provider.get_default_model() == "custom-explicit"
|
||||
|
||||
Reference in New Issue
Block a user