Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: GPT35Generator #5714

Merged
merged 44 commits into from
Sep 7, 2023
Merged

feat: GPT35Generator #5714

merged 44 commits into from
Sep 7, 2023

Conversation

ZanSara
Copy link
Contributor

@ZanSara ZanSara commented Sep 4, 2023

Related Issues

Proposed Changes:

  • Add ChatGPTGenerator
  • Add unit tests
  • Add end to end tests

How did you test it?

  • Local tests run
  • CI

Notes for the reviewer

openai already added to haystack-ai: deepset-ai/haystack-preview-package#2

Checklist

@ZanSara ZanSara changed the title add chatgpt generator feat: ChatGPTGenerator Sep 4, 2023
@github-actions github-actions bot added the type:documentation Improvements on the docs label Sep 4, 2023
@ZanSara ZanSara mentioned this pull request Sep 4, 2023
@ZanSara ZanSara changed the base branch from chatgpt-llm-backend to main September 4, 2023 16:55
@ZanSara ZanSara marked this pull request as ready for review September 5, 2023 09:45
@ZanSara ZanSara requested review from a team as code owners September 5, 2023 09:45
@ZanSara ZanSara requested review from dfokina and vblagoje and removed request for a team September 5, 2023 09:45
@mathislucka
Copy link
Member

A small note on naming, a ChatGPTGenerator that does not support chat feels a bit confusing to me.

@ZanSara
Copy link
Contributor Author

ZanSara commented Sep 6, 2023

Good point 👀 Things should clear up as soon as the Agents proposal takes shape. At that point we will rework this very simple component into something more powerful and/or clarify better what it can and can't do. It is in no way feature complete, final or anything. It's just a simple component to make RAG possible in v2. Let's keep iterating!


@mathislucka @sjrl I might rename it GPT35Generator if that helps, but it looks a bit uglier to me. Wdyt?

@mathislucka
Copy link
Member

@mathislucka @sjrl I might rename it GPT35Generator if that helps, but it looks a bit uglier to me. Wdyt?

That sounds better I think.

@ZanSara ZanSara changed the title feat: ChatGPTGenerator feat: GPT35Generator Sep 6, 2023
@dfokina
Copy link
Contributor

dfokina commented Sep 6, 2023

@ZanSara hey I see there are some more places with ChatGPT name, like the name of the .py file or in docstrings 🙂

@ZanSara
Copy link
Contributor Author

ZanSara commented Sep 6, 2023

Thank you! I'm having another pass 🙌

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

I took a general look and I have some high-level doubts.
(Probably many of these points are about legitimate implementation choices or future developments to be done in separate PRs...)

  1. Although it has already been discussed, the naming sounds strange to me. I would not expect that I can use the GPT35Generator to run GPT4. Other options: OpenAIGenerator, OpenAIGPTGenerator,...

  2. (Partly related to the previous point?) This component does not support the legacy Completions API, supported in Haystack 1.x. Although declared legacy, the Completions API will be probably retired in 2024. Don't we want to support it in Haystack 2.x? (It may well be a reasonable choice)

  3. What happens if the user enters a wrong model name? Is it ok to let the OpenAI SDK raise an error or do we want to intercept it in Haystack?

  4. What happens if the user sends a prompt that fills/exceeds the context window?

  5. Maybe the OpenAI organization should be declared in __init__ and then used. See feat: support OpenAI-Organization for authentication #5292.

  6. Azure ChatGPT Generator (corresponding to this invocation layer) will be addressed in another PR. Right?

@ZanSara
Copy link
Contributor Author

ZanSara commented Sep 6, 2023

Ok let's take them one by one:

  1. Although it has already been discussed, the naming sounds strange to me. I would not expect that I can use the GPT35Generator to run GPT4. Other options: OpenAIGenerator, OpenAIGPTGenerator,...

I plan to make a subclass called GPT4Generator that simply sets another default model. I thought about OpenAIGenerator, but there are models that have a very different interface (think about davinci, if we ever want to support it) and in any case if tomorrow OpenAI releases another model we'll have naming issues. In any case rest assured that at least the GPT4 alias is coming asap.

  1. (Partly related to the previous point?) This component does not support the legacy Completions API, supported in Haystack 1.x. Although declared legacy, the Completions API will be probably retired in 2024. Don't we want to support it in Haystack 2.x? (It may well be a reasonable choice)

If we do, I'd rather support them into a dedicated generator that we will be able to deprecate easily when the time comes.

  1. What happens if the user enters a wrong model name? Is it ok to let the OpenAI SDK raise an error or do we want to intercept it in Haystack?

Good point! I'll check how the error looks like. I expect it to be quite clear, if it is I'll let it go across. Otherwise I'll figure out a way to communicate it to the user better..

  1. What happens if the user sends a prompt that fills/exceeds the context window?

Right! I'll add a test!

  1. Maybe the OpenAI organization should be declared in __init__ and then used. See feat: support OpenAI-Organization for authentication #5292.

Ah right! It got lost in the last refactoring. Let me bring it back.

  1. Azure ChatGPT Generator (corresponding to this invocation layer) will be addressed in another PR. Right?

Yes Azure support is for the next PR 🙂

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ZanSara ZanSara merged commit 63cbde7 into main Sep 7, 2023
60 of 61 checks passed
@ZanSara ZanSara deleted the chatgpt-llm-generator branch September 7, 2023 08:06
ivanazeljkovic pushed a commit to smartcat-labs/haystack that referenced this pull request Sep 11, 2023
* chatgpt backend

* fix tests

* reno

* remove print

* helpers tests

* add chatgpt generator

* use openai sdk

* remove backend

* tests are broken

* fix tests

* stray param

* move _check_troncated_answers into the class

* wrong import

* rename function

* typo in test

* add openai deps

* mypy

* improve system prompt docstring

* typos update

* Update haystack/preview/components/generators/openai/chatgpt.py

* pylint

* Update haystack/preview/components/generators/openai/chatgpt.py

Co-authored-by: Silvano Cerza <[email protected]>

* Update haystack/preview/components/generators/openai/chatgpt.py

Co-authored-by: Silvano Cerza <[email protected]>

* Update haystack/preview/components/generators/openai/chatgpt.py

Co-authored-by: Silvano Cerza <[email protected]>

* review feedback

* fix tests

* freview feedback

* reno

* remove tenacity mock

* gpt35generator

* fix naming

* remove stray references to chatgpt

* fix e2e

* Update releasenotes/notes/chatgpt-llm-generator-d043532654efe684.yaml

Co-authored-by: Daria Fokina <[email protected]>

* add another test

* test wrong model name

* review feedback

---------

Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Silvano Cerza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants