Skip to content

feat(model-routing): thread per-call model through InvokeModelStage#7

Closed
JackYPCOnline wants to merge 2 commits into
mainfrom
feature/model-routing
Closed

feat(model-routing): thread per-call model through InvokeModelStage#7
JackYPCOnline wants to merge 2 commits into
mainfrom
feature/model-routing

Conversation

@JackYPCOnline

Copy link
Copy Markdown
Owner

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 read agent.model directly, 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:

  • InvokeModelContext gains a required model: Model, initialized from agent.model.
  • The invoke-model terminal streams ctx.model (and reads its model_id for the span) rather than agent.model.

No behavior change for existing single-model agents. This is intentionally a standalone, inert seam; the ModelRouter type 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:

async def route(context, next_fn):
    context = replace(context, model=my_other_model)  # this call streams my_other_model
    async for event in next_fn(context):
        yield event

Related Issues

Refs strands-agents#364

Type of Change

New feature

Testing

  • Added tests: context exposes agent.model by default; a middleware overriding ctx.model redirects the streamed model.
  • Full Python unit suite passes locally (hatch test); format and lint clean.

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my feature works
  • My changes generate no new warnings

Draft: opening early for review of the routing seam before ModelRouter lands.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@github-actions

Copy link
Copy Markdown

Issue: The description (and AGENTS.md, which says design docs are where the "why" for a major subsystem lives) reference team/designs/0015-model-routing.md, but that file doesn't exist in the repo — and 0015 is already taken by 0015-bidi-webrtc-design.md. So the traceability link for this routing seam currently resolves to nothing / the wrong doc.

Suggestion: Land the design doc (next free number appears to be 0016) or fix the reference before merge, so this "first slice" and the follow-up ModelRouter PRs point at real design context. (Minor)

@github-actions

Copy link
Copy Markdown

Assessment: Comment (approve-worthy once docs are squared away)

Clean, well-scoped seam: threading model through InvokeModelContext instead of reading agent.model in the terminal is the right move for concurrent-safe per-call routing, and the terminal consistently uses ctx.model for both the span model_id and the stream. Tests are solid — full-list equality plus model_a.stream.assert_not_called() verify both the default and override paths. Feedback is about documentation/traceability and a forward-looking design gap, not the code itself.

Review Categories
  • Documentation: The middleware README.md (the author-facing reference) wasn't updated to mention the new model field or its override capability, and the referenced design doc 0015-model-routing.md is missing / number-collides with the bidi design.
  • Design (follow-up): An overridden ctx.model still streams against the original model's _model_state snapshot — inert today, but a gap to resolve when ModelRouter lands.
  • Correctness / tests: No issues; changes are consistent and covered, and dataclass field ordering is correct.
  • Scope: One unrelated RuntimeError message reflow snuck in, but it's trivial.

Nice, minimal seam that sets up the routing work well.

try:
async for event in stream_messages(
agent.model,
ctx.model,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@JackYPCOnline

Copy link
Copy Markdown
Owner Author

Superseded by the upstream PR against strands-agents/harness-sdk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size/s

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant