-
Notifications
You must be signed in to change notification settings - Fork 544
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
Vector Store polish #686
base: main
Are you sure you want to change the base?
Vector Store polish #686
Conversation
… down the application on startup.
…incompatabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of replacing the InitializingBean
with the ApplicationListener<ApplicationReadyEvent>
. Unfortunately this change breaks the IT tests. I'm not sure if this is test configuration issue or a problem with the lifcycle initializaiton.
@@ -8,10 +8,10 @@ | |||
<version>1.0.0-SNAPSHOT</version> | |||
<relativePath>../../pom.xml</relativePath> | |||
</parent> | |||
<artifactId>spring-ai-azure-vector-store</artifactId> | |||
<artifactId>spring-ai-azure-store</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to updated all places the spring-ai-azure-vector-store
including poms and docs
import com.azure.search.documents.models.SearchOptions; | ||
import com.azure.search.documents.models.VectorSearchOptions; | ||
import com.azure.search.documents.models.VectorizedQuery; | ||
import com.azure.search.documents.indexes.models.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring Framework code convention: Wildcard imports such as import java.util.* or import static org.assertj.core.api.Assertions.* are forbidden, even in test code.
https://github.com/spring-projects/spring-framework/wiki/Code-Style#import-statements
import org.springframework.util.Assert; | ||
import org.springframework.util.CollectionUtils; | ||
import org.springframework.util.StringUtils; | ||
|
||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import com.datastax.oss.driver.api.core.cql.PreparedStatement; | ||
import com.datastax.oss.driver.api.core.cql.Row; | ||
import com.datastax.oss.driver.api.core.cql.SimpleStatement; | ||
import com.datastax.oss.driver.api.core.cql.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import org.springframework.ai.vectorstore.CassandraVectorStoreConfig.SchemaColumn; | ||
import org.springframework.ai.vectorstore.filter.FilterExpressionConverter; | ||
import org.springframework.beans.factory.InitializingBean; | ||
|
||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import org.springframework.util.Assert; | ||
import org.springframework.util.CollectionUtils; | ||
import org.springframework.util.StringUtils; | ||
|
||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import io.qdrant.client.grpc.Collections.Distance; | ||
import io.qdrant.client.grpc.Collections.VectorParams; | ||
import io.qdrant.client.grpc.JsonWithInt.Value; | ||
import io.qdrant.client.grpc.Points.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import redis.clients.jedis.search.IndexDataType; | ||
import redis.clients.jedis.search.Query; | ||
import redis.clients.jedis.search.RediSearchUtil; | ||
import redis.clients.jedis.search.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import redis.clients.jedis.search.schemafields.TagField; | ||
import redis.clients.jedis.search.schemafields.TextField; | ||
import redis.clients.jedis.search.schemafields.VectorField; | ||
import redis.clients.jedis.search.schemafields.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
import redis.clients.jedis.search.schemafields.VectorField.VectorAlgorithm; | ||
|
||
import java.text.MessageFormat; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
Half of the changes in the PR were applied, however the approach to initializing the vector store though the use of a Spring Boot application event is concerning as ideally we do not want to take a dependency in the vector stores on Spring Boot. There is perhaps another approach that can be used, such an an initialization flag. Moving this to M2 to look into it more deeply another time. |
general low hanging optimization and polish
VectorStore
implementations so that if they implementedInitializingBean
, they now implementApplicationListener<ApplicationReadyEvent>
, instead. The result is that the vector stores now initialize their own state after the boot app has started. this is important because a) nobody wants to wait 2x as long for their app to run SQL statements or create collections or whatever, especially when developing, and nobody wants their Kubernetes health check to fail for stuff that can be done after the application's fully online.ChromaVectorStore
? Should we add their author tag? do we need ChromaVectorStore#SIMILARITY_THRESHOLD_ALL and ChromaVectorStore#SIMILARITY_THRESHOLD_ALLspring-ai-azure-vector-store
tospring-ai-azure-store
to match other vector storesartifactId
spom.xml
name
elements looked consistent (try running./mvnw clean
and you'll see the output is nicer)