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

Update to LangChain4j 0.32.0 #721

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Update to LangChain4j 0.32.0 #721

merged 4 commits into from
Jul 4, 2024

Conversation

geoand
Copy link
Collaborator

@geoand geoand commented Jul 4, 2024

Also includes:

  • Use the Quarkus worker pool for in-process embedding models
  • Add span for each call to a ChatLanguageModel
  • Add metrics for each call to a ChatLanguageModel

@geoand geoand requested a review from a team as a code owner July 4, 2024 12:04
@geoand geoand changed the title Update to LangChain4j 0.23 Update to LangChain4j 0.32,0 Jul 4, 2024
@geoand geoand changed the title Update to LangChain4j 0.32,0 Update to LangChain4j 0.32.0 Jul 4, 2024
@geoand
Copy link
Collaborator Author

geoand commented Jul 4, 2024

@jmartisk would you take a look at the Redis failures please?

Error:  Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 25.07 s <<< FAILURE! -- in io.quarkiverse.langchain4j.redis.deployment.RedisEmbeddingStoreTest
Error:  io.quarkiverse.langchain4j.redis.deployment.RedisEmbeddingStoreTest.should_add_embedding_with_segment_with_metadata -- Time elapsed: 0.086 s <<< FAILURE!
java.lang.AssertionError: 

Expecting actual:
  0.9465928971765
to be close to:
  1.0
by less than 1% but difference was 5.34071028235%.
(a difference of exactly 1% being considered valid)
	at dev.langchain4j.store.embedding.EmbeddingStoreIT.should_add_embedding_with_segment_with_metadata(EmbeddingStoreIT.java:46)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:500)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:414)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@jmartisk
Copy link
Collaborator

jmartisk commented Jul 4, 2024

Yeah, looking.

@jmartisk
Copy link
Collaborator

jmartisk commented Jul 4, 2024

Ok so the problem is this new line:

https://github.com/langchain4j/langchain4j/blob/0.32.0/langchain4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreIT.java#L97

which adds a metadata field or type UUID, and I was only expecting numbers and strings when 'guessing' whether the corresponding field in Redis should be type NUMERIC or TEXT, and my guessing mechanism guesses that this is a number. But when you place a UUID (as a string) into such field and create an embedding for it, Redis won't return it from the index search operation, because it's invalid (unfortunately, it won't throw any error...).

So I think we can, in this case adjust the guessing mechanism to treat the UUID as a String instead of a Number.

But, FYI @langchain4j , maybe we should put in place some rules about what values you can put in the metadata. You probably shouldn't be allowed to put any arbitrary objects in there, because it wouldn't be clear how to store them in each underlying embedding store type, and how to perform equals, greaterThan, etc. operations on it. UUID is probably ok because it can be stored as a string. But I'm thinking maybe we should restrict Metadata values to be String or Number only.

@jmartisk
Copy link
Collaborator

jmartisk commented Jul 4, 2024

@langchain4j Ah, sorry I just noticed you already have some checks in place, and explicitly allow UUID along with numbers and strings. That's all good then :)

@jmartisk
Copy link
Collaborator

jmartisk commented Jul 4, 2024

Ok great, now we have some failures in Milvus instead

@geoand
Copy link
Collaborator Author

geoand commented Jul 4, 2024

lovely...

@geoand
Copy link
Collaborator Author

geoand commented Jul 4, 2024

I squashed the the last two (test fix) commits into the first one so we don't have broken intermediate states

@geoand geoand merged commit 45f0272 into main Jul 4, 2024
12 checks passed
@geoand geoand deleted the langchain4j-0.32 branch July 4, 2024 14:35
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.

Tracker for upgrading to LangChain4j 0.32
2 participants