Skip to content

Conversation

@glasnt
Copy link
Contributor

@glasnt glasnt commented May 29, 2025

Description

Tries to run tests in a branch from code changes in #4098

@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: aiplatform Issues related to the AI Platform API. labels May 29, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @glasnt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request. This PR, authored by glasnt, is a debugging effort aimed at replicating a CI issue encountered in pull request #4098. The core change involves updating the predict-text-embeddings.js snippet and its corresponding test to use the gemini-embedding-001 model instead of the previous text-embedding-005 (or text-embedding-004 in the test). Additionally, the snippet's prediction logic has been modified to send text instances one by one, as required by the new model, rather than in a single batch.

Highlights

  • Debugging CI Issue: This pull request is specifically created to replicate and debug a CI failure that occurred in pull request #4098 by applying the relevant code changes to a separate branch.
  • Model Update: The text embedding snippet and its test are updated to use the gemini-embedding-001 model.
  • Prediction Logic Adjustment: The predict-text-embeddings.js snippet is modified to iterate through input text instances and send them individually to the prediction service, accommodating the requirements of the new gemini-embedding-001 model.

Changelog

  • ai-platform/snippets/predict-text-embeddings.js
    • Updated the default model name from text-embedding-005 to gemini-embedding-001 (line 23).
    • Refactored the prediction logic to loop through each text instance and make a separate prediction request for each, collecting the results into allEmbeddings (lines 45-59).
  • ai-platform/snippets/test/predict-text-embeddings.test.js
    • Updated the model name used in the test execution command from text-embedding-004 to gemini-embedding-001 (line 40).
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


A bug appears, the CI fails,
A new branch sprouts, where testing trails.
With model changed, and loop refined,
A fix for #4098, we hope to find.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the text embedding snippet to use the gemini-embedding-001 model, replacing older models. The core logic in predict-text-embeddings.js has been adapted to handle the new model's requirement of processing one text input at a time by introducing a loop for API calls. The corresponding test in predict-text-embeddings.test.js has also been updated to use the new model name.

Overall, the changes are clear and directly address the PR's goal of testing with the new model. The refactoring to initialize the PredictionServiceClient once, outside the new prediction loop, is a good efficiency improvement. One area for potential enhancement is the error handling within the loop, which is detailed in a specific comment.

Summary of Findings

  • Error Handling in Prediction Loop (Medium Severity): The prediction loop in predict-text-embeddings.js (lines 47-59 RIGHT) currently halts on the first API error. A review comment has been added suggesting consideration for individual error handling per instance to potentially allow other instances to be processed.
  • Code Adaptation and Efficiency (Positive): The update to use the gemini-embedding-001 model and the necessary adaptation of the code to its single-input processing requirement (lines 45-59 RIGHT in predict-text-embeddings.js) are well-implemented. Moving the PredictionServiceClient initialization (line 41 RIGHT) outside the loop is an appropriate efficiency gain. The test updates are also consistent with these changes.
  • Minor Stylistic Issues (Low Severity - Not Commented): There are a couple of added empty lines (e.g., line 40 and line 61 in predict-text-embeddings.js). These are purely stylistic and considered low severity. Per the review settings, no direct comments were made on these.

Merge Readiness

The pull request successfully updates the snippet and its tests to use the gemini-embedding-001 model, adapting the logic for single-instance predictions. The changes are clear and functional.

There is one medium-severity suggestion regarding error handling within the new prediction loop. Discussing and potentially addressing this point could enhance the snippet's robustness, depending on its intended use and error tolerance.

As a reviewer, I am not authorized to approve pull requests. It is recommended that the author considers the feedback provided, and that the PR undergoes further review and approval by authorized maintainers before merging. Addressing the error handling suggestion would make the PR more robust.

Comment on lines +47 to +59
for (const instance of instances) {
const request = {endpoint, instances: [instance], parameters};
const [response] = await client.predict(request);
const predictions = response.predictions;

const embeddings = predictions.map(p => {
const embeddingsProto = p.structValue.fields.embeddings;
const valuesProto = embeddingsProto.structValue.fields.values;
return valuesProto.listValue.values.map(v => v.numberValue);
});

allEmbeddings.push(embeddings[0])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation iterates through each text instance to get its embedding, making an API call for each. If one of these client.predict() calls fails, the entire callPredict function will reject, and the loop will terminate, preventing subsequent instances from being processed.

Could you clarify if this fail-fast behavior is intended for this snippet? While it might be acceptable, especially for a debugging/testing branch, if more resilience is desired (e.g., to collect embeddings for successful instances even if some fail), we might consider adding a try...catch block inside the for...of loop.

For example, you could adapt the loop like this:

    const allEmbeddings = [];
    // gemini-embedding-001 takes one input at a time.
    for (const instance of instances) {
      try {
        const request = {endpoint, instances: [instance], parameters};
        const [response] = await client.predict(request);
        const predictions = response.predictions;

        // Assuming predictions[0] contains the embedding structure for the single instance
        const embeddingData = predictions[0].structValue.fields.embeddings;
        const valuesProto = embeddingData.structValue.fields.values;
        const singleEmbedding = valuesProto.listValue.values.map(v => v.numberValue);

        allEmbeddings.push(singleEmbedding);
      } catch (error) {
        // Safely access instance content for logging, if available
        const contentValue = instance.structValue?.fields?.content?.stringValue;
        const instanceIdentifier = contentValue ? `"${contentValue}"` : 'current instance';
        console.error(`Failed to get embedding for ${instanceIdentifier}. Error: ${error.message}`);
        // Optional: Push a placeholder (e.g., null) or skip, or collect errors
      }
    }

What are your thoughts on enhancing error handling for individual instances in this context?

@glasnt glasnt closed this May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: aiplatform Issues related to the AI Platform API. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant