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

[OPIK-610] multi models support and anthropic implementation #957

Merged
merged 49 commits into from
Jan 7, 2025

Conversation

idoberko2
Copy link
Contributor

@idoberko2 idoberko2 commented Dec 24, 2024

Details

This PR sets the necessary abstractions in order to introduce additional LLM providers to the chat completion resource logic.
In addition, it adds Anthropic as another LLM provider.

Some things to point out:

  • Anthropic requires maxCompletionTokens to have a value. Therefore, a validation is added. It will probably require some FE changes
  • Unlike Openai, Anthropic doesn't have a test server. Therefore, Anthropic related tests are assuming ANTHROPIC_API_KEY environment variable is set
  • config-test.yml had some invalid values and so parsing them in one of the tests failed. The config file is fixed in this PR

Issues

OPIK-610

Testing

Added an integration test and tested locally:

anthropic_playground

@idoberko2 idoberko2 self-assigned this Dec 24, 2024
Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

This looks in the good direction in my opinion.

I encourage to keep abstracting the minimum parts that differ per LLM provider, mostly the client instantiation, the calls (normal and streaming) and converting requests and responses from/to the OpenAI format.

Everything out of this, leave it in common classes/services, no need for interfacing them if not needed. This is to avoid unnecessary complexity.

@idoberko2 idoberko2 requested a review from andrescrz December 24, 2024 16:04
@amirzatcomet
Copy link

+1 to Andres's comment, there's still a lot that can be moved to a common ancestor class.

@idoberko2
Copy link
Contributor Author

Do you think it's still relevant to the latest version?

@idoberko2 idoberko2 force-pushed the ido/opik-610-support-anthropic-models branch from 8b7025b to 3bfb047 Compare December 25, 2024 07:18
@idoberko2 idoberko2 marked this pull request as ready for review December 29, 2024 09:27
@idoberko2 idoberko2 requested a review from a team as a code owner December 29, 2024 09:27
Copy link
Contributor

@thiagohora thiagohora left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. However, I have a few concerns about leaking implementation details between layers. Ideally, if we can abstract the bind of the streams and handling of other implementation details in the correct layer, the code would be simpler to maintain.

@idoberko2 idoberko2 requested a review from thiagohora January 7, 2025 13:52
thiagohora
thiagohora previously approved these changes Jan 7, 2025
Copy link
Contributor

@thiagohora thiagohora left a comment

Choose a reason for hiding this comment

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

Approving since, as discussed via Slack, the next PR will address some of my previous concerns.

Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

Generally in the good direction. Just needs some more polishing.

andrescrz
andrescrz previously approved these changes Jan 7, 2025
Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

Some comments from previous review still open, but not a blocker.

@idoberko2 idoberko2 merged commit dd3ed0a into main Jan 7, 2025
7 checks passed
@idoberko2 idoberko2 deleted the ido/opik-610-support-anthropic-models branch January 7, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants