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

Custom memory id is now converted to String #607

Closed
sberyozkin opened this issue May 21, 2024 · 11 comments
Closed

Custom memory id is now converted to String #607

sberyozkin opened this issue May 21, 2024 · 11 comments

Comments

@sberyozkin
Copy link
Contributor

With this PR, #594, the custom memory ID is converted to String, even if this memory ID is not a String.

I believe it introduces a significant restriction, at the langchain4J level it is documented that the memory ID can be any object.

I can no longer pass a pair of properties representing an authentication session in #539.

Is it possible to rework #594 ?

@sberyozkin
Copy link
Contributor Author

Perhaps do the extra addition only if it is an instance of String ?

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

I can no longer pass a pair of properties representing an authentication session in #539.

Why not? If should work correctly if toString works correctly.

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

Perhaps do the extra addition only if it is an instance of String ?

That would leave the use case that we intended to fix by adding the AI service, broken

@sberyozkin
Copy link
Contributor Author

Why not? If should work correctly if toString works correctly.

I can do some manual parsing, but it is not a good dev experience for anyone interested to use anything but String for a memory id.

That would leave the use case that we intended to fix by adding the AI service, broken

Sure, that one has to be supported, but now quarkus-langchain4j can only support String as a memory id.

I can workaround about it, but I guess there should be a doc note about it if you prefer to close this issue

Thanks

@sberyozkin
Copy link
Contributor Author

I suppose with @MemoryId users can still use objects, this update only impacts memory id providers

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

I can do some manual parsing, but it is not a good dev experience for anyone interested to use anything but String for a memory id.

There should really be nothing the developer needs to do because this is really an SPI meant to be implemented by integration code.

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

Sure, that one has to be supported, but now quarkus-langchain4j can only support String as a memory id.

Not exactly: It's the default memory Id service (which only kicks in when no memoryId is provided) that needs to come up with an object that can be converted to a unique string.

@sberyozkin
Copy link
Contributor Author

Georgios, but we've discussed the use case before, and I thought we agreed it was worth pursuing, how do I demo a case where a security session which correctly covers the current user interactions can be used to support the user specific retrieval ? The only tool to customize the memory id is DefaultMemoryIdProvider and now it does not work any more.

Should I reopen #523 to have a provider which is not meant to be for internal use only ?

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

Sure, you can go ahead and do it, it's just that now you'll have to make sure that whatever you use as the key for the memory can be properly converted into a unique string unfortunately there is no way around this

@sberyozkin
Copy link
Contributor Author

All right, I can work with a string, and expect a # suffix, it is not too difficult, thanks

@geoand
Copy link
Collaborator

geoand commented May 21, 2024

👍🏼

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

No branches or pull requests

2 participants