Update google params transform #91
Conversation
…ion parameters in gemini requests
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: aa87da3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ff40a16 in 22 seconds
More details
- Looked at
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. public-packages/llm-client/src/providers/google/index.ts:60
- Draft comment:
The change from 'additionalProperties' to '_additionalProperties' is not explained. Ensure '_additionalProperties' is correctly set elsewhere. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_PHK80DNdMOHACdt1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 199439a in 1 minute and 38 seconds
More details
- Looked at
63lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. public-packages/llm-client/src/providers/google/index.ts:59
- Draft comment:
ThecleanSchemafunction does not handle nestedadditionalPropertieswithinpropertiesoritems. Consider recursively cleaning these nested structures as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code clearly shows recursive handling of nested structures through the cleanSchema function. When it encounters nested properties or items, it calls cleanSchema again which will remove additionalProperties at that level. The comment appears to be incorrect in its assessment.
Could there be some edge cases or specific nested structures that the current implementation doesn't handle? The recursive approach seems sound but maybe there's something subtle I'm missing.
No, the implementation is thorough - it handles both properties and items recursively, and the destructuring of additionalProperties happens at each level. The comment is simply incorrect.
Delete this comment as it incorrectly claims the code doesn't handle nested structures, when it clearly does through recursive calls to cleanSchema.
Workflow ID: wflow_yhL0R1ODsm0KWDwm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2cb5349 in 50 seconds
More details
- Looked at
1107lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/www/.source/index.js:6
- Draft comment:
The reordering of import statements is purely organizational and does not affect functionality. Ensure this aligns with your project's style guidelines. - Reason this comment was not posted:
Confidence changes required:10%
The import statements in theindex.jsfile have been reordered, but the change seems to be purely organizational. This doesn't affect functionality but could improve readability.
2. public-packages/llm-client/src/providers/google/index.ts:160
- Draft comment:
ThecreateGenerationConfigfunction correctly maps parameters fromGoogleChatCompletionParamstoGenerationConfig. This ensures proper configuration for chat sessions. - Reason this comment was not posted:
Confidence changes required:10%
ThecreateGenerationConfigfunction inindex.tsis correctly mapping parameters fromGoogleChatCompletionParamstoGenerationConfig. This ensures that the configuration is properly set for the chat session.
3. public-packages/llm-client/src/providers/google/index.ts:178
- Draft comment:
ThetransformHistoryfunction correctly filters out system messages and transforms the remaining messages into the required format for Google's API. - Reason this comment was not posted:
Confidence changes required:10%
ThetransformHistoryfunction inindex.tsis correctly filtering out system messages and transforming the remaining messages into the required format. This is important for maintaining the correct chat history format for Google's API.
4. public-packages/llm-client/src/providers/google/index.ts:223
- Draft comment:
ThegetChatSessionfunction effectively manages chat sessions, including caching and session reuse, which is crucial for performance and resource management. - Reason this comment was not posted:
Confidence changes required:10%
ThegetChatSessionfunction inindex.tsis responsible for managing chat sessions, including caching and session reuse. This is crucial for performance and resource management.
5. public-packages/llm-client/src/providers/google/index.ts:292
- Draft comment:
ThetransformResponsefunction effectively converts API responses into a structured format, including grounding metadata and tool calls, which is essential for detailed responses. - Reason this comment was not posted:
Confidence changes required:10%
ThetransformResponsefunction inindex.tsis responsible for converting the API response into a format that includes grounding metadata and tool calls. This is essential for providing detailed and structured responses.
Workflow ID: wflow_IAIYu20YQlFWbgSM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| * Gets system instructions from messages | ||
| */ | ||
| private getSystemInstructions(messages: GoogleChatCompletionParams["messages"]): string { | ||
| const systemMessages = messages.filter(message => message.role === "system") |
There was a problem hiding this comment.
could use reduce to only use one pass for really long conversations but perf is prob negligible in the vast majority of cases
There was a problem hiding this comment.
yeah fair - this was actually just form before splitting things up a bit, but google does support system messages so next push will bring that in and remove this entirely
| ): ExtendedCompletionGoogle | ExtendedCompletionChunkGoogle { | ||
| const responseText = responseDataChunk.text() | ||
| const toolCalls = | ||
| responseDataChunk.candidates?.[0]?.content?.parts?.flatMap(part => |
There was a problem hiding this comment.
worth null checking candidates?
| let contentWithGrounding = responseText | ||
| let sources: Array<{ url: string; title: string }> = [] | ||
|
|
||
| if (groundingMetadata) { |
| id: `chatcmpl-${Math.random().toString(36).slice(2)}`, | ||
| object: stream ? "chat.completion.chunk" : "chat.completion", | ||
| created: Date.now(), | ||
| model: "gemini-pro", |
There was a problem hiding this comment.
I believe 1.0 pro is deprecated, but we should be using the dynamically passed model regardless no?
There was a problem hiding this comment.
yeaha this was what was there - - we actualyl dont have the passed in params in this point so its hard coded, but yeah it should be the user provided param not this value
There was a problem hiding this comment.
😶🌫️ somehow that was missed on the original PR my b
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on dbaecbf in 45 seconds
More details
- Looked at
819lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. public-packages/llm-client/src/providers/google/index.ts:176
- Draft comment:
message.content.toString()can be simplified tomessage.contentsince it's already a string. - Reason this comment was not posted:
Confidence changes required:50%
ThetransformHistoryfunction is mappingmessage.contenttotextusingtoString(). This is unnecessary sincemessage.contentis already a string. This can be simplified by removingtoString().
Workflow ID: wflow_FZro9E5MUFDTMSdF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
… the resposne that google doesnt provide
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c58584a in 21 seconds
More details
- Looked at
65lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. public-packages/llm-client/src/providers/google/index.ts:278
- Draft comment:
Update the comment to reflect the change in parameters from a boolean toGoogleChatCompletionParams. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the parameter from a boolean to an object, which is a good change for extensibility. However, the comment on line 277 still refers to the old parameter type.
2. public-packages/llm-client/src/providers/google/index.ts:389
- Draft comment:
Update the comment to reflect the change in parameters from a boolean toGoogleChatCompletionParams. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the parameter from a boolean to an object, which is a good change for extensibility. However, the comment on line 389 still refers to the old parameter type.
Workflow ID: wflow_deabnF2znfAyoH02
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 708ec64 in 39 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. public-packages/llm-client/src/types/index.ts:9
- Draft comment:
GoogleGenerativeAIis imported but not used. Consider removing it to clean up the code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_OEH4SFW8yypM4Qy9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4378126 in 20 seconds
More details
- Looked at
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .changeset/afraid-rings-cough.md:5
- Draft comment:
The changeset description is too vague. Please provide a more detailed description of the changes made, including specific features or fixes implemented. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_DlcpVSx9ddJeb3ln
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on aa87da3 in 16 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. public-packages/llm-client/tests/google.test.ts:204
- Draft comment:
Removing theconsole.logstatement is a good practice for cleaner test output and avoiding unnecessary logging. - Reason this comment was not posted:
Confidence changes required:10%
The removal of the console.log statement is appropriate for production code, as it prevents unnecessary logging and potential exposure of sensitive information.
Workflow ID: wflow_jCOPJNUn6SEk1m6G
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Enhances Google Gemini API support in
llm-polyglotwith grounding, safety settings, session management, and updates documentation and tests.llm-polyglotwith grounding, safety settings, session management, and more.additionalPropertiesfrom function parameters in Gemini requests.google.mdxandREADME.mdto reflect new features and usage examples.index.tsinsrc/providers/googleto handle new features and clean schemas.index.jsto reorder imports for documentation files.google.test.tsfor new features like grounding and session management.This description was created by
for aa87da3. It will automatically update as commits are pushed.