Fix Agents to support code and rag simultaneously #908
Conversation
hardikjshah
requested review from
ashwinb,
yanxi0830,
dltn,
raghotham,
dineshyv,
vladimirivic and
sixianyi0721
as code owners
facebook-github-bot
added
the
CLA Signed
| @@ -476,9 +476,6 @@ async def _run( | |||
| ) | |||
| span.set_attribute("output", retrieved_context) | |||
| span.set_attribute("tool_name", MEMORY_QUERY_TOOL) | |||
| if retrieved_context: | |||
| last_message = input_messages[-1] | |||
| last_message.context = retrieved_context | |||
There was a problem hiding this comment.
is there another place where we set the context for RAG?
There was a problem hiding this comment.
yeah how does RAG even work if you don't do this?
There was a problem hiding this comment.
yes, all tests were passing for RAG but then i realized there was nothing getting added to the context.
I tested this change and now can see RAG work properly.
|
|
||
| # append retrieved_context to the last user message | ||
| for message in input_messages[::-1]: | ||
| if isinstance(message, UserMessage): |
There was a problem hiding this comment.
What other message type could this be if it's triggering RAG?
Separately curious why for RAG results we use .context but for other tool execs we do 708 input_messages = input_messages + [message, result_message]
There was a problem hiding this comment.
@ehhuang I think @hardikjshah did this so you could identify which message you added context to and then in the next turn get rid of it. we needed to keep only one context around as the turns proceeded.
I really think we should nuke that .context field completely and manage whatever state we need to manage completely within agent_instance.py (and ensure the invariant above)
There was a problem hiding this comment.
What other message type could this be if it's triggering RAG?
That was the original bug , where if both code and rag are enabled, we end up with a ToolResponseMessage coming from the code_interpreter side (check out def handle_documents)
And it was erroring since ToolResponseMessage does not have any attr context.
I agree that we should find a better solution to context. May be RAG responses are also passed in the ToolResponseMessage like all other tools. Although outside the scope of this PR.
What does this PR do?
Fixes a bug where agents were not working when both rag and code-interpreter were added as tools.
Test Plan
Added a new client_sdk test which tests for this scenario