-
Notifications
You must be signed in to change notification settings - Fork 0
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
implement get history for image captions, change openai to gpt3.5 #39
base: master
Are you sure you want to change the base?
Conversation
tritct
commented
Aug 29, 2023
- Implement get history for captions
- change OpenAI model to gpt3.5
I added endpoint for getting caption history, currently I'm outputing all information stored in captions table, like in the picture below, tell me if you need me to change the output fields @minhnld : |
@BinhPhamQuang I also changed OpenAI to ChatOpenAI gpt3.5 since it's almost 30x cheaper according to my calculations based on their token pricing, so I updated some parts that use OpenAI in vocab service. Can you check to see if anything is wrong. Thank you!! |
Please don't change what is not your scope in the PR, if it is not tested carefully it should lead to outages on production |
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.
I cannot approve this unless you can explain the code you wrote, Check my comments tho.
@@ -122,10 +122,10 @@ class Container(containers.DeclarativeContainer): | |||
model_id=config.infrastructures.replicate.caption_model.model_id, | |||
caption_client=caption_client, | |||
) | |||
open_ai: OpenAI = providers.Singleton( | |||
OpenAI, | |||
open_ai: ChatOpenAI = providers.Singleton( |
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.
It seems clear in the code that, You're not using anything assembled to the "Chat" function of the OpenAI at all, so here You can use the base OpenAI class enough, just update the model name
openai_api_key=config.infrastructures.open_ai.openai_api_key, | ||
max_tokens=config.infrastructures.open_ai.max_tokens, |
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.
Why remove this, this param should not be remove even using whatever API
if isinstance(text, BaseMessageChunk): | ||
yield text.dict()["content"] | ||
elif isinstance(text, str): | ||
yield text |
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.
I don't think this is going to work with the vocab generation,
Did you test these code changes before committing? Be honest