-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add OpenAI Chat Completion Agent (GPT-3.5/GPT-4) #5061
Conversation
Some questions related to this diff:
The reason I want to create an abstract class is so that I can house shared logic such as shared arg parameters and additional helper functions (like token counting and prompt truncation).
Do you have a good way to handle this? The only thing that I can think of is parsing the agent id for an int because self-chat with the same model results in an int appended to the end of the agent id like this:
However, this solution is imperfect since I don't believe it handles n-party convos when n >= 3. |
msg = {'role': self.counterparty_role, 'content': observation['text']} | ||
if self.counterparty_name: | ||
msg['name'] = self.counterparty_name | ||
self.turns.append(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to have a custom version of History
to handle turns.
This looks great, thanks for your contribution. To address your questions:
|
@mojtaba-komeili Thanks for reviewing this PR! Ok, I addressed the smaller nits and suggestions that you highlighted so far. The individual commits in the PR should make it clear what was addressed. I also made a a first pass at the OpenAI base class: it only abstracts out shared CLI params. Let me know what you think so far. I can add an improved fake history if this is the right approach. Also, should we split up the base API changes into a separate PR? I'm not super well versed in working in PRs, so I am not sure at what point there is too much scope creep within an individual PR. However, I can imagine that tacking this feature onto this PR could make it harder to read and understand the git history rather than having 2 PRs. Let me know if it is ok to keep all of the code in this PR or if it should be split up. |
d077808
to
1adc079
Compare
Thanks a lot. This looks great.
Thanks again for your contribution. |
Thanks for the suggestions. I think we can definitely get a core history class in, and this time I will give it a better name than FakeHistory haha. So, if I were to split this PR into 2, this is my proposed split
I think if we split it that way, we can just chronologically split the later commits into a separate PR. Let me know what you think @mojtaba-komeili. Thanks again! |
Sorry for late reply. Yeah, that sounds like a good plan to me. The first PR is making a practical change to be used immediately and the second makes the basis for future similar ones. |
The latest change represents the "First PR adds the Chat Completion Agent" as discussed above. In other words, we removed the the 2 latest commits which do the following "base openai agent, a base history class, and usage of the base history class". We will add those in a follow-up PR to keep the commit history cleaner and more readable. Sorry for the late response, @mojtaba-komeili! Please let me know if anything else needs to be changed before it is good to go. |
``` | ||
|
||
## Limitations | ||
This API wrapper has three major limitations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said three, but four is listed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can fix this in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Looking forward to your follow up PR.
Patch description
We are adding support for the latest OpenAI models like GPT-3.5 (ChatGPT) and GPT-4 by adding a wrapper for the chat completion API.
Testing steps
We've manually tested that the wrapper can handle a multi-turn conversation without error-ing which demonstrates that the wrapper is properly storing and querying the API endpoint. We tested conversation capability both with GPT-3.5 and GPT-4 models in both
self-chat
andinteractive
modes.Self Chat
Interactive Mode