fix: concatenate list values in deep_merge_dicts during parallel tool call merge - #5191
fix: concatenate list values in deep_merge_dicts during parallel tool call merge#5191enjoykumawat wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Please move the tests to test_functions_parallel.py
|
Done! Moved all the |
|
Hi @wuliang229, just following up — the tests have been moved to |
rohityan
added
needs review
… call merge Merge #5191 ## Summary - **Bug:** When multiple tool calls run in parallel and each writes to the same `state_delta` key containing a list value, `deep_merge_dicts` silently drops all but the last value because lists hit the `else` branch and get overwritten. - **Fix:** Add a list-type check in `deep_merge_dicts` so list values are concatenated (`d1[key] + value`) instead of overwritten, preserving all entries from parallel function responses. - **Tests:** Added 5 unit tests covering list concatenation, scalar overwrite, nested dict merge, mixed-type handling, and an integration test for `merge_parallel_function_response_events` with `state_delta` lists. ## Reproduction ```python # Tool A's delta: {"state_delta": {"items": ["a"]}} # Tool B's delta: {"state_delta": {"items": ["b"]}} # Before fix: {"state_delta": {"items": ["b"]}} — item "a" is lost # After fix: {"state_delta": {"items": ["a", "b"]}} — both preserved ``` ## Test plan - [x] All 5 new unit tests pass (`test_deep_merge_dicts_*` and `test_merge_parallel_function_response_events_merges_state_delta_lists`) - [x] All 32 existing tests in `test_functions_simple.py` pass (0 regressions) - [ ] Manual: create parallel tool calls that accumulate list state and verify no data loss Fixes #5190 Co-authored-by: George Weale <gweale@google.com> COPYBARA_INTEGRATE_REVIEW=#5191 from enjoykumawat:fix/parallel-state-delta-list-merge 7425481 PiperOrigin-RevId: 936119923
|
Thank you @enjoykumawat for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 1ff84eb. Closing this PR as the changes are now in the main branch. |
… call merge Merge #5191 ## Summary - **Bug:** When multiple tool calls run in parallel and each writes to the same `state_delta` key containing a list value, `deep_merge_dicts` silently drops all but the last value because lists hit the `else` branch and get overwritten. - **Fix:** Add a list-type check in `deep_merge_dicts` so list values are concatenated (`d1[key] + value`) instead of overwritten, preserving all entries from parallel function responses. - **Tests:** Added 5 unit tests covering list concatenation, scalar overwrite, nested dict merge, mixed-type handling, and an integration test for `merge_parallel_function_response_events` with `state_delta` lists. ## Reproduction ```python # Tool A's delta: {"state_delta": {"items": ["a"]}} # Tool B's delta: {"state_delta": {"items": ["b"]}} # Before fix: {"state_delta": {"items": ["b"]}} — item "a" is lost # After fix: {"state_delta": {"items": ["a", "b"]}} — both preserved ``` ## Test plan - [x] All 5 new unit tests pass (`test_deep_merge_dicts_*` and `test_merge_parallel_function_response_events_merges_state_delta_lists`) - [x] All 32 existing tests in `test_functions_simple.py` pass (0 regressions) - [ ] Manual: create parallel tool calls that accumulate list state and verify no data loss Fixes #5190 PiperOrigin-RevId: 936249245
|
Hi @GWeale — I see this was reopened after the Copybara merge (commit |
… call merge When multiple tool calls run in parallel and each writes to the same state_delta key containing a list value, deep_merge_dicts silently drops all but the last value because lists hit the else branch and get overwritten. Add a list-type check so that list values are concatenated instead of overwritten, preserving all entries from parallel function responses. Fixes google#5190
…nctions_parallel Move tests for deep_merge_dicts and merge_parallel_function_response_events with list state_delta merging from test_functions_simple.py to test_functions_parallel.py per reviewer feedback.
7425481 to
e625f51
Compare
|
Quick question before this gets another look — digging into the Copybara history here, I see this was actually merged internally ( Was there a concern found internally with this approach (the list-concatenation logic in |
5 participants
Summary
state_deltakey containing a list value,deep_merge_dictssilently drops all but the last value because lists hit theelsebranch and get overwritten.deep_merge_dictsso list values are concatenated (d1[key] + value) instead of overwritten, preserving all entries from parallel function responses.merge_parallel_function_response_eventswithstate_deltalists.Reproduction
Test plan
test_deep_merge_dicts_*andtest_merge_parallel_function_response_events_merges_state_delta_lists)test_functions_simple.pypass (0 regressions)Fixes #5190