-
Notifications
You must be signed in to change notification settings - Fork 593
feat: pass invocation_state to model providers #1414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: pass invocation_state to model providers #1414
Conversation
Enables custom model providers to access invocation_state from agent
calls via kwargs. This supports use cases like custom request metadata,
tracing context, and provider-specific configuration.
Changes:
- Add invocation_state parameter to stream_messages()
- Pass invocation_state through event_loop to model.stream()
- Update BedrockModel to extract and forward kwargs
Custom providers can access via: kwargs.get('invocation_state')
| *, | ||
| tool_choice: ToolChoice | None = None, | ||
| system_prompt_content: Optional[list[SystemContentBlock]] = None, | ||
| **kwargs: Any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I am generally inclined.
The one change I would ask for is for us to make it explicit that invocation_state is being passed. So that would mean adding the invocation_state: Optional[dict[str, Any]] = None on stream and then passing it explicitly to the private methods.
I do not think we need kwargs on the private methods. Th reason we have it on stream is for extensibility without breaking backwards compatibilty. For private methods, we are not breaking anyone by adding a new parameter.
Also, this PR seems fairly small would you be able to update the other mode providers too?
Thanks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
457ad1d to
4cd7fdf
Compare
|
@tirth14, I made some modifications here as I play around with addressing your request. I have reverted back to the original state though Passing invocation state to the models makes sense for me in terms of extensibility. This allows custom implementations of the interface to leverage it as they see fit. But, what was proposed in 230e6d8 seems like it might not be the right fix. You are passing the invocation_state to This seems like the wrong approach. For 1, this is a private method. So if you are overriding this it means we probably want some other mechanism for extensibility. For 2, passing unrelated and otherwise unused parameters to a private method doesn't make sense. The only reason to do that would be if you believe that method might be overriden, which I see as problematic and related to #1. I think I have a good compromise proposal though. If we begin passing in invocation_state, and I'm correct about what you are doing with Does this more limited change work for you? |
|
Hi @dbschmigelski, thank you for the review. Your changes makes sense to me. That would be a good compromise for now which would unblock us to access Sharing more context behind passing it through I understand overriding the private method |
|
Hi regarding,
Yes, please create a new issue and lets discuss there! The overriding of the private method is flaky because we may unintentionally break you if we, for example, removed or refactored that method. But, the fact that you feel the need to override it in the first place means we likely a use case, beyond just you, where customizing the model implementation would be useful. Let me get a commit out with the interface-only change and happy to work through what a more customizable BedrockModel looks like! |
Enables custom model providers to access invocation_state from agent calls via kwargs. This supports use cases like custom request metadata, tracing context, and provider-specific configuration.
Changes:
Custom providers can access via: kwargs.get('invocation_state')
Description
This PR enables custom model providers to receive
invocation_statefrom agent invocations. Theinvocation_stateparameter flows from the agent call through the event loop to the model'sstream()method, where custom providers can access it viakwargs.get('invocation_state').This supports use cases such as:
The implementation is fully backward compatible - the abstract
Model.stream()already accepts**kwargs, so no changes are needed to the base interface.Related Issues
N/A
Documentation PR
N/A - No documentation changes needed as this extends existing
**kwargsfunctionalityType of Change
New feature
Testing
How have you tested the change?
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.