-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(vertexai): add google_vertex_ai_index for Vertex AI Matching Engine #6728
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @melinath, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 1447 insertions(+), 2 deletions(-)) |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 1447 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccVertexAIIndex_vertexAiIndexExample|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
A local unit test run failed due to logs2022-10-20T23:30:42.955-0700 [ERROR] sdk.helper_resource: Unexpected error: test_name=TestAccVertexAIIndex_vertexAiIndexExample test_step_number=1 test_terraform_path=/opt/homebrew/bin/terraform test_working_directory=/var/folders/kd/2mm1sqn16s96zq819f1504r80000gp/T/plugintest2392748359
error=
| After applying this test step, the plan was not empty.
| stdout:
|
|
| Terraform used the selected providers to generate the following execution
| plan. Resource actions are indicated with the following symbols:
| -/+ destroy and then create replacement
|
| Terraform will perform the following actions:
|
| # google_vertex_ai_index.index must be replaced
| -/+ resource "google_vertex_ai_index" "index" {
| ~ create_time = "2022-10-21T05:39:21.900880Z" -> (known after apply)
| ~ deployed_indexes = [] -> (known after apply)
| ~ etag = "AMEw9yPr9AaH6zIFf4fVKHE4-pdjDx1kYROQsHq3C-D4E22idlSiDQS0213sOLP9TwWb" -> (known after apply)
| ~ id = "projects/kouzoh-p-kohama/locations/us-central1/indexes/1299552375287054336" -> (known after apply)
| ~ metadata_schema_uri = "gs://google-cloud-aiplatform/schema/matchingengine/metadata/nearest_neighbor_search_1.0.0.yaml" -> (known after apply)
| ~ name = "1299552375287054336" -> "terraformn00mrw8m9i" # forces replacement
| ~ project = "kouzoh-p-kohama" -> (known after apply)
| ~ update_time = "2022-10-21T06:20:16.414646Z" -> (known after apply)
| # (4 unchanged attributes hidden)
|
| ~ metadata {
| + contents_delta_uri = "gs://kouzoh-p-kohama-tf-test-vertex-ai-index-testn00mrw8m9i/contents"
| # (1 unchanged attribute hidden)
|
| ~ config {
| # (4 unchanged attributes hidden)
|
| ~ algorithm_config {
| # (1 unchanged block hidden)
| }
| }
| }
| }
|
| Plan: 1 to add, 0 to change, 1 to destroy.
provider_test.go:315: Step 1/2 error: After applying this test step, the plan was not empty |
Hello @melinath, I could fix the above error, but I faced another error like below. I'm not sure why only contents_delta_uri was not stored in the terraform state. I'd appreciate it if I could get some advice on that if you've seen a similar error before. Thank you.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 1434 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccVertexAIIndex_vertexAiIndexExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
It turns out
|
I wanted to use |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 1435 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccVertexAIIndex_vertexAiIndexExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi @melinath, could you help me see the error messages in the VCR tests? The custom_flatten function fixed the drift regarding |
Error is:
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 6 files changed, 1535 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccVertexAIIndex_basic|TestAccVertexAIIndex_vertexAiIndexExample |
@melinath Thank you for your comment. I figured out what made it a failure. Because All tests passed. Could you please review this PR when you have time? Thanks |
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.
great, it looks like the test is now passing after ~40 minutes (which is the time it takes to create the resource) so that means the patch is probably basically instant if you just change the description. It sounds like you're already planning to take a look at whether the nested fields already work 💯
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 6 files changed, 1895 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccLoggingBucketConfigProject_cmekSettings|TestAccVertexAIIndex_updated |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 6 files changed, 1896 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccVertexAIIndex_updated |
@melinath OK, all tests passed again. Could you please review the updates when you have time? Happy holidays 🦃 |
LGTM assuming the manual test run passes; I'll let you know if there are any issues. |
Approved & merged, thanks! |
Thank you for reviewing this PR! I learned a lot from your concise advice! |
…ine (GoogleCloudPlatform#6728) * feat: add google_vertex_ai_index for Vertex AI Matching Engine * fix: increase timeouts to 60 min because 20 wasn't enough for creation * fix: change coe to make name computed instead of an input * fix: use costom flatten code to ignore_read a nested property's field * fix: add skip_import_test: true to the auto-gen test * feat: add a test with a manually updated ImportStateVerifyIgnore * Apply suggestions from code review [ci skip] Update descriptions based on the suggestions Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * refactor: use ignore_read_extra instead of a manual test * fix: use an empty object for bruteForceConfig * feat: define additional fields to api.yaml * feat: add an example to increase test coverage * feat: deal with contentsDeltaUri as an updatable field * fix: fix the error because the cosine distance type only supports unit l2 norm type This is the error message from the endpoint: "Index with `COSINE_DISTANCE` distanceMeasureType currently only supports `UNIT_L2_NORM` featureNormType." * feat: remove approximate_neighbors_count from an example with brute_force_config approximate_neighbors_count is required if tree-AH algorithm is used. from https://cloud.google.com/vertex-ai/docs/matching-engine/configuring-indexes#brute-force-config * test: add a handwritten test for patch * fix: add update_mask: true to use the mask as a url param * refactor: put 'input: true' on the fields patch couldn't update * feat: use custom pre update code for a nested object * fix: update the handwritten test accordingly * feat: add custom flatten code for is_complete_overwrite Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…ine (GoogleCloudPlatform#6728) * feat: add google_vertex_ai_index for Vertex AI Matching Engine * fix: increase timeouts to 60 min because 20 wasn't enough for creation * fix: change coe to make name computed instead of an input * fix: use costom flatten code to ignore_read a nested property's field * fix: add skip_import_test: true to the auto-gen test * feat: add a test with a manually updated ImportStateVerifyIgnore * Apply suggestions from code review [ci skip] Update descriptions based on the suggestions Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * refactor: use ignore_read_extra instead of a manual test * fix: use an empty object for bruteForceConfig * feat: define additional fields to api.yaml * feat: add an example to increase test coverage * feat: deal with contentsDeltaUri as an updatable field * fix: fix the error because the cosine distance type only supports unit l2 norm type This is the error message from the endpoint: "Index with `COSINE_DISTANCE` distanceMeasureType currently only supports `UNIT_L2_NORM` featureNormType." * feat: remove approximate_neighbors_count from an example with brute_force_config approximate_neighbors_count is required if tree-AH algorithm is used. from https://cloud.google.com/vertex-ai/docs/matching-engine/configuring-indexes#brute-force-config * test: add a handwritten test for patch * fix: add update_mask: true to use the mask as a url param * refactor: put 'input: true' on the fields patch couldn't update * feat: use custom pre update code for a nested object * fix: update the handwritten test accordingly * feat: add custom flatten code for is_complete_overwrite Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…ine (GoogleCloudPlatform#6728) * feat: add google_vertex_ai_index for Vertex AI Matching Engine * fix: increase timeouts to 60 min because 20 wasn't enough for creation * fix: change coe to make name computed instead of an input * fix: use costom flatten code to ignore_read a nested property's field * fix: add skip_import_test: true to the auto-gen test * feat: add a test with a manually updated ImportStateVerifyIgnore * Apply suggestions from code review [ci skip] Update descriptions based on the suggestions Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * refactor: use ignore_read_extra instead of a manual test * fix: use an empty object for bruteForceConfig * feat: define additional fields to api.yaml * feat: add an example to increase test coverage * feat: deal with contentsDeltaUri as an updatable field * fix: fix the error because the cosine distance type only supports unit l2 norm type This is the error message from the endpoint: "Index with `COSINE_DISTANCE` distanceMeasureType currently only supports `UNIT_L2_NORM` featureNormType." * feat: remove approximate_neighbors_count from an example with brute_force_config approximate_neighbors_count is required if tree-AH algorithm is used. from https://cloud.google.com/vertex-ai/docs/matching-engine/configuring-indexes#brute-force-config * test: add a handwritten test for patch * fix: add update_mask: true to use the mask as a url param * refactor: put 'input: true' on the fields patch couldn't update * feat: use custom pre update code for a nested object * fix: update the handwritten test accordingly * feat: add custom flatten code for is_complete_overwrite Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Part of hashicorp/terraform-provider-google#12818
Part of hashicorp/terraform-provider-google#9298
Add
google_vertex_ai_index
to create an index for Vertex AI Matching Engine.If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)