feat(model-routing): thread per-call model through InvokeModelStage#7
feat(model-routing): thread per-call model through InvokeModelStage#7JackYPCOnline wants to merge 2 commits into
Conversation
Add a required model field to InvokeModelContext, initialized from agent.model, and make the invoke-model terminal stream the context model instead of reading agent.model directly. This lets invoke-stage middleware redirect a single model call without mutating shared agent state, the seam model routing builds on. Behavior is unchanged for existing single-model agents. Refs strands-agents#364
| tool_specs: list[ToolSpec] | ||
| tool_choice: ToolChoice | None | ||
| invocation_state: dict[str, Any] | ||
| model: Model |
There was a problem hiding this comment.
Issue: The docstring here is updated nicely, but src/strands/_middleware/README.md — the reference doc for middleware authors — is now stale. Its "Defensive copies" section enumerates the context fields (messages, system_prompt, tool_specs, tool_choice) and states that model state "is excluded from the context entirely … The terminal reads it directly from the agent at invocation time." Neither the new model field nor its override capability is mentioned, so authors won't discover routing from the docs.
Suggestion: Add model to the README's field discussion and note that middleware may replace it via dataclasses.replace() to redirect a single call. (Important)
|
Issue: The description (and Suggestion: Land the design doc (next free number appears to be |
|
Assessment: Comment (approve-worthy once docs are squared away) Clean, well-scoped seam: threading Review Categories
Nice, minimal seam that sets up the routing work well. |
| try: | ||
| async for event in stream_messages( | ||
| agent.model, | ||
| ctx.model, |
There was a problem hiding this comment.
Issue / question (re-posting here since my earlier comment targeted an unchanged line): Now that the terminal streams ctx.model, note that just below this call it still streams against model_state — a snapshot of agent._model_state, which belongs to the original agent.model. When middleware overrides ctx.model (the whole point of this seam), that snapshot could be semantically wrong for the redirected model, and it's written back to agent._model_state after the chain.
This is inert today since nothing overrides the model yet, so it's not blocking this PR. But please capture it as a known gap for the ModelRouter follow-up so it isn't silently inherited: is the intent that routed models are stateless, or should model_state become per-model when routing lands? (Important — follow-up)
|
Superseded by the upstream PR against strands-agents/harness-sdk. |
Description
First slice of model routing (design
team/designs/0015-model-routing.md): the per-call model plumbing that later routing middleware builds on.InvokeModelStage's terminal previously readagent.modeldirectly, so nothing could redirect an individual model call without mutating shared agent state (which is unsafe under concurrent invocations). This PR threads the model to invoke through the middleware context instead:InvokeModelContextgains a requiredmodel: Model, initialized fromagent.model.ctx.model(and reads itsmodel_idfor the span) rather thanagent.model.No behavior change for existing single-model agents. This is intentionally a standalone, inert seam; the
ModelRoutertype and strategies land in follow-up PRs.Public API Changes
InvokeModelContext(consumed by invoke-stage middleware authors) now carries the concrete model for the call, which middleware may replace to redirect a single invocation:Related Issues
Refs strands-agents#364
Type of Change
New feature
Testing
agent.modelby default; a middleware overridingctx.modelredirects the streamed model.hatch test); format and lint clean.Checklist