Skip to content

Conversation

JM-Lab
Copy link
Contributor

@JM-Lab JM-Lab commented Jan 9, 2024

This PR adds supports for Elasticsearch vector search.

To achieve compatibility with both Elasticsearch versions 7 and 8, I implemented it using Elasticsearch's Java Low Level REST Client.

To-do:

  • Add ElasticsearchAiSearchFilterExpressionConverter
  • Add support for FilterExpression
  • Add support for various vector functions provided by Elasticsearch

@tzolov tzolov self-assigned this Jan 13, 2024
@tzolov tzolov added vector store enhancement New feature or request labels Jan 13, 2024
@JM-Lab JM-Lab marked this pull request as ready for review January 14, 2024 10:07
@JM-Lab
Copy link
Contributor Author

JM-Lab commented Jan 14, 2024

Just finished up the Elasticsearch Vector Store support.
Next, I'll be working on adding support for OpenSearch (https://opensearch.org/) vector store.
Additionally, breaking down tasks for separate PR:

  • Add autoconfiguration
  • Write README

Let me know your thoughts.

@markpollack
Copy link
Member

This is a great contribution. I believe @tzolov has started to review. I'll put it into 0.8.0 as a stretch goal for now.

@markpollack markpollack added this to the 0.8.0 milestone Jan 23, 2024
Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

@JM-Lab, thank you for the contribution.

I started with a quick code check and have few questions related to the chosen ES client library and consequently the custom vs library query classes/records.

<version>${parent.version}</version>
</dependency>

<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be using the elasticsearch-java client instead?

    <dependency>
      <groupId>co.elastic.clients</groupId>
      <artifactId>elasticsearch-java</artifactId>
      <version>xxx</version>
    </dependency>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to use the Java Low Level REST Client (https://www.elastic.co/guide/en/elasticsearch/client/java-api-client/8.12/java-rest-low.html) with minimal dependencies instead of the elasticsearch-java library's High Level Rest Client. This decision was based on the High Level Rest Client's heavy reliance on dependencies and its sensitivity to Elasticsearch server versions, requiring exact matching with versions 7, 8, and even minor releases. By implementing only the essential data classes for Vectorstore, I successfully tested compatibility with both version 7 and 8.

public static final String COSINE_SIMILARITY_FUNCTION =
"(cosineSimilarity(params.query_vector, 'embedding') + 1.0) / 2";

public record ElasticsearchBulkIndexId(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all those custom records? Wouldn't the co.elastic client already provide such?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I focused on simplicity by implementing only the necessary custom classes/records for Vectorstore, minimizing dependencies and Elasticsearch version sensitivity as mentioned earlier.

@JsonProperty("query")
Query query
) {
public record Query(
Copy link
Contributor

Choose a reason for hiding this comment

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

For example the isn't this already covered by the co.elastic.clients.elasticsearch._types.query_dsl.Query ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I focused on simplicity by implementing only the necessary custom classes/records for Vectorstore, minimizing dependencies and Elasticsearch version sensitivity as mentioned earlier.


private ElasticsearchScriptScoreQuery getElasticsearchSimilarityQuery(List<Double> embedding, int topK,
float similarityThreshold, Filter.Expression filterExpression) {
return new ElasticsearchScriptScoreQuery(topK, new Query(
Copy link
Contributor

Choose a reason for hiding this comment

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

Again wouldn't the co.elastic.clients.elasticsearch._types.query_dsl.ScriptScoreQuery help here in places of custom records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I focused on simplicity by implementing only the necessary custom classes/records for Vectorstore, minimizing dependencies and Elasticsearch version sensitivity as mentioned earlier.

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Feb 3, 2024

If we use elasticsearch-java, we might need to either split Vectorstore for Elasticsearch 7 and 8, or let users add elasticsearch-java version matching their elasticsearch server version. What's your preference? I'd love to hear your thoughts on this.

@markpollack markpollack modified the milestones: 0.8.0, 0.9.0 Feb 5, 2024
@markpollack
Copy link
Member

I think moving to use the latest version that focuses on elastic search 8 and to build upon the elasticsearch-java will be the best use of everyone's time. Sorry for the delay in responding, but we are still very interested in this contribution and appreciate your interest/help.

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Mar 2, 2024

@markpollack I understand, and I'll update to support the latest version, Elasticsearch 8, and commit soon.

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Mar 3, 2024

@markpollack @tzolov Review code with Elasticsearch 8 support using elasticsearch-java library as discussed.

tzolov and others added 10 commits March 4, 2024 13:58
…tability

  VertexAiGeminiAutoConfiguration creates its own FunctionCallbackContext
  instance with schema type set to SchemaType.OPEN_API_SCHEMA.
  This ensures that one other (non OPEN_API_SCHEMA) FucntionCallbackContext
  Bean overrds it.
 - Make it use the new OpenAiAudioApi.
 - Remove trascription code from spring-ai-core. Too early to generalize.
   Move all related code under the spring-ai-openai project.
 - Fix missing licenses and javadocs.
 - Add 'Audio' prefix for Transcription classes and packages.
 - Add missing auto-configuraiotn and tests.
 - Move the OpenAiAudioTranscriptionClient and OpenAiAudioTranscriptionOptions under the org.springframework.ai.openai package.
 - Rename the AudioTranscriptionRequest into AudioTranscriptionPrompt.
 - Minor imports cleaning.
* root project pom now sets <dependencyManagement>remove</dependencyManagement>
* spring-ai-bom pom flatten plugin added with <dependencyManagement>keep</dependencyManagement>
@tzolov
Copy link
Contributor

tzolov commented Mar 15, 2024

Hi @JM-Lab ,
Finally i had some time today to start reviewing the PR.
After rebasing it on top of upstream main there were some compilation issues i had to fix.
Also i had to add the project the parent POM and to the BOM pom.

After this the filter expression test passes but the IT fails for half of the tests.
Here is my review branch: https://github.com/tzolov/spring-ai/pull/new/gh-234-pr

Please have a look, hopefully you would be able to resolve those.

tzolov and others added 14 commits March 16, 2024 10:53
  - tename /api/clients/ into /api/chat
  - move the the image from /api/clients to /api
  - fix the layout inside the chat and embeddings docs. Moving the runtime options and sample controllers at top level.
  - adjust all affected links.
 - Add VectorSearchAggregation used to actually preform the search
   on a given collection with embeddings.
 - add MongoDBVectorStore
 - Add MongoDBVectorStoreIT.  Integration test runs fine given...
   - You have a mongo atlas cluster to connect to (local or remote)
   - You have the search index "spring_ai_vector_search" setup correctly
   - Need to explore getting around this
   - Need to filter results using threshold
 - Add postfilter for threshold values - While a post filter is not ideal,
   it gets the job done. The mongo team seems to be working on having
   it availible as a prefilter option, in which this implementation
   can be updated to use later.
 - implement filtering threshold
 - fix a few sonar issues
 - formatting
 - use higher default num_candidates
 - use builder for configuration
 - add documentation and some refactor
 - use consistent property in integration test
 - finish implementing filter support
 - add documentation to filter converter
 - add vector search index auto creation

 - Add to BOM.
 - Fix version to 1.0.0-SN.
 - Move expresion converter from core to models/mongodb.
 - Fix style and license headers
 - Implemented Bedrock Jurassic ChatClient
 - added documentation reference
 - implement auto-configuration and boot starter

 - Disable the BedrockAi21Jurassic2ChatClientIT.emojiPenaltyWhenTrueByDefaultApplyPenaltyTest() test
   as it  fails when run in combination with the other tests.
`Reuse Container` is a Testcontainers experimental feature. It requires
`testcontainers.reuse.enable=true` in `~/.testcontainers.properties` in
order to take effect but in order to avoid surprises, this commit remove it.

See https://java.testcontainers.org/features/reuse/
@JM-Lab
Copy link
Contributor Author

JM-Lab commented Mar 17, 2024

Hi @tzolov

I've reviewed the PR.
I cherry-picked your commit 'Rebase, fix compilation and style errors, add missing dependencies in parent pom and Bom pom' and resolved the issues with Elasticsearch8VectorStoreIT. Additionally, I renamed it to ElasticsearchVectorStoreIT.

Please review the changes. Looking forward to your feedback!

@tzolov
Copy link
Contributor

tzolov commented Mar 18, 2024

Thanks @JM-Lab ,
75 commits and 656 files changed, doesn't seem right.
Can you please squash the commits and make sure that only the ElasticSearch related files a touched.
It will be even better if you copy the ElasticSearch related files into a fresh new PR on top of the upstream main.

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Mar 19, 2024

@tzolov

I've just created a new PR focusing solely on the ElasticSearch files, on the main branch. You can find it here: #471.

Going to close this current one. Thanks!

@tzolov
Copy link
Contributor

tzolov commented Mar 19, 2024

Thanks @JM-Lab I will have a look

@JM-Lab JM-Lab deleted the elasticsearch-vector-store branch March 22, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request vector store

Projects

None yet

Development

Successfully merging this pull request may close these issues.