refactor: reorganize model providers with subpath exports and rename classes#694
refactor: reorganize model providers with subpath exports and rename classes#694pgrayy wants to merge 2 commits intostrands-agents:mainfrom
Conversation
Code Review SummaryAssessment: Approve ✅ Well-structured refactoring that establishes a clear naming convention for model providers ( Review Notes
Nice work on the comprehensive rename and the forward-thinking naming convention! |
|
Thanks for addressing the feedback! The unused import has been removed. All previous review comments have been addressed - this looks good to merge. 🚀 |
|
Note, to shorten the names, a pattern we could promote instead is something like import {ConverseModel as BedrockConverseModel} from '@strands-agents/sdk/models/bedrock'
...
model = new BedrockConverseModelor import {* as bedrock} from '@strands-agents/sdk/models/bedrock'
...
model = new bedrock.ConverseModel(...) |
| "./gemini": { | ||
| "types": "./dist/src/models/gemini/model.d.ts", | ||
| "default": "./dist/src/models/gemini/model.js" | ||
| "./models/google": { |
There was a problem hiding this comment.
Do we need the models prefix here? Is it for tree shaking?
There was a problem hiding this comment.
It's just for namespacing:
import { ChatModel } from '@strands-agents/sdk/models/openai'vs
import { ChatModel } from '@strands-agents/sdk/openai'Tree shaking works just the same.
4627722 to
c4ef5fe
Compare
Code Review SummaryAssessment: Request Changes There's a significant discrepancy between the PR description and the actual implementation that needs to be resolved. Critical Issue: PR Description vs Implementation MismatchThe PR description documents one naming convention, but the implementation uses a different one:
Action needed: Either update the PR description to match the implementation, or update the implementation to match the description. The PR description serves as documentation for future reference. Naming Convention ConsiderationsThe shorter names ( Pros:
Cons:
The original Once the PR description is updated to reflect the actual implementation (or vice versa), this will be ready for final review. |
|
|
||
| ```typescript | ||
| import { Agent, BedrockModel } from '@strands-agents/sdk' | ||
| import { Agent, ConverseModel } from '@strands-agents/sdk' |
There was a problem hiding this comment.
Issue: Import name doesn't match export
The main entry point exports BedrockConverseModel (aliased from ConverseModel), but this code uses ConverseModel which won't be found.
Suggestion: Update to use the exported name:
import { Agent, BedrockConverseModel } from '@strands-agents/sdk'
const model = new BedrockConverseModel({
// ...
})This also applies to lines 246 and 273 in the Graph and Swarm examples.
Code Review Summary (Updated)Assessment: Request Changes The PR description has been updated and now clearly explains the naming convention. However, there's an inconsistency in the README that needs to be fixed. Issue: README.md uses wrong import nameThe main entry point exports // README.md (current - incorrect)
import { Agent, ConverseModel } from '@strands-agents/sdk'
// Should be
import { Agent, BedrockConverseModel } from '@strands-agents/sdk'This affects lines 101, 246, and 273 in README.md. What's Good
Once the README.md imports are fixed to use |
…classes - Rename BedrockModel to BedrockConverseModel - Rename OpenAIModel to OpenAIChatModel - Rename AnthropicModel to AnthropicMessagesModel - Rename GeminiModel to GoogleGenAIModel - Move src/models/gemini/ to src/models/google/ - Add subpath exports: ./models/bedrock, ./models/openai, ./models/anthropic, ./models/google - Rename all config/options types to match class names - Update all tests, examples, docs, and READMEs
Rename model providers to shorter, API-focused names: - BedrockConverseModel -> ConverseModel - OpenAIChatModel -> ChatModel - AnthropicMessagesModel -> MessagesModel - GoogleGenAIModel -> GenAIModel All related types (Config, Options, etc.) renamed accordingly. Updated all source, tests, examples, and documentation.
5785500 to
3dafc1e
Compare
Code Review Summary (Follow-up)Assessment: Request Changes The README.md import issue from the previous review is still outstanding. Blocking Issue: README.md uses // src/index.ts exports:
export { ConverseModel as BedrockConverseModel } from './models/bedrock.js'
// README.md incorrectly uses:
import { Agent, ConverseModel } from '@strands-agents/sdk' // ❌ Won't workLines to fix in README.md:
Once this is fixed, the PR is ready to approve. Everything else looks good! |
Code Review SummaryAssessment: Approve ✅ The README.md import issue has been fixed. All imports now correctly use Review Summary
Note: There's still an open question from @Unshure about the Nice work on this well-documented refactor! |
71e7ff1 to
3b9b3c4
Compare
Motivation
The current model provider class names are generic (
BedrockModel,OpenAIModel, etc.) and the import paths sit at the package root (@strands-agents/sdk). As we add more providers and API variants (e.g., OpenAI Responses API alongside Chat Completions), generic names become ambiguous and root-level exports create a cluttered namespace.This PR does two things:
@strands-agents/sdk/models/<provider>), keeping the main entry point focused on core SDK types.@strands-agents/sdk/models/bedrockConverseModel@strands-agents/sdk/models/openaiChatModel@strands-agents/sdk/models/anthropicMessagesModel@strands-agents/sdk/models/googleGenAIModelThis convention scales naturally. Adding OpenAI Responses support later would be
ResponsesModelalongsideChatModelunder the same./models/openaisubpath with no naming collision.Why drop the provider prefix?
When a class lives behind a module path like
/models/bedrock, repeating "Bedrock" in the class name is redundant. The import path already tells you the provider; the class name should tell you the API.Node.js core modules follow this pattern. The subpath provides the scoping context, and the exports just describe what they do:
node:fs/promisesexportsreadFile,writeFile,mkdir, notfsReadFileorfsPromisesWriteFile. The module path carries the context.node:stream/webexportsReadableStream,WritableStream,TransformStream, notStreamReadableStreamorWebReadableStream.node:stream/consumersexportstext(),json(),buffer(), notstreamConsumersTextorstreamJson.node:path/posixandnode:path/win32both exportjoin,resolve,basename, notposixJoinorwin32Resolve. The subpath tells you the platform variant, the function name tells you the operation.That last one is a close analogy to what we are doing here.
path/posixandpath/win32are two implementations of the same interface, scoped by subpath. Similarly,models/bedrockandmodels/openaiare two implementations ofModel, scoped by subpath.The same short names are used everywhere, including the top-level re-export of the default Bedrock provider. There is no ambiguity at the top level because each provider's API name is already distinct (
ConverseModel,ChatModel,MessagesModel,GenAIModel). Users who want a longer form for clarity can alias on import:Or use namespace imports:
Public API Changes
Each provider now has a dedicated subpath export:
The Bedrock provider is also re-exported from the main
@strands-agents/sdkentry point since Bedrock is the default provider:Other providers are only available through their respective subpath exports.
Config and options types follow the same pattern (e.g.,
ConverseModelConfig,ChatModelOptions). The Gemini directory is renamed togoogle/and allGemini-prefixed SDK types becomeGenAI-prefixed, while upstream API references (model IDs, env vars) remain as "gemini."Since the SDK is still in preview, no backward compatibility aliases are provided.
Documentation PR
strands-agents/docs#682
Type of Change
Breaking change
Testing
npm run checkAll 1632 unit tests pass. Lint, format, and type-check all pass via pre-commit hooks.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Additional Considerations
This PR updates the naming to reflect how the model providers are implemented today. However, we could iterate on this and implement an aggregated provider that switches between APIs based on the configurations passed in. For example, we could one day expose an OpenAIModel and set it up to support both chat completions and responses based on configs or based on streaming options that are controlled dynamically. Nothing stops us from doing that. But at least with what we have implemented now, changing the name to reflect the implementation I think is beneficial.