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-546: Implement create chat completions endpoint #890

Merged

Conversation

andrescrz
Copy link
Collaborator

@andrescrz andrescrz commented Dec 13, 2024

Details

Implemented create chat completions endpoint:

  • Only Open AI provider is supported.
  • Using openai4j client from langchain4j.
  • Provided extensive configuration for the client with defaults values for retry policy, timeouts etc.
  • Updated Open API spec to align with the Open AI spec.
  • Fixed some bugs and made some improvements along the way.

A PR will follow to update the Open API spec.

Issues

Resolves OPIK-546

Testing

  • Added integration tests covering success and reproducible error cases.
  • Both for streaming and non-streaming reponses.
  • Using the LangChain4j demo endpoint for the tests.

Documentation

@andrescrz andrescrz self-assigned this Dec 13, 2024
@andrescrz andrescrz added the enhancement New feature or request label Dec 13, 2024
@andrescrz andrescrz marked this pull request as ready for review December 13, 2024 12:48
@andrescrz andrescrz requested a review from a team as a code owner December 13, 2024 12:48
Copy link

@ldaugusto ldaugusto left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

One minor consideration and one question, neither is blocking.


@Positive private Double backoffExp;

@MinDuration(value = 1, unit = TimeUnit.MILLISECONDS)

Choose a reason for hiding this comment

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

maybe the minimum value in milliseconds for timeouts(this and the next ones) could be higher, like at least 100? This way we protect users that dont know/understand if its seconds or milliseconds.

it can be very frustating for users setting a value like '10' imagining seconds and only getting timeouts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to make it as flexible as possible to not to restrict the user to any value. Also, I took some inspiration from the values in some of the core Dropwizard configs such as DataSourceFactory.

But your comment makes sense. I might update the values in a follow-up PR.

openApiClient:
# See demo endpoint Langchain4j documentation: https://docs.langchain4j.dev/get-started
# Not https but only used for testing purposes. It's fine as long as not sensitive data is sent.
url: http://langchain4j.dev/demo/openai/v1

Choose a reason for hiding this comment

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

is this a public endpoint provided by the library developers to support testing?

Copy link
Collaborator Author

@andrescrz andrescrz Dec 13, 2024

Choose a reason for hiding this comment

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

Correct, you can find it in the documentation linked in the description of this PR.

A potential downside is that tests might fail or be flaky, if that endpoint is no longer supported or if they throttle us or if they make any backwards incompatible change etc.

I had zero issues while developing and also we are already hitting other external endpoints from other non-backend tests. Specially for the SDK integrations, including the overall end to end tests.

Considering all this, I think it's ok to follow this approach and we'll change it if any issue appears.

@andrescrz andrescrz merged commit 1415da1 into main Dec 13, 2024
7 checks passed
@andrescrz andrescrz deleted the andrescrz/OPIK-546-implement-create-chat-completions-endpoint branch December 13, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants