[OPIK-744] fix providers' models list #1041
Conversation
| @RequiredArgsConstructor | ||
| public enum OpenaiModelName { |
There was a problem hiding this comment.
We have a similar list in ModelPrice. Can we try to unify both lists?
There was a problem hiding this comment.
Nice catch! I completely forgot about this one. Updated.
@BorisTkachenko, I noticed a few openai models missing in ModelPrice and added them. Your review will be appreciated. In addition, can we consider renaming ModelPrice to be openai specific? Something like OpenaiModelPrice?
There was a problem hiding this comment.
@idoberko2 Feel free to rename, seems ok to me. Regarding new models they sometimes add them, so list should be updated. However I'm not sure about those two: gpt-4-0613, gpt-4-turbo-preview.
It looks like they are not listed under OpenAI prices, so I guess we do not need them:
https://openai.com/api/pricing/
There was a problem hiding this comment.
They are used as aliases and therefore can be used by users, see here: https://platform.openai.com/docs/models#gpt-4-turbo-and-gpt-4
idoberko2
force-pushed
the
ido/opik-744-fix-openai-models-list
branch
from
ccf0c4a to
fd36b12
Compare
@idoberko2 yeah, I agree. I was never a big fan of that requirement. It was a good way to start, but I encourage you to open a backlog ticket and make a proposal in there for a better solution. Or at least just create the ticket and document the potential problem you described. |
There was a problem hiding this comment.
Let's think for a second if we want to couple the model pricing enumeration with the LLM proxy enumeration.
It doesn't seem like a good idea, see my comments.
Additionally, we don't support pricing yet for Anthropic and Gemini, so that inconsistency in the enums used is another indicator of this problem.
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderFactoryTest.java
Outdated
Show resolved
Hide resolved
This reverts commit c3ca2e0.
idoberko2
dismissed stale reviews from thiagohora and BorisTkachenko
via
92f67da
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java
Outdated
Show resolved
Hide resolved
This reverts commit d18f06c
Details
This PR extends supported OpenAI models list to match their documentation. During the work on that I noticed langchain4j's Anthropic models list is missing all the
*-latestflavors so I took care of that as well.@aadereiko FYI
Issues
OPIK-744
Testing
Based on existing tests.
Note
@andrescrz, during the work on that I noticed OpenAi's model named
o1. I think we should reconsider provider resolution based on the model name since I guess at some point we will discover a name collision.