-
Notifications
You must be signed in to change notification settings - Fork 2k
chore(ci): migrate ai-platform/snippets via Node 16 #4103
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hello @davidcavazos, 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 davidcavazos, aims to address potential CJS (CommonJS) errors encountered by the ai-platform/snippets
tests by migrating their CI environment to use Node.js version 16. This change involves updating the CI configuration files to reflect the new testing setup, creating a dedicated CI configuration file for the snippets, updating package dependencies, and migrating the test files from require
syntax (CJS) to import
syntax (ESM).
Highlights
- CI Node Version Downgrade: The primary goal is to run the
ai-platform/snippets
tests using Node.js 16 instead of a newer version, specifically to mitigate issues related to CJS module compatibility. - CI Configuration Updates: The
.github/config/nodejs.jsonc
and.github/config/nodejs-dev.jsonc
files are updated to removeai-platform/snippets
from the list of failing/skipped tests in the main Node.js CI run and add it to the list of tests skipped in the dev CI run (as it will now pass in the dedicated prod CI run). - Dedicated CI Setup File: A new file,
ai-platform/snippets/ci-setup.json
, is added to define the specific CI environment for these snippets, explicitly setting thenode-version
to "16" and configuring secrets and timeout. - Dependency Updates: The
ai-platform/snippets/package.json
file sees updates to several dependencies, including@google-cloud/aiplatform
,@google-cloud/bigquery
,chai
,mocha
,uuid
, andsinon
. The test script is also modified to use the--reporter list
flag. - ESM Import Migration: Test files (
batch-prediction-gemini.test.js
andcreate-batch-prediction-job-text-classification.test.js
) are updated to use ES moduleimport
syntax instead of CommonJSrequire
.
Changelog
Click here to see the changelog
- .github/config/nodejs-dev.jsonc
- Added
"ai-platform/snippets"
to the list of tests skipped in the dev CI run (line 80).
- Added
- .github/config/nodejs.jsonc
- Removed
"ai-platform/snippets"
from the list of failing tests that need fixing (line 78).
- Removed
- ai-platform/snippets/ci-setup.json
- Added a new file to configure the CI environment for
ai-platform/snippets
, specifying Node.js version 16, timeout, and secrets.
- Added a new file to configure the CI environment for
- ai-platform/snippets/package.json
- Updated dependencies:
@google-cloud/aiplatform
from ^3.0.0 to ^4.0.0 (line 16),@google-cloud/bigquery
from ^7.0.0 to ^8.0.0 (line 17),chai
from ^4.5.0 to ^5.0.0 (line 22),mocha
from ^10.0.0 to ^11.0.0 (line 23),uuid
from ^10.0.0 to ^11.0.0 (line 24),sinon
from ^18.0.0 to ^20.0.0 (line 25). - Added
--reporter list
to the test script (line 13).
- Updated dependencies:
- ai-platform/snippets/test/batch-prediction-gemini.test.js
- Changed
require
statements toimport
statements forchai
,mocha
,child_process
, and@google-cloud/aiplatform
(lines 19-22). - Aliased
execSync
import fromchild_process
as_execSync
and reassigned toexecSync
constant (line 21, 24).
- Changed
- ai-platform/snippets/test/create-batch-prediction-job-text-classification.test.js
- Changed
require
statements toimport
statements forchai
,mocha
,uuid
,child_process
, and@google-cloud/aiplatform
(lines 19-22, 25). - Aliased
v4
import fromuuid
asuuid
(line 21). - Aliased
execSync
import fromchild_process
as_execSync
and reassigned toexecSync
constant (line 22, 23). - Aliased
v1
import from@google-cloud/aiplatform
asv1
(line 25) and updated theJobServiceClient
instantiation (line 30).
- Changed
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.
Node version changed,
From new to older, arranged,
CI runs green now.
Footnotes
-
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. ↩
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 aims to migrate the ai-platform/snippets
to Node.js 16, primarily to resolve CJS (CommonJS) errors previously encountered. The changes involve CI configuration updates, significant dependency upgrades, and modernization of test files to use ES Module (ESM) syntax.
Overall, the changes appear to be well-executed and align with the stated goal. The move to Node 16 for this specific package, along with dependency updates and ESM adoption in tests, seems like a pragmatic approach to address the CI issues.
Summary of Findings
- Node.js Version Downgrade/Pinning: The CI configuration for
ai-platform/snippets
is now explicitly set to use Node.js 16 via the newai-platform/snippets/ci-setup.json
file. This is the core change to address the CJS errors. - Dependency Upgrades: Several key dependencies (
@google-cloud/aiplatform
,@google-cloud/bigquery
) and devDependencies (chai
,mocha
,uuid
,sinon
) have been upgraded to newer major versions inai-platform/snippets/package.json
. These updates likely contribute to better compatibility with Node 16 and ESM. - Test Syntax Modernization: Test files (
batch-prediction-gemini.test.js
,create-batch-prediction-job-text-classification.test.js
) have been refactored from CommonJS (require
) to ES Module (import
) syntax. This aligns with modern JavaScript practices and is often necessary with newer library versions and Node.js ESM support. - CI Test Configuration Update: The
ai-platform/snippets
tests are now skipped in thedev
CI environment (as per.github/config/nodejs-dev.jsonc
) because they are reportedly passing in production, and removed from the "TODO: fix these" list in.github/config/nodejs.jsonc
, indicating the underlying issues are resolved. - Mocha Reporter Update: The test script in
package.json
now uses--reporter list
for Mocha, which should improve the readability of test outputs in CI.
Merge Readiness
The changes in this pull request seem to correctly implement the goal of migrating ai-platform/snippets
to Node.js 16 and addressing related CJS errors. The dependency upgrades and ESM syntax adoption in tests are logical steps in this process. Based on the code review, no critical or high-severity issues were identified in the implementation itself. The PR appears to be in good shape for merging, assuming CI checks pass and the 'experiment' is deemed successful. As a reviewer, I cannot approve the PR, but it seems technically sound.
Looks like the "node-version" feature is not yet implemented in custard-run.yaml, and it doesn't seem to be working properly on the legacy custard-ci.yaml either. |
Based from #4100
DO NOT MERGE: This is only an experiment, but changing Node versions is not yet fully supported.
Experiment to downgrade to Node 16 to avoid CJS errors.