-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(genai): Sample/rest of samples #4195
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
Conversation
|
Here is the summary of changes. You are about to add 5 region tags.
This comment is generated by snippet-bot.
|
Summary of ChangesHello @Guiners, 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! This pull request significantly enhances the Generative AI samples for Node.js by introducing several new examples that cover a range of advanced functionalities. These additions aim to provide developers with practical demonstrations of features such as image analysis with bounding boxes, text embeddings for retrieval, direct API key authentication, managing provisioned throughput, and implementing content safety measures. The new samples enhance the documentation and provide clearer guidance for integrating these GenAI capabilities into applications. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
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.
Code Review
This pull request adds several new GenAI samples and their corresponding tests. Overall, the additions are valuable, but there are several critical and high-severity issues that need to be addressed. These include a security vulnerability from a hardcoded API key placeholder, incorrect package versions in package.json that will cause installation to fail, and several bugs in the samples that could lead to crashes (e.g., when a response is blocked by safety filters). I've also noted inconsistencies in API client initialization and a misleading sample for provisioned throughput. Please review the detailed comments for specific suggestions.
|
|
||
| // [START googlegenaisdk_vertexai_express_mode] | ||
| const {GoogleGenAI} = require('@google/genai'); | ||
| const API_KEY = 'PUT HERE YOUR API KEY'; |
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.
Hardcoding credential placeholders like 'PUT HERE YOUR API KEY' is a significant security risk. A developer might replace the placeholder with a real key and accidentally commit it. Secrets should always be loaded from a secure source, such as environment variables. It's also good practice to add a check inside generateWithApiKey to ensure the key is present before making an API call.
| const API_KEY = 'PUT HERE YOUR API KEY'; | |
| const API_KEY = process.env.GOOGLE_API_KEY; |
| "uuid": "^10.0.0" | ||
| "uuid": "^10.0.0", | ||
| "proxyquire": "^2.1.3", | ||
| "canvas": "^3.1.0", |
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.
| "typescript": "^5.0.4" | ||
| }, | ||
| "dependencies": { | ||
| "canvas": "^3.1.2", |
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.
| async function generateEmbeddingsForRetrieval( | ||
| projectId = GOOGLE_CLOUD_PROJECT | ||
| ) { | ||
| const client = new GoogleGenAI(projectId); |
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.
This sample appears to be for Vertex AI, but the GoogleGenAI client is not initialized with vertexai: true. This will cause it to target the Google AI endpoint, which uses API keys, not project IDs. For correct functionality with a project ID, you should initialize it with an object containing vertexai: true, project: projectId, and a location. You will also need to update the function signature to accept a location and define GOOGLE_CLOUD_LOCATION at the top of the file for consistency with other samples.
| // - "dedicated": Use Provisioned Throughput | ||
| // - "shared": Use pay-as-you-go | ||
| // https://cloud.google.com/vertex-ai/generative-ai/docs/use-provisioned-throughput | ||
| 'X-Vertex-AI-LLM-Request-Type': 'shared', |
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.
The sample is named provisionedthroughput-with-txt.js and the comments indicate that 'dedicated' should be used for provisioned throughput. However, the code is using 'shared', which is for pay-as-you-go. This is misleading for a sample that is supposed to demonstrate provisioned throughput.
| 'X-Vertex-AI-LLM-Request-Type': 'shared', | |
| 'X-Vertex-AI-LLM-Request-Type': 'dedicated', |
| "commander": "^12.0.0", | ||
| "eslint": "^8.57.0" | ||
| "eslint": "^8.57.0", | ||
| "node-fetch": "^3.3.2", |
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.
You are adding node-fetch@^3.3.2, which is an ESM-only package. The root package.json does not have "type": "module", so it defaults to CommonJS. Any attempt to require('node-fetch') will fail. If this package is needed for CommonJS code, you should use node-fetch@^2.7.0, which is what the genai sub-package correctly does.
| "node-fetch": "^3.3.2", | |
| "node-fetch": "^2.7.0", |
| ctx.fillText(bbox.label, absXMin + 8, absYMin + 20); | ||
| }); | ||
|
|
||
| fs.writeFileSync('output.png', canvas.toBuffer('image/png')); |
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.
Hardcoding the output filename to output.png can be problematic as it might overwrite an existing file in the directory where the script is run. Consider making the output filename unique, for example by including a timestamp. You'll also need to update the log message on the next line to use the dynamic filename.
| fs.writeFileSync('output.png', canvas.toBuffer('image/png')); | |
| fs.writeFileSync(`output-${Date.now()}.png`, canvas.toBuffer('image/png')); |
| minItems: '4', | ||
| maxItems: '4', |
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.
| const candidate = response.candidates[0].content.parts[0].text; | ||
| const boundingBoxes = JSON.parse(candidate); |
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.
Accessing nested properties directly can lead to a TypeError if the response structure is not what's expected (e.g., if candidates is empty). Using optional chaining (?.) and providing a fallback will make the code more robust.
| const candidate = response.candidates[0].content.parts[0].text; | |
| const boundingBoxes = JSON.parse(candidate); | |
| const candidateText = response.candidates?.[0]?.content?.parts?.[0]?.text; | |
| const boundingBoxes = candidateText ? JSON.parse(candidateText) : []; |
| it('should call generateContentStream with safety instructions', async function () { | ||
| this.timeout(50000); | ||
| const output = await sample.generateWithSafetySettings(projectId); | ||
| assert(output.text.length > 0); |
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.
This assertion assert(output.text.length > 0) will fail if the response is blocked due to safety settings, as accessing .text will throw an error. The test should be more robust to handle this case. You could use a try...catch block to assert that either output.text has content, or that the finishReason is 'SAFETY' in the catch block.
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.