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

Vector Store polish #686

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

Conversation

joshlong
Copy link
Member

@joshlong joshlong commented May 6, 2024

general low hanging optimization and polish

  • refactored the VectorStore implementations so that if they implemented InitializingBean, they now implement ApplicationListener<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.
  • who is the author of ChromaVectorStore? Should we add their author tag? do we need ChromaVectorStore#SIMILARITY_THRESHOLD_ALL and ChromaVectorStore#SIMILARITY_THRESHOLD_ALL
  • renamed the module/artifact for spring-ai-azure-vector-store to spring-ai-azure-store to match other vector stores
  • renamed a few directories to match their artifactIds
  • made sure pom.xml name elements looked consistent (try running ./mvnw clean and you'll see the output is nicer)

@tzolov tzolov self-assigned this May 7, 2024
@tzolov tzolov added this to the 1.0.0-M1 milestone May 7, 2024
@markpollack markpollack self-assigned this May 8, 2024
Copy link
Collaborator

@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.

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>
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

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.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no wildcard imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants