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
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)