[PY] fix: #1705 - Output of multiple calls to same tool are overwritten #2228
Conversation
Add a sublevel to the list of tools called as part of a run (this list is stored on assistants_planner.py `tool_map:dict`), so that their result values are not overwritten by subsequent calls to the same tool within the same run. The current implementation did not account for multiple calls to the same tool as it did not keep track of the call id (`call.id`) which is different for each call, regardless of which tool. The list was only being indexed by `tool_id`. The result was that subsequent calls to the same tool would overwrite the result of the previous call. The fix consists on adding a second level to the dictionary of the tool calls (which contains the return values), so instead of being (pseudocode) `tools[tool_id]` it becomes `tools[tool_id][call_id]`. This way the return value of each and every call is retained.
andres-swax
requested review from
aacebo,
corinagum,
lilyydu,
singhk97 and
rajan-chari
as code owners
|
@microsoft-github-policy-service agree |
|
@andres-swax thanks for the contribution! Could you please fix the failing tests and add a new one for the multiple tool calls scenario? |
Hi, I'll try to do my best, I saw those failures yesterday and honestly, I could not understand a thing nor what I could do. I felt the failures were on the Python libraries and hoping the messages were just noise. My dev-ops software and workflow knowledge is still on Visual SourceSafe level or so. If I cannot make it work, i'll ask for the PR to be closed. On the meantime I'll give it a try for the next cpl of days. |
|
@andres-swax No rush, and we really appreciate your contribution! We may be able to keep the PR open and have someone help out the with tests, but I can't guarantee how soon that will be. Our bandwidth is pretty tight right now. Please add the tests if you can and feel free to ask questions! |
I tried to see how I can update the tests but tbh I couldn't figure out. If necessary please do close pR so the bug can be taken care of by someone else, it's kind of important to fix it soon. |
The current implementation of the planner did not account for multiple calls to the same tool since it did not keep track of the call id (
call.id) which is different for each call, regardless of which tool.Therefore, the list was only being indexed by
tool_id.The result was that subsequent calls to the same tool within the same run would overwrite the result of the previous call, and at the end of the run, results would be missing triggering an Exception.
The issue is fixed by adding a sublevel to the list of tools called as part of a run (this list is stored on assistants_planner.py
tool_map:dict) which does keep track of the unique call_id, so that their result values are not overwritten by subsequent calls to the same tool within the same run.Linked issues
closes: #1705
Details
In order to trigger this issue, submit a prompt that triggers multiple calls to the same tool on the same run (i.e. which city is warmer today, LA or Chicago?). The same tool would be called once for each city (i.e. get-weather) but the data returned would only contain results for one city. This results on an Exception being raised. Steps can be found on #1705.
Change details
Key change is that
state.temp.action_outputs[command.action_id]becomesstate.temp.action_outputs[command.action][command.action_id]on teams/ai/ai.py.Attestation Checklist
My code follows the style guidelines of this project
I have checked for/fixed spelling, linting, and other errors
I have commented my code for clarity
I have made corresponding changes to the documentation (updating the doc strings in the code is sufficient)
My changes generate no new warnings
I have added tests that validates my changes, and provides sufficient test coverage. I have tested with:
New and existing unit tests pass locally with my changes