Enforce batch atomicity for condenser#775
Conversation
Coverage Report •
|
||||||||||||||||||||
...ts/anthropic_claude_haiku_4_5_20251001_agent-sdk-integration_N7_20251017_093024/results.json
Outdated
Show resolved
Hide resolved
enyst
left a comment
There was a problem hiding this comment.
I have a concern here, I think that both Claude and GPT models may crash if we repeat the same reasoning block... but I could be wrong. Do we have a log with parallel tool calls on the PR branch?
Yep, it's in the associated issue. |
| batch are forgotten. | ||
|
|
||
| This prevents partial batches from being sent to the LLM, which can cause | ||
| API errors when thinking blocks are separated from their tool calls. |
There was a problem hiding this comment.
Just a thought: I think maybe the action events are in the order we received tool calls from the LLM? If so, maybe we could check in a simpler way, if the event(s) just before forgotten_event_ids have the same response_id as... the first of those forgotten?
Really just a thought, it's all good with checking this way too
| # Enforce batch atomicity: if any event in a multi-action batch is forgotten, | ||
| # forget all events in that batch to prevent partial batches with thinking | ||
| # blocks separated from their tool calls | ||
| forgotten_event_ids = View._enforce_batch_atomicity(events, forgotten_event_ids) |
There was a problem hiding this comment.
I think it might make sense to call this after we call filter_unmatched_tool_calls? Not sure if it's even possible for that function to break up a batch but maybe better safe than sorry.
There was a problem hiding this comment.
I think it makes more sense to do this event removal before filter_unmatched_tool_calls, since in the _enforce_batch_atomicity we only remove actions, not observations. Maybe putting the filter_unmatched_tool_calls at the end can help us remove some left-over observations that doesn't have a corresponding action.
There was a problem hiding this comment.
Maybe I'll go with this for now, we can reconsider this later if we run into issues!
There was a problem hiding this comment.
Yeah, makes sense to me.
csmith49
left a comment
There was a problem hiding this comment.
Modulo a few minor suggestions, I think this is good to go!
thinking_blocks when splitting from Message into actionsCo-authored-by: Xingyao Wang <xingyao@all-hands.dev>
Fix #738
Integration tests: 7/7 (100%)
Agent Server images for this PR
• GHCR package: https://github.com/All-Hands-AI/agent-sdk/pkgs/container/agent-server
Variants & Base Images
golang:1.21-bookwormeclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22Pull (multi-arch manifest)
Run
All tags pushed for this build
The
14e6b9atag is a multi-arch manifest (amd64/arm64); your client pulls the right arch automatically.