-
Notifications
You must be signed in to change notification settings - Fork 420
feat: replace kwargs with invocation_state in agent APIs #966
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?
Conversation
Can you add details to the PR description explaining the purpose of this change? |
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.
Can you add unit tests for this
src/strands/agent/agent.py
Outdated
merged_state["invocation_state"] = invocation_state | ||
else: | ||
if invocation_state is not None: | ||
merged_state["invocation_state"] = invocation_state |
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.
aren't we double nesting here?
4. Backward compatibility breaks silently
Old code using agent("prompt", user_id="123") will work but
with a deprecation warning. However, if someone tries to migrate
by wrapping it: agent("prompt", invocation_state={"user_id": "123"}),
the data ends up in a different location in the state dict.
so when you try to use agent("prompt", invocation_state={"user_id": "123"})
, you would need to do this in the tool:
tool_context.invocation_state.invocation_state.user_id
is this the desired devx for now?
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.
yep, see comment
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.
I'm not sure I agree with this; see the Unit Test comment below. But the intent is that users would replace kwargs
with invocation_state
- but if they're already using kwargs, then we mimic the old behavior to avoid a breaking change, while nudging them towards invocation_state
Basically it should be something like:
if kwargs:
merged_state = kwargs
# emit warning
if invocation_state:
# If the user is using kwargs, that means they intended all arguments to be kwargs
merged_state["invocation_state'] = kwargs
else:
merged_state = invocation_state
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.
Adjusted.
if xxx:
........
else:
if invocation_state is not none: # Added this line, we are not accepting `None` now.
Actually this change is covered by origin test code but I can add a specific one |
Co-authored-by: Nick Clegg <nac542@gmail.com>
src/strands/agent/agent.py
Outdated
merged_state["invocation_state"] = invocation_state | ||
else: | ||
if invocation_state is not None: | ||
merged_state["invocation_state"] = invocation_state |
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.
I'm not sure I agree with this; see the Unit Test comment below. But the intent is that users would replace kwargs
with invocation_state
- but if they're already using kwargs, then we mimic the old behavior to avoid a breaking change, while nudging them towards invocation_state
Basically it should be something like:
if kwargs:
merged_state = kwargs
# emit warning
if invocation_state:
# If the user is using kwargs, that means they intended all arguments to be kwargs
merged_state["invocation_state'] = kwargs
else:
merged_state = invocation_state
…nds-agents#897) Replace sequential message loading with async concurrent reading in both S3SessionManager and FileSessionManager to improve performance for long conversations. Uses asyncio.gather() with run_in_executor() to read multiple messages simultaneously while maintaining proper ordering. Resolves: strands-agents#874 Co-authored-by: Vamil Gandhi <vamgan@amazon.com>
Description
The current Agent APIs (Agent.call, Agent.invoke_async, Agent.stream_async) accept arbitrary keyword arguments via **kwargs, which are then converted to invocation_state and passed through the execution pipeline to tools. This has the downside that any new parameter we want to add to these methods could potentially conflict with user-provided kwargs, making it impossible to evolve the API without breaking changes.
Change:
Add invocation_state as parameter to agent's APIs.
Still keep **kwargs for now to keep backward compatible, merge **kwargs to invocation_state instead.
Related Issues
#919
Documentation PR
Type of Change
Bug fix
New feature
Breaking change
Documentation update
Other (please describe):
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepare
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.