fix(ai): Add a warning when setting top-level params in hybrid mode#9577
fix(ai): Add a warning when setting top-level params in hybrid mode#9577
Conversation
|
Vertex AI Mock Responses Check
|
Summary of ChangesHello @hsubox76, 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 enhances the developer experience for users of the AI SDK's hybrid inference mode by introducing a warning system. It notifies developers when they attempt to configure top-level parameters, such as Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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.
Code Review
The pull request introduces a helpful warning for developers using hybrid mode when they accidentally set top-level parameters that are ignored. The implementation is functional, but there are opportunities to improve the robustness of the tests and the efficiency of the parameter check. Specifically, the test stub for the logger should be restored to avoid side effects, and the parameter iteration should use Object.keys() to avoid potential issues with inherited properties.
packages/ai/src/api.ts
Outdated
| const hybridParamKeys: Array<keyof HybridParams> = [ | ||
| 'mode', | ||
| 'onDeviceParams', | ||
| 'inCloudParams' | ||
| ]; | ||
| for (const param in modelParams) { | ||
| if (!hybridParamKeys.includes(param as keyof HybridParams)) { | ||
| logger.warn( | ||
| `When a hybrid inference mode is specified (mode is currently set` + | ||
| ` to ${hybridParams.mode}), "${param}" cannot be ` + | ||
| `configured at the top level. Configuration for in-cloud and ` + | ||
| `on-device must be done separately in inCloudParams and onDeviceParams. ` + | ||
| `Configuration values set outside of inCloudParams and onDeviceParams will` + | ||
| ` be ignored.` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using for...in to iterate over object properties is generally discouraged as it includes inherited enumerable properties from the prototype chain. Additionally, the hybridParamKeys array is recreated on every function call. Consider using Object.keys() for safer iteration and defining the allowed keys more efficiently.
| const hybridParamKeys: Array<keyof HybridParams> = [ | |
| 'mode', | |
| 'onDeviceParams', | |
| 'inCloudParams' | |
| ]; | |
| for (const param in modelParams) { | |
| if (!hybridParamKeys.includes(param as keyof HybridParams)) { | |
| logger.warn( | |
| `When a hybrid inference mode is specified (mode is currently set` + | |
| ` to ${hybridParams.mode}), "${param}" cannot be ` + | |
| `configured at the top level. Configuration for in-cloud and ` + | |
| `on-device must be done separately in inCloudParams and onDeviceParams. ` + | |
| `Configuration values set outside of inCloudParams and onDeviceParams will` + | |
| ` be ignored.` | |
| ); | |
| } | |
| } | |
| const hybridParamKeys: Array<keyof HybridParams> = [ | |
| 'mode', | |
| 'onDeviceParams', | |
| 'inCloudParams' | |
| ]; | |
| for (const param of Object.keys(modelParams)) { | |
| if (!hybridParamKeys.includes(param as keyof HybridParams)) { | |
| logger.warn( | |
| `When a hybrid inference mode is specified (mode is currently set` + | |
| ` to ${hybridParams.mode}), "${param}" cannot be ` + | |
| `configured at the top level. Configuration for in-cloud and ` + | |
| `on-device must be done separately in inCloudParams and onDeviceParams. ` + | |
| `Configuration values set outside of inCloudParams and onDeviceParams will` + | |
| ` be ignored.` | |
| ); | |
| } | |
| } |
When using hybrid mode, top level params will be ignored (see comments in code)
Adding a warning so developers are aware and will not be confused why those settings are doing nothing.