-
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): Elastic Text-Embedding Model demo. #4127
feat(vertexai): Elastic Text-Embedding Model demo. #4127
Conversation
f5e6632
to
20f37c5
Compare
This PR is modifying a sample that's currently in conflict with https://github.com/GoogleCloudPlatform/golang-samples/blob/main/aiplatform/text-embeddings/embeddings.go. One of these samples should be removed or the region tag changed. |
aiplatform/snippets/embeddings.go
Outdated
apiEndpoint, project, model string, texts []string, | ||
task string, customOutputDimensionality *int) ([][]float32, error) { |
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.
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.
SG, I'll hard-coded some of them and only leave project
and texts.
aiplatform/snippets/embeddings.go
Outdated
apiEndpoint, project, model string, texts []string, | ||
task string, customOutputDimensionality *int) ([][]float32, error) { |
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.
From https://googlecloudplatform.github.io/samples-style-guide/#minimal-arguments, we should be using minimal arguments.
blocker: apiEndpoint
is generally not specified in samples and should be removed. Looking at the code, it seems like maybe the problem is there's a missing method in the library to facilitate text embedding? If so we can make this not-blocker.
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.
SG
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if len(embeddings) != len(texts) || len(embeddings[0]) != 768 { | ||
t.Errorf("len(embeddings), len(embeddings[0]) = %d, %d, want %d, 768", len(embeddings), len(embeddings[0]), len(texts)) | ||
if len(embeddings) != len(texts) || len(embeddings[0]) != dimensionality { |
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.
suggestion: consider improving readability by extracting repeating len
calls to 'got' variables.
aiplatform/snippets/embeddings.go
Outdated
@@ -28,7 +28,8 @@ import ( | |||
) | |||
|
|||
func embedTexts( | |||
apiEndpoint, project, model string, texts []string, task string) ([][]float32, error) { | |||
apiEndpoint, project, model string, texts []string, | |||
task string, customOutputDimensionality *int) ([][]float32, error) { |
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.
From https://googlecloudplatform.github.io/samples-style-guide/#minimal-arguments, we should be using minimal arguments.
suggestion: Remove customOutputDimensionality
as a parameter in favor of hard-coding in the function body.
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.
SG
Here is the summary of changes. You are about to delete 1 region tag.
This comment is generated by snippet-bot.
|
Removed duplicated files under ./text-embeddings/ |
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 think I missed this feedback about specifying the endpoint on my last review, sorry. This should be the last adjustment.
aiplatform/snippets/embeddings.go
Outdated
ctx := context.Background() | ||
|
||
apiEndpoint := "us-central1-aiplatform.googleapis.com:443" |
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.
blocker: samples do not specify the API Endpoint unless specifically needed for the sample. Setting the region explicitly is typical.
I just checked https://pkg.go.dev/cloud.google.com/go/aiplatform/apiv1beta1/aiplatformpb#DirectPredictRequest, and realized specifying the Endpoint is required. This is not typical and I assumed it could be replaced with a direct "Location" setting. I'm sure that meant my previous feedback about Endpoint/Location was confusing. Thank you for your patience through my misunderstanding of the API design. |
Description
Fixes: http://b/334964454
Checklist
go test -v ./..
(see Testing)gofmt
(see Formatting)go vet
(see Formatting)