-
Notifications
You must be signed in to change notification settings - Fork 7
Description
Problem statement
I'm just diving into using this SDK to add a compatibility layer to an agent service for ACP clients, and encountered a few initial difficulties with regards to the schema that felt could be improved.
First is that when instantiating classes that have a required field and only one possible value, that field shouldn't need to be provided, it should default. For example, when I construct AgentMessageChunk, the session_update field only has one possible value, "agent_message_chunk". It's a required field, and it cannot be anything else. So why do I have to provide it? I understand that it's used as the discriminator when deserializing, but when AgentMessageChunk is constructed explicitly, it shouldn't be needed. This is a common pattern, not exclusive to AgentMessageChunk.
However if these fields are given a default such as:
class AgentMessageChunk(ContentChunk):
session_update: Annotated[Literal["agent_message_chunk"], Field(alias="sessionUpdate")] = "agent_message_chunk"
This then leads to another issue, in that when the SDK serializes the message, it's omitted because of exclude_defaults=True in
Line 33 in a09e090
| return params.model_dump(by_alias=True, exclude_none=True, exclude_defaults=True) |
Now after going through the examples, I noticed that you provide helpers such as update_agent_message for addressing this. But IMHO these should not be necessary. When I look at a function or class, and I see that it accepts some type, I shouldn't have to go hunting through the source code to see if there's a helper that is supposed to be used for construction of that type. And while I would argue that the type shouldn't need a helper at all, and that types should be simple to instantiate directly, if there is going to be a helper, I think it would be better for discoverability to put the helper as a static/class method on the class itself. Then if you want to expose all the helpers in some top level package for ease of import, you would declare local variables which reference the static method.
Proposed solution
- Change all types that have a required parameter with only a single possible value to default to that value.
- Change
params.model_dumpto useexclude_defaults=False.- A little more work, but if you still want to use
exclude_defaults, then implement a custom serializer that does exclude defaults, but only when the field is optional.
- A little more work, but if you still want to use
- Remove the helpers.
Alternatives considered
Make the helpers easier to discover by putting them as static methods on the class they construct.
Additional context
No response
Can you help build it?
- I can contribute code or docs for this request.