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

vertex-ai: add :UPLOAD support for model resource #12075

Open
wants to merge 14 commits into
base: vertex-ai-model-resource
Choose a base branch
from

Conversation

BBBmau
Copy link
Collaborator

@BBBmau BBBmau commented Oct 21, 2024

adding support of :UPLOAD for models resource. Once completed will be merged into base models PR: #12490

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.


@BBBmau BBBmau force-pushed the add-vertex-ai-model-COPY branch from fc631f3 to 8904529 Compare October 23, 2024 01:33
@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch 2 times, most recently from 25fafc1 to 5e6bf9b Compare October 23, 2024 01:56
@BBBmau
Copy link
Collaborator Author

BBBmau commented Oct 24, 2024

latest commit fixes some errors from initial running the basic_upload test. After running locally it works as intended.

Next is breaking up the list of model upload types.

@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch 5 times, most recently from 9a89dda to 05bb7e1 Compare October 29, 2024 23:14
@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch from 05bb7e1 to 2507926 Compare October 31, 2024 01:57
@BBBmau
Copy link
Collaborator Author

BBBmau commented Oct 31, 2024

recent commit reflects updates from add-vertex-model-COPY

@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch from 2507926 to f16163d Compare October 31, 2024 02:34
@BBBmau
Copy link
Collaborator Author

BBBmau commented Oct 31, 2024

ready for review after cleanup

explanationSpec is commented out since the coming PRs are meant to comment out sections one by one.

@BBBmau BBBmau marked this pull request as ready for review October 31, 2024 02:39
@BBBmau BBBmau changed the base branch from add-vertex-ai-model-COPY to vertex-ai-model-resource December 4, 2024 19:07
@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch from 38f197e to 46696d8 Compare December 4, 2024 20:53
Copy link
Collaborator Author

@BBBmau BBBmau left a comment

Choose a reason for hiding this comment

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

some thoughts after getting upload_basic to pass, though there is some things to consider.

What's next is supporting the copy tests with upload in order to:

  1. setup a model
  2. copy the pre-made model in the same test in order to test the functionality of copy without needing a model to always be present since nightly tests update these and we need to be explicit with the model ID

@@ -309,9 +318,9 @@ properties:
ignore_read: true
- type: Array
name: 'versionAliases'
Copy link
Collaborator Author

@BBBmau BBBmau Dec 5, 2024

Choose a reason for hiding this comment

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

Currently trying to resolve the diff we get on versionAliases where a user inputs their field [v2beta2] but get returned a diff on a second apply due to default_from_api returning [default, v2beta2]

although we could remove default_from_api this is necessary for copy to work since we don't input versionAlias yet we still expect a value from the model we want to copy.

In this case we can input the value but we should expect it to have default already as part of the config. I haven't been able to figure out a proper solution. A DiffSuppress was the first idea but because this is an array we are unable to use it (unless there's a hacky way that I don't know about. Leaving this for suggestions @SarahFrench

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After some further investigation we may want to consider removing this for the time being, there is currently no way to include suppot of StateFunc which would help with supporting this (more can be found here). A Diff_suppress_func can't be applied due to lack of support on Lists.

I was almost convinced to leave it with ignore_read: true though this leaves us with a case where we don't receive the version_aliases when copying (e.g the target model contains v1beta1 but instead is set as null when attempting to copy)

Because of the reasons above it may be best to remove this field. Any thoughts on this would be appreciated.

@@ -1,4 +1,8 @@
func vertexAiModelDiffSuppress(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) error {
// we require diffSuppress on COPY method and not UPLOAD
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is included so that we can fill out the description field on create when wanting to upload.

As a reminder this diffsuppress was created because of copy not supporting description field on create. More info can be found in this thread. #12074 (comment)

@BBBmau
Copy link
Collaborator Author

BBBmau commented Dec 18, 2024

recent commit supports UPLOAD for COPY tests, this allows us to not rely on a pre-existing model for source_model testing:

(base) ┌─(~/Dev/terraform-provider-google)────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s083)─┐
└─(18:53:19 on vertex-ai-model-base ✹ ✭)──> envchain GCLOUD make testacc TEST=./google/services/vertexai TESTARGS='-run=TestAccVertexAIModel_'
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google/services/vertexai -v -run=TestAccVertexAIModel_ -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccVertexAIModel_vertexAiModelUploadBasicExample
=== PAUSE TestAccVertexAIModel_vertexAiModelUploadBasicExample
=== RUN   TestAccVertexAIModel_postCreationUpdates
=== PAUSE TestAccVertexAIModel_postCreationUpdates
=== RUN   TestAccVertexAIModel_modelIdNotProvidedAtCreateTime
=== PAUSE TestAccVertexAIModel_modelIdNotProvidedAtCreateTime
=== RUN   TestAccVertexAIModel_modelIdProvidedAtCreateTime
=== PAUSE TestAccVertexAIModel_modelIdProvidedAtCreateTime
=== CONT  TestAccVertexAIModel_vertexAiModelUploadBasicExample
=== CONT  TestAccVertexAIModel_modelIdNotProvidedAtCreateTime
=== CONT  TestAccVertexAIModel_modelIdProvidedAtCreateTime
=== CONT  TestAccVertexAIModel_postCreationUpdates
--- PASS: TestAccVertexAIModel_vertexAiModelUploadBasicExample (23.28s)
--- PASS: TestAccVertexAIModel_modelIdNotProvidedAtCreateTime (34.93s)
--- PASS: TestAccVertexAIModel_modelIdProvidedAtCreateTime (35.33s)
--- PASS: TestAccVertexAIModel_postCreationUpdates (45.14s)
PASS
ok      github.com/hashicorp/terraform-provider-google/google/services/vertexai 46.573s

@BBBmau BBBmau force-pushed the vertex-ai-model-UPLOAD branch from 618905e to 8d7d880 Compare December 18, 2024 04:20
obj["displayName"] = obj["modelName"]
delete(obj, "modelName")
delete(obj, "sourceModel")
return obj, nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This update_encoder is necessary due to source_model being included in PATCH. The API Reference doesn't include any mention of source_model for patching existing models (such as for updating the description for a copied model)

Reference: https://cloud.google.com/vertex-ai/docs/reference/rest/v1/projects.locations.models/patch

@BBBmau
Copy link
Collaborator Author

BBBmau commented Dec 18, 2024

This is ready for another review. Only thing to consider is whether we want to keep investigating solutions for versionAlias as discussed in #12075 (comment) @SarahFrench

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

Successfully merging this pull request may close these issues.

1 participant