Revert "refactor(genai): migrate region tags from generative-ai to genai folder (Batch 2)"#4337
Conversation
…nai fold…" This reverts commit 7517761.
product-auto-label
Bot
added
samples
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request cleans up legacy Vertex AI snippet tags across several files and introduces a new suite of Google Gen AI SDK snippets and tests under generative-ai/snippets/. The feedback highlights a few key areas for improvement: in safetySettings.js, the setSafetySettings function should be refactored to accept parameters and command-line arguments to match established patterns, and its safety termination message logic should be corrected to avoid duplicate or unconditional printing. Additionally, several test files should be updated to pass synchronous callbacks to Mocha's describe blocks instead of async functions to avoid anti-patterns.
| const PROJECT_ID = process.env.CAIP_PROJECT_ID; | ||
| const LOCATION = 'us-central1'; | ||
| const MODEL = 'gemini-2.0-flash-001'; | ||
|
|
||
| async function setSafetySettings() { | ||
| // Initialize Vertex with your Cloud project and location | ||
| const vertexAI = new VertexAI({project: PROJECT_ID, location: LOCATION}); | ||
|
|
||
| // Instantiate the model | ||
| const generativeModel = vertexAI.getGenerativeModel({ | ||
| model: MODEL, |
There was a problem hiding this comment.
The setSafetySettings function does not accept any parameters and relies on global constants, unlike other snippets in this repository. This causes the test suite to pass command-line arguments that are completely ignored, and makes the snippet fail if process.env.CAIP_PROJECT_ID is not set. Refactor the function to accept projectId, location, and model as parameters with defaults.
| const PROJECT_ID = process.env.CAIP_PROJECT_ID; | |
| const LOCATION = 'us-central1'; | |
| const MODEL = 'gemini-2.0-flash-001'; | |
| async function setSafetySettings() { | |
| // Initialize Vertex with your Cloud project and location | |
| const vertexAI = new VertexAI({project: PROJECT_ID, location: LOCATION}); | |
| // Instantiate the model | |
| const generativeModel = vertexAI.getGenerativeModel({ | |
| model: MODEL, | |
| async function setSafetySettings( | |
| projectId = process.env.CAIP_PROJECT_ID || 'PROJECT_ID', | |
| location = 'us-central1', | |
| model = 'gemini-2.0-flash-001' | |
| ) { | |
| // Initialize Vertex with your Cloud project and location | |
| const vertexAI = new VertexAI({project: projectId, location: location}); | |
| // Instantiate the model | |
| const generativeModel = vertexAI.getGenerativeModel({ | |
| model: model, |
| for await (const item of responseStream.stream) { | ||
| if (item.candidates[0].finishReason === 'SAFETY') { | ||
| console.log('This response stream terminated due to safety concerns.'); | ||
| break; | ||
| } else { | ||
| process.stdout.write(item.candidates[0].content.parts[0].text); | ||
| } | ||
| } | ||
| console.log('This response stream terminated due to safety concerns.'); |
There was a problem hiding this comment.
The message 'This response stream terminated due to safety concerns.' is printed unconditionally at the end of the function, even if the stream completed successfully without any safety blocks. Additionally, if a safety block is encountered, the message is printed twice (once inside the loop and once after). Use a boolean flag to track if the stream was blocked by safety, and only print the message if that flag is true.
let safetyBlocked = false;
for await (const item of responseStream.stream) {
if (item.candidates[0].finishReason === 'SAFETY') {
safetyBlocked = true;
break;
} else {
process.stdout.write(item.candidates[0].content.parts[0].text);
}
}
if (safetyBlocked) {
console.log('\nThis response stream terminated due to safety concerns.');
}| setSafetySettings().catch(err => { | ||
| console.error(err.message); | ||
| process.exitCode = 1; | ||
| }); |
There was a problem hiding this comment.
Pass command-line arguments to setSafetySettings using process.argv.slice(2) to match the pattern used in other snippets and to allow the test suite to configure the project, location, and model dynamically. Note that we omit setting process.exitCode = 1 to maintain consistency with established boilerplate patterns in this repository.
setSafetySettings(...process.argv.slice(2)).catch(err => {
console.error(err.message);
});References
- Maintain established boilerplate patterns across samples within a repository, even if it means deviating from technically optimal solutions (e.g., keeping synchronous main functions and omitting process.exitCode = 1 for consistency).
| const projectId = process.env.CAIP_PROJECT_ID; | ||
| const model = 'gemini-2.5-flash'; | ||
|
|
||
| describe('Analyze video with audio', async () => { |
There was a problem hiding this comment.
Passing an async function to Mocha's describe block is an anti-pattern. describe is executed synchronously to register the test cases, and returning a promise from it can lead to unexpected behavior or unhandled rejections. The callback should be a regular synchronous function.
| describe('Analyze video with audio', async () => { | |
| describe('Analyze video with audio', () => { |
| const location = process.env.LOCATION; | ||
| const model = 'gemini-2.5-flash'; | ||
|
|
||
| describe('Generative AI NonStreaming Chat', async () => { |
There was a problem hiding this comment.
Passing an async function to Mocha's describe block is an anti-pattern. describe is executed synchronously to register the test cases, and returning a promise from it can lead to unexpected behavior or unhandled rejections. The callback should be a regular synchronous function.
| describe('Generative AI NonStreaming Chat', async () => { | |
| describe('Generative AI NonStreaming Chat', () => { |
| const location = process.env.LOCATION; | ||
| const model = 'gemini-2.0-flash-001'; | ||
|
|
||
| describe.skip('Safety settings', async () => { |
There was a problem hiding this comment.
Passing an async function to Mocha's describe block is an anti-pattern. describe is executed synchronously to register the test cases, and returning a promise from it can lead to unexpected behavior or unhandled rejections. The callback should be a regular synchronous function.
| describe.skip('Safety settings', async () => { | |
| describe.skip('Safety settings', () => { |
|
Here is the summary of changes. You are about to add 11 region tags.
You are about to delete 11 region tags.
This comment is generated by snippet-bot.
|
iennae
left a comment
iennae
left a comment
There was a problem hiding this comment.
I think you should be able to leave the region tag added to the files that you've verified it's the equivalent sample.
angelcaamal
deleted the
revert-4336-refactor/region-tags-generativeai-to-genai-batch2
branch
2 participants
Reverts #4336