-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(vectorsearch): add create/list/delete index samples #9880
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
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.
Hi @ericgribkoff and reviewers,
Gemini is currently reviewing this pull request and will post a full review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:
This PR adds sample code for creating, listing, and deleting vector indexes in Vertex AI. The intent is to provide clear, concise examples for developers using the Java client library. The changes include:
- Adding four new Java files:
CreateIndexSample.java
,CreateStreamingIndexSample.java
,DeleteIndexSample.java
, andListIndexesSample.java
to theaiplatform/src/main/java/aiplatform/vectorsearch
directory. These files contain the sample code for the respective operations. - Updating the
aiplatform/pom.xml
file to include necessary dependencies for the Vertex AI Vector Search API. Specifically, the following dependencies were added:grpc-google-cloud-aiplatform-v1
,grpc-testing
. - Adding unit tests for the new sample code in
CreateIndexSampleTest.java
,CreateStreamingIndexSampleTest.java
,DeleteIndexSampleTest.java
, andListIndexesSampleTest.java
.
All changes are well-documented and follow the sample format guide. The tests are comprehensive and cover all aspects of the functionality. I'll provide a more detailed review shortly.
And now, a little haiku to brighten your day:
Code flows like a stream,
Tests run green, a joyful sight,
Vectors find their place.
Thanks,
Gemini
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.
This pull request adds samples for creating, listing, and deleting vector search indexes. The implementation looks good overall, and the provided tests seem comprehensive. However, there are a few minor suggestions for improvement and some missing tests:
- The
contentsDeltaUri
variable is declared in themain
methods but not used in the corresponding sample methods (createIndexSample
andcreateStreamingIndexSample
). It's currently only used in the string formatting formetadataJson
. Consider passing it directly to the sample methods and using it to set thecontentsDeltaUri
field in the metadata. Also, clarify the purpose ofcontentsDeltaUri
in the sample code and ensure it's used correctly. - The
metadataJson
string is hardcoded within themain
methods. Consider making it a parameter of the sample methods or loading it from a configuration file to make the samples more flexible. - Add more comprehensive Javadoc documentation for the sample methods, explaining the purpose of each parameter and the expected behavior. Include examples of how to use the methods in the documentation.
- Add a section to the README with examples of how to use these new samples.
- Add integration tests that interact with the actual Vertex AI service to verify the end-to-end functionality of the samples.
aiplatform/src/main/java/aiplatform/vectorsearch/CreateStreamingIndexSample.java
Show resolved
Hide resolved
aiplatform/src/main/java/aiplatform/vectorsearch/CreateStreamingIndexSample.java
Show resolved
Hide resolved
aiplatform/src/main/java/aiplatform/vectorsearch/DeleteIndexSample.java
Outdated
Show resolved
Hide resolved
d6c48b5
to
1431d6a
Compare
The sample methods are themselves examples of using the API and the method names are self-explanatory; I don't think additional Javadoc provides any value, and this matches with the other samples in the repository.
There is no README, nor do I see a need for one - these samples will exist at https://cloud.google.com/vertex-ai/docs/vector-search/create-manage-index
I intentionally did not do this; these are samples using the GAPIC API, and the unit tests use stubs for the gRPC vertex API service so that we can validate that the intended API calls are sent by the GAPIC API. E.g., the GAPIC API codegen is fully exercised by the tests in this sample. Validating that the API itself "works" is far beyond the scope of a sample, requires muc greater testing that lives elsewhere, and some of these operations (e.g., create index) are costly and time consuming to run on every commit for sample code. |
Description
Part of b/378713849
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only