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

Add secure-fraud-detection demo #539

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

sberyozkin
Copy link
Contributor

@sberyozkin sberyozkin commented May 3, 2024

Draft PR for a secure-fraud-detection demo.

The startup issue described here earlier was resolved as advised by Georgios

@geoand
Copy link
Collaborator

geoand commented May 4, 2024

Nice!

I am guessing that the error you are seeing is due to maven not being configured with to make javac retain method parameter names.

@sberyozkin
Copy link
Contributor Author

sberyozkin commented May 4, 2024

Thanks @geoand, yes, I was wondering how exactly does the translate demo work which has 2 parameters, and now I've found https://github.com/quarkiverse/quarkus-langchain4j/blob/main/samples/cli-translator/pom.xml#L12 :-)

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 38b3c99 to 162391e Compare May 7, 2024 18:15
@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 162391e to 38e5513 Compare May 8, 2024 14:17
@sberyozkin
Copy link
Contributor Author

sberyozkin commented May 8, 2024

@geoand @jmartisk
I'm getting closer to finalizing the demo flow, a user accesses the login page where a Google authentication option is offered, user logs in and is redirected to the fraud detection entry page where the user can choose to check it by amount or distance.
This in itself is useful enough to show how to use the authentication token, but I think the most interesting part is actually how to utilize the fact the OIDC session is available for correlating multiple fraud queries to get more interesting responses each time the same authenticated user tries to check the fraud status, so I'll play around next with the custom memory id idea.
Once we are all happy with the demo flow, I'll fix README

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 38e5513 to 0fe8b82 Compare May 20, 2024 14:48
@sberyozkin
Copy link
Contributor Author

@geoand I prototyped a custom DefaultMemoryIdProvider and in this demo I'd like to do something simple, since no WebSockets are involved, like adding a boolean property indicating if the customer has already issued a given query before (not 100% sure how to do it yet :-), right now I'm just hoping the existing chat history can be correctly detected), and then later in the secure variation of the csv chat box demo, use the security identity info in the memory id to impact the SQL query.

But the following error is becoming quite persistent:

Resulted in: java.lang.RuntimeException: dev.ai4j.openai4j.OpenAiHttpException: {
  "error": {
    "message": "Invalid parameter: messages with role 'tool' must be a response to a preceeding message with 'tool_calls'.",
    "type": "invalid_request_error",
    "param": "messages.[1].role",
    "code": null
  }
}
	at dev.langchain4j.internal.RetryUtils$RetryPolicy.withRetry(RetryUtils.java:195)
	at dev.langchain4j.internal.RetryUtils.withRetry(RetryUtils.java:229)
	at dev.langchain4j.model.openai.OpenAiChatModel.generate(OpenAiChatModel.java:153)
	at dev.langchain4j.model.openai.OpenAiChatModel.generate(OpenAiChatModel.java:118)
	at dev.langchain4j.model.chat.ChatLanguageModel_XNMsOaekknG7BdNZ5YSUkjh1SqE_Synthetic_ClientProxy.generate(Unknown Source)
	at io.quarkiverse.langchain4j.runtime.aiservice.AiServiceMethodImplementationSupport.doImplement(AiServiceMethodImplementationSupport.java:228)
	at io.quarkiverse.langchain4j.runtime.aiservice.AiServiceMethodImplementationSupport.implement(AiServiceMethodImplementationSupport.java:97)
	at io.quarkiverse.langchain4j.sample.FraudDetectionAi$$QuarkusImpl.detectAmountFraudForCustomer(Unknown Source)
	at io.quarkiverse.langchain4j.sample.FraudDetectionAi$$QuarkusImpl_Subclass.detectAmountFraudForCustomer$$superforward(Unknown Source)
	at io.quarkiverse.langchain4j.sample.FraudDetectionAi$$QuarkusImpl_Subclass$$function$$1.apply(Unknown Source)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
	at io.smallrye.faulttolerance.FaultToleranceInterceptor.lambda$syncFlow$3(FaultToleranceInterceptor.java:253)
	at io.smallrye.faulttolerance.core.InvocationContext.call(InvocationContext.java:20)
	at io.smallrye.faulttolerance.core.Invocation.apply(Invocation.java:29)
	at io.smallrye.faulttolerance.core.timeout.Timeout.doApply(Timeout.java:55)
	at io.smallrye.faulttolerance.core.timeout.Timeout.apply(Timeout.java:30)
	at io.smallrye.faulttolerance.FaultToleranceInterceptor.syncFlow(FaultToleranceInterceptor.java:255)
	at io.smallrye.faulttolerance.FaultToleranceInterceptor.intercept(FaultToleranceInterceptor.java:182)
	at io.smallrye.faulttolerance.FaultToleranceInterceptor_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
	at io.quarkiverse.langchain4j.sample.FraudDetectionAi$$QuarkusImpl_Subclass.detectAmountFraudForCustomer(Unknown Source)
	at io.quarkiverse.langchain4j.sample.FraudDetectionAi$$QuarkusImpl_ClientProxy.detectAmountFraudForCustomer(Unknown Source)
	at io.quarkiverse.langchain4j.sample.FraudDetectionResource.detectBaseOnAmount(FraudDetectionResource.java:39)
	at io.quarkiverse.langchain4j.sample.FraudDetectionResource_Subclass.detectBaseOnAmount$$superforward(Unknown Source)
	at io.quarkiverse.langchain4j.sample.FraudDetectionResource_Subclass$$function$$2.apply(Unknown Source)

Can you please, when you have a few mins, have a quick look at FraudDetectionAi and both TransactionRepository and CustomerRepository, does something look wrong ? I've seen this error on the web, but I'm not sure yet where to start digging, thanks

@sberyozkin
Copy link
Contributor Author

It is transient though, after some delay, I've restarted and I'm getting an interesting response, There are no recent transactions for the customer John Doe with the email [email protected]., and then later I got a normal response.
Probably related to the OpenAi session left hanging after some restarts, so may be some extra close is needed.

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 0fe8b82 to cf7bfeb Compare May 20, 2024 17:14
@sberyozkin
Copy link
Contributor Author

sberyozkin commented May 20, 2024

In any case, I'm moving away from using tools in favor of ContentRetriever, yet to be implemented similarly to how it is done in the csv chatbot demo, since it gives a nice option to use Query.getMetadata().getMemoryId, but I'm keeping it as a separate commit in case I get stuck :-)

Update: I've followed with a few minor updates and squashed everything, it will be easy to get back to tools if necessary

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch 2 times, most recently from dc36dbf to ec93506 Compare May 20, 2024 18:46
@geoand geoand requested a review from jmartisk May 21, 2024 05:30
@jmartisk
Copy link
Collaborator

Gonna look into this one in a bit!

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from ec93506 to e5eede2 Compare May 21, 2024 10:19
@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch 2 times, most recently from e9a6a85 to 978b215 Compare May 21, 2024 16:37
@sberyozkin sberyozkin marked this pull request as ready for review May 21, 2024 16:37
@sberyozkin
Copy link
Contributor Author

Thanks @jmartisk, makes sense to make it easier for users to register emails, may be with a system property, will have a look next week

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.

I've been thinking about this some more.

IIUC, the intent here is to show that the AiService can access the logged in user when retreving data, correct?
If so, I think that using the MemoryIdProvider is the not best abstraction.

Would it not be far easier to inject the logged in user in FraudDetectionRetrievalAugmentor?

@sberyozkin
Copy link
Contributor Author

Thanks @geoand,

Would it not be far easier to inject the logged in user in FraudDetectionRetrievalAugmentor?

Yeah, good point, I tried it earlier, to inject it directly to FraudDetectionContentRetriever but RequestContext carrying the security identity was not available.

I can try to inject it into a custom retrieval augmentor, but if that works, how would I pass it to the content retriever ?

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Yeah, good point, I tried it earlier, to inject it directly to FraudDetectionContentRetriever but RequestContext carrying the security identity was not available.

You are right, I see why this is the case...

Unfortunately I don't have any good solutions for you at the moment, but I do see how we add something to LangChain4j to make this possible.

Essentially what I have in mind is to enhance dev.langchain4j.rag.query.Metadata with a Map<String, Object> where custom data can be added.
Then we would also need a new concept like MetadataEnhancer which we could then use in Quarkus to put in whatever we want, in this case the user id or email or whatever.

Then your ContentRetriever could retrieve the information from query.getMetadata();

What I don't like about this is that it would force user code (the ContentRetriever to know specific keys).

An alternative approach would be for us to write our own version of DefaultRetrievalAugmentor which would not suffer from the problem of not passing the context (as it take steps to have that done properly).

WDYT @jmartisk @sberyozkin ?

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Jun 13, 2024

Essentially what I have in mind is to enhance dev.langchain4j.rag.query.Metadata with a Map<String, Object> where custom data can be added.

Sure, that can definitely do it, some well known key properties like io.quarkus.security.SecurityIdentity can be doc-ed

An alternative approach would be for us to write our own version of DefaultRetrievalAugmentor which would not suffer from the problem of not passing the context (as it take steps to have that done properly).

I'm not sure yet as I don't know much about the mechanics involved, on the basic level having an extra Metadata Map looks like it can work better as the identity may be needed across several pieces where Query is available, not only at the RAG level.

I was actually thinking about some workarounds on the way to the office :-) I was still thinking in terms of a custom memory id since the authenticated/logged in user's session lifetime matches the secure memory id lifetime.
So what I had in mind is that the project introduces CompositeMemoryIdBean bean and now, the core code, before converting a provided memory id object to String, checks if this object has a marker like CompositeMemoryBeanExpected (or may be simply if it an instance of String or not - if not - wrapping is expected). And if the composite memory id bean is expected, then CompositeMemoryIdBean will wrap and carry the original custom memory id object around, its toString() method will be done the way it is done now in the core (memory id object string rep plus an AI service class/method name), and then whichever provider needs it, it casts a memory id object to CompositeMemoryIdBean after an instanceof check, and gets an original memoryId object as getMemoryId

May be it is a bit hacky, not sure, though it would probably fit closely enough the expectation that the memory id can be any object. I think I also like the idea of the extra metadata map.

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

I was actually thinking about some workarounds on the way to the office :-) I was still thinking in terms of a custom memory id since the authenticated/logged in user's session lifetime matches the secure memory id lifetime.

This is the one solution I would like to avoid because MemoryId is really a different concept. It's fine to populate it automatically, but it's not fine to use it's value to do something else.

@sberyozkin
Copy link
Contributor Author

@geoand I'm happy enough to pursue adding extra Map to Metadata, I can probably suggest a PR to langchain4j, it looks it can be of general interest... How would we set the extra bits into this Map though at the quarkus-langchain4 level ?

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

it looks it can be of general interest.

Yeah, I think so to, but there also needs to be a way to add metadata, something like the MetadataEnhanced I suggested above.

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Jun 13, 2024

@geoand OK, then, it will certainly cover the RAG flow. Can you clarify please, when you get a sec, why RequestContext is not available in the custom augmentor or content retriever, like you said, that would definitely be a simplest solution, if it were available. (I'm also thinking about making the identity visible in other places like custom tools and other custom non-RAG related providers).
I'm pretty sure your idea of adding an extra map to Metadata can be generally useful though, starting from it would be great, so I'll look into opening a PR in langchain4j

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Can you clarify please, when you get a sec, why RequestContext is not available in the custom augmentor or content retrieve

Because the implementation in upstream LangChain4j uses a CompletableFuture and it's own executor in order to do calls in parallel.
Now that I think about it, it probably makes sense to see updating DefaultRetrievalAugmentor to use our own Executor which we would feed with the one from context propagation works. Do you want to give that a shot?

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 8c11a7e to 4029ad8 Compare June 13, 2024 15:34
@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Now that I think about it, it probably makes sense to see updating DefaultRetrievalAugmentor to use our own Executor which we would feed with the one from context propagation works. Do you want to give that a shot?

Actually, it should be possible to do this even now, no changes to LangChain4j needed.

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from 4029ad8 to ee23108 Compare June 13, 2024 15:39
@sberyozkin
Copy link
Contributor Author

Hi @geoand, I was just dealing with formatting as well as a suggestion from @jmartisk to report something to the user when a customer is missing, and now a customer full name and email must be provided at startup as system properties, as opposed to having to modify Setup.java. Hopefully the user exp will be better. Jan, please have a look when you are back.

Super cool that we can try our own executor, I can test, how do I do it ? Is Quarkus executor injectable or which one do you have in mind ?

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

You can inject org.eclipse.microprofile.context.ManagedExecutor from context propagation and use that

@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from ee23108 to f1f0df6 Compare June 13, 2024 15:46
@sberyozkin sberyozkin marked this pull request as draft June 13, 2024 15:47
@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from f1f0df6 to c5df9fa Compare June 13, 2024 16:06
@sberyozkin sberyozkin force-pushed the secure-fraud-detection-demo branch from c5df9fa to 58e2662 Compare June 13, 2024 16:29
@sberyozkin sberyozkin marked this pull request as ready for review June 13, 2024 16:29
@sberyozkin
Copy link
Contributor Author

@geoand Now I'm really happy, the custom memory id provider has gone :-), secure authenticated access to the content retriever is done, thanks for the ManagedExecutor idea. I think it can probably make sense to doc it separately, I can work a bit later on a doc dedicated to securing various AI service parts...

I think this PR is now quite ready to go once yourself and @jmartisk happy enough with it in general

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Very nice!

+1 on getting this in - but let's wait for @jmartisk

@jmartisk
Copy link
Collaborator

Guys I see you were discussing passing some extra metadata around a RAG pipeline, please check langchain4j/langchain4j#1122, that is related

@sberyozkin
Copy link
Contributor Author

sberyozkin commented Jun 14, 2024

@jmartisk

Guys I see you were discussing passing some extra metadata around a RAG pipeline, please check langchain4j/langchain4j#1122, that is related

Thanks for the link, it is interesting, luckily, at least for the security identity, it can be injected into RAG related parts, but I'm not sure yet if it can be injected into Tools, something I can try in the next demo for the secure web socket based sql chatbot one, so that is indeed a very related discussion, thanks

@jmartisk jmartisk merged commit e1b070c into quarkiverse:main Jun 17, 2024
12 checks passed
@sberyozkin sberyozkin deleted the secure-fraud-detection-demo branch June 17, 2024 07:44
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