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

Fix shoppingassistant demo #2675

Merged
merged 2 commits into from
Aug 12, 2024
Merged

Conversation

bourgeoisor
Copy link
Member

@bourgeoisor bourgeoisor commented Aug 12, 2024

This PR fixes a few issues with the shopping assistant demo and tweaks some of the instruction steps for added clarity.

How to review

@@ -82,7 +82,8 @@ gcloud alloydb instances create ${ALLOYDB_INSTANCE_NAME}-replica \
gcloud beta alloydb instances update ${ALLOYDB_INSTANCE_NAME} \
--cluster=${ALLOYDB_CLUSTER_NAME} \
--region=${REGION} \
--assign-inbound-public-ip=ASSIGN_IPV4
--assign-inbound-public-ip=ASSIGN_IPV4 \
--database-flags password.enforce_complexity=on
Copy link
Member Author

Choose a reason for hiding this comment

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

This flag is now required or else the AlloyDB instance update fails with an error.

@@ -119,9 +120,6 @@ gcloud projects add-iam-policy-binding ${PROJECT_ID} --member=serviceAccount:${A
gcloud projects add-iam-policy-binding ${PROJECT_ID} --member=serviceAccount:${ALLOYDB_USER_GSA_ID} --role=roles/alloydb.databaseUser
gcloud projects add-iam-policy-binding ${PROJECT_ID} --member=serviceAccount:${ALLOYDB_USER_GSA_ID} --role=roles/secretmanager.secretAccessor
gcloud projects add-iam-policy-binding ${PROJECT_ID} --member=serviceAccount:${ALLOYDB_USER_GSA_ID} --role=roles/serviceusage.serviceUsageConsumer
gcloud projects add-iam-policy-binding ${PROJECT_ID} --member=service-${PROJECT_NUMBER}@gcp-sa-alloydb.iam.gserviceaccount.com --role=roles/aiplatform.user
Copy link
Member Author

Choose a reason for hiding this comment

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

This line now fails with an error saying the member flag is in an invalid format. It doesn't seem needed (anymore).

@@ -28,6 +28,7 @@ FROM scratch

WORKDIR /src
COPY --from=builder /productcatalogservice ./server
COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
Copy link
Member Author

Choose a reason for hiding this comment

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

The productscatalog started failing with x509 issues, which this line fixes.

@@ -69,7 +69,7 @@ def talkToGemini():
prompt = unquote(prompt)

# Step 1 – Get a room description from Gemini-vision-pro
llm_vision = ChatGoogleGenerativeAI(model="gemini-pro-vision")
llm_vision = ChatGoogleGenerativeAI(model="gemini-1.5-flash")
Copy link
Member Author

Choose a reason for hiding this comment

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

The gemini-pro-vision model is deprecated and returned a 404.

@@ -34,7 +34,7 @@ patches:
- name: ALLOYDB_PRIMARY_IP
value: ALLOYDB_PRIMARY_IP_VAL
- name: ALLOYDB_DATABASE_NAME
value: ALLOYDB_CARTS_DATABASE_NAME
value: ALLOYDB_CARTS_DATABASE_NAME_VAL
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an omission; caught it on a re-run of the script.

Copy link

🚲 PR staged at http://34.72.98.16

Copy link
Collaborator

@Mukamik Mukamik left a comment

Choose a reason for hiding this comment

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

Lgtm

@@ -74,13 +75,16 @@ func getSecretPayload(project, secret, version string) (string, error) {
// Call the API.
result, err := client.AccessSecretVersion(ctx, req)
if err != nil {
log.Warnf("failed to access SecretVersion: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Warn logs are a great addition here

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to troubleshoot what was happening with the products catalog, and figured I'd leave them for the next time there's a potential issue :)

Copy link
Collaborator

@Mukamik Mukamik left a comment

Choose a reason for hiding this comment

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

Lgtm

@bourgeoisor bourgeoisor merged commit 9cdbc60 into main Aug 12, 2024
12 checks passed
@bourgeoisor bourgeoisor deleted the issue-fix-shoppingassistant-demo branch August 12, 2024 22:18
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.

2 participants