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

Feat/metada with azure ai search #1291

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fb33
Copy link
Contributor

@fb33 fb33 commented Jun 14, 2024

Issue

#1263

Change

add a mapper of filter to azure ai search filter string base on the default index structure with metadata.

General checklist

  • [X ] There are no breaking changes
  • [X ] I have added unit and integration tests for my change
  • [X ] I have manually run all the unit and integration tests in the module I have added/changed, and they are all green
  • I have manually run all the unit and integration tests in the core and main modules, and they are all green

Checklist for adding new model integration

  • I have added my new module in the BOM

Checklist for adding new embedding store integration

  • I have added a {NameOfIntegration}EmbeddingStoreIT that extends from either EmbeddingStoreIT or EmbeddingStoreWithFilteringIT
  • I have added my new module in the BOM

Checklist for changing existing embedding store integration

  • I have manually verified that the {NameOfIntegration}EmbeddingStore works correctly with the data persisted using the latest released version of LangChain4j

@fb33
Copy link
Contributor Author

fb33 commented Jun 14, 2024

Ping @jdubois

@langchain4j
Copy link
Owner

@fb33 thanks a lot!

Please make sure EmbeddingStoreWithFilteringIT.should_filter_by_metadata and should_filter_by_metadata_not are passing for Azure implementations.

cc @jdubois

@fb33 fb33 force-pushed the feat/metada-with-azure-ai-search branch from e7e7b71 to 65a5874 Compare June 17, 2024 17:35
@fb33
Copy link
Contributor Author

fb33 commented Jun 17, 2024

@jdubois I've made a few changes to make it possible to create and use specific mapper implementations.
I change AzureAiSearchContentRetrieverIT to extend EmbeddingStoreWithFilteringIT as mention by @langchain4j but I don't run it 😁

@langchain4j
Copy link
Owner

@fb33 could you run it? :)

@fb33
Copy link
Contributor Author

fb33 commented Jun 18, 2024

I don't have the well configured environment for that... The IT in azure-ai-search module are not managed with TestContainer so I need to have the good env in my Azure subscription.

@jdubois
Copy link
Contributor

jdubois commented Jun 18, 2024

@fb33 I've run it, unfortunately I'm getting errors, here's a screenshot
Screenshot 2024-06-18 at 17 21 00

@fb33
Copy link
Contributor Author

fb33 commented Jun 18, 2024

Ok, I'll have a look. Do I need some specific index definition ?
I quickly ran it this morning and all the tests was failed. I didn't have time to look for the root cause.

I create an empty azure ai search resource and the IT from the main branch are Ok, so I'll be able to investigate.

fb33 added 4 commits June 18, 2024 22:31
Create a AzureAiSearchFilterMapper to manage azure filter query format.

Signed-off-by: François Barbe <[email protected]>

langchain4j#1263
Add a filter in the constructor.
Switch from the deprecated method findRelevant to the new search method.
EmbeddingSearchRequest allows to pass the filter to use.

Signed-off-by: François Barbe <[email protected]>

langchain4j#1263
create an interface to open the mapping to other structure of index.
change static declaration to instance, and manage mapper with builders

Signed-off-by: François Barbe <[email protected]>

langchain4j#1263
@fb33 fb33 force-pushed the feat/metada-with-azure-ai-search branch from 65a5874 to fd06cf1 Compare June 18, 2024 20:55
@fb33
Copy link
Contributor Author

fb33 commented Jun 18, 2024

@jdubois It should be ok now.

@jdubois
Copy link
Contributor

jdubois commented Jun 19, 2024

@fb33 I'm still getting 2 errors:

DefaultAzureAiSearchFilterMapperTest.map_handlesComplexFilter()

org.opentest4j.AssertionFailedError: 
Expected :(metadata/attributes/any(k: k/key eq 'key1' and k/value eq 'value1') and (metadata/attributes/any(k: k/key eq 'key2' and not search.in(k/value, ('value2, value3'))) or metadata/attributes/any(k: k/key eq 'key3' and k/value gt '100')))
Actual   :(metadata/attributes/any(k: k/key eq 'key1' and k/value eq 'value1') and ((not metadata/attributes/any(k: k/key eq 'key2' and search.in(k/value, ('value2, value3')))) or metadata/attributes/any(k: k/key eq 'key3' and k/value gt '100')))
<Click to see difference>


	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
	at dev.langchain4j.rag.content.retriever.azure.search.DefaultAzureAiSearchFilterMapperTest.map_handlesComplexFilter(DefaultAzureAiSearchFilterMapperTest.java:75)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)

And DefaultAzureAiSearchFilterMapperTest.map_handlesIsNotIn() :

org.opentest4j.AssertionFailedError: 
Expected :metadata/attributes/any(k: k/key eq 'key1' and not search.in(k/value, ('value1, value2')))
Actual   :(not metadata/attributes/any(k: k/key eq 'key1' and search.in(k/value, ('value1, value2'))))
<Click to see difference>


	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
	at dev.langchain4j.rag.content.retriever.azure.search.DefaultAzureAiSearchFilterMapperTest.map_handlesIsNotIn(DefaultAzureAiSearchFilterMapperTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)

@fb33
Copy link
Contributor Author

fb33 commented Jun 19, 2024

Sorry, I was focus on the IT test, I forgot to align my TU... It's done now.

@fb33
Copy link
Contributor Author

fb33 commented Jun 21, 2024

@langchain4j @jdubois, did this PR go to the right way for you ?

@jdubois
Copy link
Contributor

jdubois commented Jun 21, 2024

@fb33 yes, I confirm all the tests pass on my side!

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