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

Putting Generic Authentication flow to the Core #694

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

csotiriou
Copy link
Contributor

@csotiriou csotiriou commented Jun 20, 2024

Implementation of request mentioned in #626

@csotiriou
Copy link
Contributor Author

csotiriou commented Jun 20, 2024

@geoand @sberyozkin, I am putting this as a draft here in reference to #626 , is this what you were looking for? I am going to update the documentation once we agree that this is what's needed.

I opted to keep the usage of the ModelAuthProvider inside the Azure OpenAI since I imagine that each provider will have their own authentication implementation using this interface.

I welcome every proposal / change to this logic.

@sberyozkin
Copy link
Contributor

sberyozkin commented Jun 20, 2024

Hi @csotiriou Thanks for starting looking into it. Yes, I think this is what we thought about with Georgios.

I opted to keep the usage of the ModelAuthProvider inside the Azure OpenAI since I imagine that each provider will have their own authentication implementation using this interface.

+1. I'll do the same for Vertex Gemini and subsequently we can do it for all other remote model providers as necessary.
And then users can register ModelAuthProvider with whatever authentication logic they want...

Thanks

@geoand
Copy link
Collaborator

geoand commented Jun 21, 2024

Very nice, thanks!

The formatter is complaining:

Failed to execute goal net.revelc.code:impsort-maven-plugin:1.9.0:check (check-imports) on project quarkus-langchain4j-openai-common: Imports are not sorted in /home/runner/work/quarkus-langchain4j/quarkus-langchain4j/model-providers/openai/openai-common/runtime/src/main/java/io/quarkiverse/langchain4j/openai/OpenAiRestApi.java 

Furthermore, please rebase the PR onto main so we don't have any merge commits.

@csotiriou csotiriou marked this pull request as ready for review June 22, 2024 10:37
@csotiriou csotiriou requested a review from a team as a code owner June 22, 2024 10:37
@csotiriou csotiriou force-pushed the feature/core-authentication branch 3 times, most recently from 7d23f5d to 832ab48 Compare June 22, 2024 11:12
@csotiriou
Copy link
Contributor Author

@geoand @sberyozkin thanks,

I just corrected the documentation, since atm OpenAI is the only module that is using it, I suppose we don't need to move it elsewhere. If we want to move it elsewhere, I wonder where this might be (a new section, perhaps?)

@sberyozkin
Copy link
Contributor

Thanks @csotiriou

Having model provider sections like it is done for OpenAI is good IMHO, perhaps, once I have it supported for one more model provider, Vertex Gemini, it would make sense to have a higher level short section on ModelAuthProvider linking to model providers supporting it... Does it sound reasonable ? If you prefer, please add it now, or I can do it later...

@geoand
Copy link
Collaborator

geoand commented Jun 25, 2024

Hey folks,

sorry for the delay, I was travelling last week and a ton of stuff had piled up!

I will review shortly

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I added a small comment

@csotiriou csotiriou force-pushed the feature/core-authentication branch 2 times, most recently from e13e938 to 3c9bbb2 Compare June 25, 2024 12:00
putting the resolve methods in the interface, not in the embedded Resolver class

updating documentation

formatting

added forgotten unremovable beans configuration

Update docs/modules/ROOT/pages/openai.adoc

Co-authored-by: Georgios Andrianakis <geoand@gmail.com>
@csotiriou csotiriou force-pushed the feature/core-authentication branch from a6318c1 to 90bccba Compare June 25, 2024 12:02
@geoand
Copy link
Collaborator

geoand commented Jun 25, 2024

Thanks a lot for this!

@sberyozkin do you want to test it locally to make sure if fits your needs?

@sberyozkin
Copy link
Contributor

Hi @geoand @csotiriou , sure, give me a few days please, I'll rework vertex gemini PR, cheers


public OpenAIRestAPIFilter(AuthProvider authorizer) {
public OpenAIRestAPIFilter(ModelAuthProvider authorizer) {
Copy link
Contributor

@sberyozkin sberyozkin Jun 28, 2024

Choose a reason for hiding this comment

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

IMHO it should be aligned with how it is done for Vertex AI Gemini TokenFilter, to have ExecutorService injected as well and then call a custom provider from this ExecutorService as providers may want to use some blocking calls etc. Using ManagedExecutor is best as it retains the original RequestScope which lets users inject Quarkus API beans which require it, as proposed in #708.

We confirmed with Georgios that using ManagedExecutot helps in the secure-fraud-detection demo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point!

import io.quarkiverse.langchain4j.ModelName;

public interface ModelAuthProvider {
String getAuthorization(Input input);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @csotiriou @geoand, only a couple of questions/suggestions for you to consider.

I was thinking, can we expect situations going forward where local models will require a secure token access, and where a token is passed, for example, as a property ?

I guess one can say that if Input is null then do not return a Bearer prefix in the response... This is related to the next question...

The other question, so ModelAuthProvider is expected to return a complete HTTP Authorization header value, including the scheme, Bearer <token>, instead of only <token>. In #708, moving to ModelAuthProvider has an impact of having to update the test filter to include Bearer - I don't think it is a breaking change though, since the current AuthProvider is not public.

I wonder, should we expect providers returns the token only, and then filters, depending on a context (access to the remote or local model) will either add Bearer or not.

IMHO it might be a bit better, and a little bit simpler for custom auth providers too...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to worry about that now, it's very early days to be able to know exactly how systems using auth will evolve.
For now, I think the proposal in this PR is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@geoand Sure, Ok then, this PR works well for #708 so it is good to go from my perspective, thanks

@geoand geoand merged commit d4c5b17 into quarkiverse:main Jun 28, 2024
12 checks passed
@csotiriou csotiriou deleted the feature/core-authentication branch June 28, 2024 13:51
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.

None yet

3 participants