refactor(agents): migrate to OpenAI chat completions API #3097
Conversation
meta-cla
bot
added
the
CLA Signed
aakankshaduggal
force-pushed
the
agents-openai-migration
branch
from
f558cb1 to
7c93b63
Compare
| response_format=self.agent_config.response_format, | ||
| # Convert messages to OpenAI format | ||
| openai_messages = [] | ||
| for message in input_messages: |
There was a problem hiding this comment.
I think if we are going to convert to openai responses we should also shift our inputs to match so we don't need to convert anything using the openai compat stuff.
you can look at the inference API for how it handles this with things like openai_chat_completion
| ) | ||
|
|
||
| # Convert OpenAI stream back to Llama Stack format | ||
| response_stream = convert_openai_chat_completion_stream( |
There was a problem hiding this comment.
yeah, same here, I wonder if we should return the openai format not the llama stack format. it looks to me here like we are taking llama stack input, converting to openai format, doing a request, and then converting back to llama stack format. I think to do a proper refactor here we should depend on the openai types
wdyt about this @aakankshaduggal ? I am open to anything here
| if self.tool_defs: | ||
| openai_tools = [] | ||
| for tool_def in self.tool_defs: | ||
| openai_tool = convert_tooldef_to_openai_tool(tool_def) |
There was a problem hiding this comment.
I could be wrong, but I think we should avoid conversion of any inputs and instead rely on openai native inputs? I will leave this up to others though since I don't have too much context here.
There was a problem hiding this comment.
I think this is a good first step, when changing things we can change things piecemeal. Then ripple changes outwards and do what you suggest. When things go outwards, you are going to make things backwards incompatible also, and that is something we have slated for 0.3.0.
@aakankshaduggal this is great and looks nearly ready perhaps? have you run the tests?
There was a problem hiding this comment.
ok makes sense, we can start with this approach as long as there is work to do the following changes!
Replace chat_completion calls with openai_chat_completion to eliminate dependency on legacy inference APIs.
aakankshaduggal
force-pushed
the
agents-openai-migration
branch
from
7c93b63 to
ff58e9b
Compare
| # Convert messages to OpenAI format | ||
| openai_messages = [] | ||
| for message in input_messages: | ||
| openai_message = await convert_message_to_openai_dict_new(message) |
There was a problem hiding this comment.
I think this is what is causing the failure in the integration tests. When it gets down into the inference router and then the provider implementation, it's expecting a different type.
the type here needs to be OpenAIMessageParam rather than a dict. I don't know if there is currently a helper for this, I kind of hit the same thing when I was doing some initial work in telemetry. You might need to augment openai_compat.py with a utility to go from Messages input -> [OpenAIMessageParam]
There was a problem hiding this comment.
Yes, thanks Charlie. That's what I was wondering as well.
aakankshaduggal
requested review from
hardikjshah,
raghotham,
ehhuang,
terrytangyuan,
leseb,
bbrowning,
reluctantfuturist,
mattf and
slekkala1
as code owners
There was a problem hiding this comment.
This looks reasonable, thank you for doing this! I would suggest maybe opening an issue to do the "full conversion" and by that I mean the actual params and response types for this API get converted to OpenAI compat, not just the internal translation logic.
I have no other comments here though, the logic looks great!
| # Convert tool definitions to OpenAI format | ||
| openai_tools = None | ||
| if self.tool_defs: | ||
| openai_tools = [] |
There was a problem hiding this comment.
nit: compact to write as:
openai_tools = [convert...(x) for x in (self.tool_defs or [])]
|
Uh oh this PR has bugs. Unclear why CI was green. |
…)" This reverts commit e743d3f.
) Reverts #3097 It has broken agents tests.
|
Please re-do this PR and please specify the exact command line you used to run tests. |
Replace chat_completion calls with openai_chat_completion to eliminate dependency on legacy inference APIs.
What does this PR do?
Closes #3067
Test Plan