feat: support all model providers in JSON agent configuration#2109
feat: support all model providers in JSON agent configuration#2109
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| _ENV_VAR_PATTERN = re.compile(r"^\$\{([^}]+)\}$|^\$([A-Za-z_][A-Za-z0-9_]*)$") | ||
|
|
||
| # Provider name to factory function mapping — populated at module level, lazy imports at call time | ||
| PROVIDER_MAP: dict[str, str] = { |
There was a problem hiding this comment.
Issue: PROVIDER_MAP maps provider names to string function names, which are then resolved via globals()[factory_name] on line 438. This pattern is fragile — it breaks silently if a function is renamed or removed, and bypasses static analysis tools and IDE navigation.
Suggestion: Map directly to function references instead:
PROVIDER_MAP: dict[str, Callable[[dict[str, Any]], Any]] = {
"bedrock": _create_bedrock_model,
"anthropic": _create_anthropic_model,
...
}This requires moving PROVIDER_MAP below the factory function definitions, but it gives you type safety, IDE go-to-definition, and eliminates the globals() lookup.
There was a problem hiding this comment.
This has been addressed in the latest revision. PROVIDER_MAP now maps to class name strings resolved via getattr(models, class_name), which leverages the existing lazy __getattr__ pattern in models/__init__.py. The factory logic has been moved to from_dict classmethods on each model class. This is a clean approach that works well with the lazy-loading architecture.
| return factory_fn(config) | ||
|
|
||
|
|
||
| def config_to_agent(config: str | dict[str, Any], **kwargs: dict[str, Any]) -> Any: |
There was a problem hiding this comment.
Issue: The type annotation for **kwargs is dict[str, Any], but **kwargs already captures keyword arguments as a dict — the annotation should be the value type, not the full dict type.
Suggestion:
def config_to_agent(config: str | dict[str, Any], **kwargs: Any) -> Any:There was a problem hiding this comment.
Fixed in the latest revision — **kwargs: Any is now used correctly.
| import jsonschema | ||
| from jsonschema import ValidationError | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Issue: logging is imported and logger is defined but never used anywhere in the module.
Suggestion: Either add logging calls (e.g., in _create_model_from_config for debugging provider instantiation) or remove the import and logger definition. Per the style guide, a useful log would be:
logger.debug("provider=<%s> | creating model from config", provider)There was a problem hiding this comment.
Addressed in the latest revision — the unused logging import has been removed.
| if client_args is not None: | ||
| kwargs["client_args"] = client_args | ||
| kwargs.update(config) | ||
| return AnthropicModel(**kwargs) |
There was a problem hiding this comment.
Issue: 7 of the 12 factory functions (_create_anthropic_model, _create_openai_model, _create_gemini_model, _create_litellm_model, _create_llamaapi_model, _create_writer_model, _create_openai_responses_model) share identical logic: pop client_args, build kwargs, update with remaining config, call constructor.
Suggestion: Extract a shared helper to eliminate the duplication:
def _create_client_args_model(model_cls: type, config: dict[str, Any]) -> Any:
"""Common factory for providers that accept client_args + **model_config."""
client_args = config.pop("client_args", None)
kwargs: dict[str, Any] = {}
if client_args is not None:
kwargs["client_args"] = client_args
kwargs.update(config)
return model_cls(**kwargs)Then each common provider becomes a one-liner with just the import and call. The unique providers (bedrock, ollama, mistral, llamacpp, sagemaker) keep their custom logic.
There was a problem hiding this comment.
Addressed in the latest revision. The common client_args pattern is now a default Model.from_dict classmethod on the base class, which 7 providers inherit. Only providers with non-standard constructors (Bedrock, Ollama, Mistral, LlamaCpp, SageMaker) override it. This eliminates the duplication completely.
| if "boto_client_config" in config: | ||
| raw = config.pop("boto_client_config") | ||
| kwargs["boto_client_config"] = BotocoreConfig(**raw) if isinstance(raw, dict) else raw | ||
| return SageMakerAIModel(**kwargs) |
There was a problem hiding this comment.
Issue: The SageMaker factory silently drops any extra config keys after popping endpoint_config, payload_config, and boto_client_config. For example, if a user passes model_id (a common pattern for other providers), it's silently ignored. All other factories pass remaining config through via kwargs.update(config).
Suggestion: Either pass remaining config to the constructor (if SageMaker accepts **kwargs) or explicitly warn/raise on unexpected keys so users get feedback when their config is wrong:
if config:
logger.warning("provider=<sagemaker> | ignoring unsupported config keys: %s", list(config.keys()))There was a problem hiding this comment.
Addressed in the latest revision. SageMakerAIModel.from_dict now raises ValueError with a clear message when unexpected config keys are present.
| _VALIDATOR = jsonschema.Draft7Validator(AGENT_CONFIG_SCHEMA) | ||
|
|
||
| # Pattern for matching environment variable references | ||
| _ENV_VAR_PATTERN = re.compile(r"^\$\{([^}]+)\}$|^\$([A-Za-z_][A-Za-z0-9_]*)$") |
There was a problem hiding this comment.
Issue: The regex ^\$\{([^}]+)\}$|^\$([A-Za-z_][A-Za-z0-9_]*)$ only matches full-string env var references (anchored with ^ and $). This means "prefix-$VAR-suffix" won't be resolved, which may surprise users coming from shell-like environments.
Suggestion: This is a reasonable design choice for security and simplicity, but it should be explicitly documented — either in the module docstring or as a comment near the pattern. Something like:
# Only full-string env var references are resolved (no inline interpolation).
# "prefix-$VAR" is NOT resolved; use the object format to construct values programmatically.There was a problem hiding this comment.
Addressed in the latest revision — there is now an inline comment above _ENV_VAR_PATTERN documenting this behavior.
|
Assessment: Comment Solid implementation that extends the experimental JSON agent configuration to support all 12 SDK model providers with backward compatibility. The main areas for improvement are reducing code duplication in the provider factory functions and replacing the fragile Review Categories
Good backward compatibility preservation, thorough test coverage, and clean lazy-import pattern across all providers. |
Unshure
force-pushed
the
agent-tasks/1064
branch
from
142bcb1 to
e3c44bf
Compare
- Fix **kwargs type annotation (dict[str, Any] -> Any) in config_to_agent - Add defensive copy in all from_dict methods to avoid mutating caller's dict - Raise ValueError on unsupported config keys in SageMaker from_dict - Improve _create_model_from_dict return type to Model - Document env var pattern full-string-only matching
| call_kwargs = mock_init.call_args[1] | ||
| assert isinstance(call_kwargs["boto_client_config"], BotocoreConfig) | ||
|
|
||
| def test_default_from_dict_client_args_pattern(self): |
There was a problem hiding this comment.
Issue: Dead code — the first with patch.object(BedrockModel, "__init__", ...) block on line 597 exits its context before anything useful happens, and mock_init is immediately reassigned. This appears to be a leftover from earlier refactoring.
Suggestion: Remove lines 595–598 (the BedrockModel import and its unused patch block).
| kwargs["boto_client_config"] = BotocoreConfig(**raw) if isinstance(raw, dict) else raw | ||
| if config: | ||
| unexpected = ", ".join(sorted(config.keys())) | ||
| raise ValueError(f"Unsupported SageMaker config keys: {unexpected}") |
There was a problem hiding this comment.
Issue: The new ValueError for unexpected SageMaker config keys is a good addition, but there's no test covering this error path.
Suggestion: Add a test (in the appropriate model test file) like:
def test_sagemaker_from_dict_rejects_unexpected_keys(self):
with pytest.raises(ValueError, match="Unsupported SageMaker config keys"):
SageMakerAIModel.from_dict({
"endpoint_config": {},
"payload_config": {},
"model_id": "unexpected",
})
| # ============================================================================= | ||
|
|
||
|
|
||
| class TestModelFromConfig: |
There was a problem hiding this comment.
Issue: TestModelFromConfig (lines 446–625) tests from_dict methods on model classes (BedrockModel, OllamaModel, MistralModel, LlamaCppModel, SageMakerAIModel, AnthropicModel) but lives in test_agent_config.py. Per AGENTS.md, unit tests should mirror the src/strands/ structure — these tests exercise model class methods, not agent_config functionality.
Suggestion: Move these tests to the corresponding model test files (tests/strands/models/test_bedrock.py, tests/strands/models/test_ollama.py, etc.). The TestCreateModelFromConfig class (lines 380–438) correctly belongs here since it tests the dispatch logic in agent_config.py.
|
Assessment: Comment The rework since the last review is excellent — all 6 prior issues have been addressed. Moving factory logic to Review Details
Clean architecture, solid backward compatibility, and thorough test coverage overall. |
1 participant
Motivation
The experimental
agent_config.pyfeature currently only accepts a simple string for themodelfield, which is always interpreted as a Bedrockmodel_id. This limits JSON-based agent configuration to a single provider, preventing use cases like agent-builder tools and theuse_agenttool from working with any of the SDK's 12 model providers.Resolves #1064
Public API Changes
The
modelfield in agent JSON configuration now supports two formats:Environment variable references (
$VARor${VAR}) in model config values are resolved automatically before provider instantiation, enabling secure configuration without embedding secrets.All 12 SDK providers are supported:
bedrock,anthropic,openai,gemini,ollama,litellm,mistral,llamaapi,llamacpp,sagemaker,writer,openai_responses. Each provider's constructor parameters are correctly routed — for example,boto_client_configdicts are converted toBotocoreConfigobjects for Bedrock/SageMaker, Ollama'sclient_argsmaps toollama_client_args, and Mistral'sapi_keyis extracted as a separate parameter.All provider imports are lazy to avoid requiring optional dependencies that aren't installed. Non-serializable parameters (
boto_session,client,gemini_tools) cannot be specified from JSON and are documented as such.Use Cases