BREAKING CHANGE: major refactor done, bump genkit version to 0.9 #81
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/github_embedders.ts (1)
Line range hint
112-122: LGTM! Core implementation updated for Genkit v0.9+.The change to
ai.defineEmbedderand input mapping updates are correct.Consider adding these improvements:
- Add error handling for invalid embeddings response
- Add validation for embedding dimensions
- Enhance type safety with a more specific return type
return ai.defineEmbedder( { info: model.info!, configSchema: TextEmbeddingConfigSchema, name: model.name, }, - async (input, options) => { + async (input: { text: string }[], options?: TextEmbeddingGeckoConfig) => { const body = { body: { model: name, input: input.map((d) => d.text), dimensions: options?.dimensions, encoding_format: options?.encodingFormat, }, } as GetEmbeddingsParameters; const embeddings = (await client .path("/embeddings") .post(body)) as GetEmbeddings200Response; + // Validate response + if (!embeddings?.body?.data?.length) { + throw new Error('Invalid embeddings response'); + } + // Validate dimensions if specified + if (options?.dimensions) { + const actualDimensions = embeddings.body.data[0]?.embedding?.length; + if (actualDimensions !== options.dimensions) { + throw new Error(`Expected ${options.dimensions} dimensions but got ${actualDimensions}`); + } + } return { embeddings: embeddings.body.data.map((d) => ({ embedding: Array.isArray(d.embedding) ? d.embedding : [], })), }; }, );src/index.ts (1)
106-108: ReplaceObject.keys().forEach()withfor...ofloop for better readability.Using a
for...ofloop can improve readability and maintain consistency with modern JavaScript practices.Refactor the code as follows:
-Object.keys(SUPPORTED_GITHUB_MODELS).forEach((name) => { - githubModel(name, client, ai); -}); +for (const name of Object.keys(SUPPORTED_GITHUB_MODELS)) { + githubModel(name, client, ai); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/github_embedders.ts(4 hunks)src/github_llms.ts(4 hunks)src/index.ts(3 hunks)
🔇 Additional comments (10)
package.json (2)
28-28: LGTM: Azure SSE dependency version specification.
The explicit version specification for @azure/core-sse is appropriate.
31-31: Verify stability implications of using genkit dev version.
While the migration from @genkit-ai/{core,ai}@0.5.0 to genkit@0.9.0-dev.4 aligns with the PR objectives, using a development version (-dev.4) in production could pose stability risks:
- Development versions may contain breaking changes
- APIs might not be finalized
- Future updates could be unpredictable
Please confirm:
- Is this dev version required for specific features?
- Are there plans to upgrade to a stable release?
- Has this version been thoroughly tested?
Let's check the dependency usage:
Consider:
- Adding a comment in package.json explaining why this specific dev version is required
- Creating an upgrade plan to move to the stable version when available
- Documenting any known limitations or issues with this version
✅ Verification successful
Using genkit dev version is appropriate in this context
The dev version usage is justified for the following reasons:
- This is a plugin package (
genkitx-github) that properly declaresgenkitas a peer dependency, meaning the actual version used will be determined by the end application - The git history shows a recent intentional upgrade to this version (
refactor: major refactor done, bump genkit version to 0.9) - The package itself is a community plugin for Firebase Genkit, and the version aligns with the current development state
- The dependency is properly scoped as a peer dependency, which is the correct pattern for plugin packages to avoid version conflicts
The stability concerns are mitigated by:
- Clear version pinning (
0.9.0-dev.4) - Proper peer dependency declaration allowing end users to override if needed
- The plugin nature of the package, where version compatibility is an expected consideration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the genkit dependency usage and potential breaking changes
# Test 1: Check for genkit imports and usage patterns
echo "Checking genkit import patterns..."
rg -l 'from\s+[\'"]genkit[\'"]|require\([\'"]genkit[\'"]\)'
# Test 2: Look for any TODO/FIXME comments related to genkit version
echo "Checking for version-related comments..."
rg -i "todo|fixme|hack|xxx" -A 3 -B 3 | rg -i "genkit|0\.9|version"
Length of output: 172
Script:
#!/bin/bash
# Let's try a more comprehensive analysis
# Check for genkit imports with proper quoting and extended patterns
echo "=== Checking genkit import patterns ==="
rg -l "from genkit|require.*genkit|import.*genkit"
# Check package.json for any comments or related dependencies
echo -e "\n=== Analyzing package.json ==="
cat package.json
# Look for migration or upgrade related comments
echo -e "\n=== Checking for migration/upgrade comments ==="
rg -i "migration|upgrade|genkit" -A 3 -B 3
# Check git history for genkit related changes
echo -e "\n=== Recent genkit-related commits ==="
git log --grep="genkit" --pretty=format:"%h %s" -n 5
Length of output: 19070
src/github_embedders.ts (2)
18-18: LGTM! Import changes align with Genkit v0.9+ upgrade.
The addition of Genkit import while maintaining embedderRef properly supports the refactoring objectives.
93-97: LGTM! Function signature properly updated for Genkit v0.9+.
The addition of the ai: Genkit parameter follows good practices with proper type safety and maintains backward compatibility.
src/index.ts (2)
1-2: Imports updated appropriately.
The import statements for Genkit and genkitPlugin have been correctly updated to reflect the new module paths.
110-112:
Fix syntax error due to unnecessary comma after function call.
There is an extra comma after the githubEmbedder function call, which may cause a syntax error.
Apply this diff to correct the issue:
-Object.keys(SUPPORTED_EMBEDDING_MODELS).forEach((name) =>
- githubEmbedder(name, ai, options),
-);
+Object.keys(SUPPORTED_EMBEDDING_MODELS).forEach((name) => {
+ githubEmbedder(name, ai, options);
+});Additionally, consider using a for...of loop for consistency:
+for (const name of Object.keys(SUPPORTED_EMBEDDING_MODELS)) {
+ githubEmbedder(name, ai, options);
+}Likely invalid or redundant comment.
src/github_llms.ts (4)
19-27: Imports updated to include Genkit
The import statements have been correctly updated to import Genkit from the "genkit" package. This change ensures compatibility with the new Genkit library and aligns with the updates made in other parts of the code.
794-796: Function signature updated to accommodate Genkit instance
The githubModel function signature has been updated to include the ai: Genkit parameter. This change allows the function to utilize the Genkit instance for defining models, which is necessary for integration with the new Genkit API.
Line range hint 800-824: Model definition using ai.defineModel
The use of ai.defineModel to define the model ensures proper registration and integration within the Genkit framework. The modifications correctly pass the necessary model information and configuration to the defineModel method.
600-600: Verify access to msg.text property
The code now accesses msg.text as a property instead of calling it as a method with msg.text(). Please verify that msg.text is a property of the Message class and that this change does not affect the expected functionality.
To confirm that msg.text is a property, run the following script:
This script searches for the Message class definition in the TypeScript files and checks if there is a getter method for text, indicating that msg.text can be used as a property.
Also applies to: 608-608, 637-637
✅ Verification successful
Let's search for the Message class definition and its properties to understand how the text property is defined. I'm expanding the context lines to see more of the class definition and also including interface/type definitions that might be relevant.
Let's try to understand how msg.text is being used in the codebase and where the Message type/class is being imported from, which might help us trace its definition.
From the previous results, we can see that msg.text is consistently used as a property in multiple places. Let's try to find the actual Message type definition to confirm how the text property is declared.
Property access to msg.text appears to be the correct usage pattern
Based on the search results, we can see that msg.text is consistently used as a property access throughout the codebase in multiple locations (lines 600, 608, and 637 in src/github_llms.ts). This consistent usage pattern across different code paths suggests this is the intended way to access the text content. While we couldn't locate the exact Message type definition, the uniform property access pattern in the codebase indicates this change from method call to property access is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `Message` class has a `text` property.
# Search for the `Message` class definition and check for a `text` getter.
fd --exclude node_modules --extension ts | xargs rg -A 5 'class Message' | rg 'get text\(\)'
Length of output: 94
Script:
#!/bin/bash
# Let's try a broader search to find the Message class definition and its properties
fd --exclude node_modules --extension ts | xargs rg -A 20 'class Message'
# Also search for any type definitions or interfaces related to Message
fd --exclude node_modules --extension ts | xargs rg -A 10 'interface Message|type Message'
Length of output: 168
Script:
#!/bin/bash
# Let's search for the actual usage of msg.text in the codebase to understand its context
rg -B 5 -A 5 'msg\.text'
# Also search for Message-related imports to find where it might be defined
rg 'import.*Message'
Length of output: 1297
Script:
#!/bin/bash
# Let's look at the Message type/interface definition by searching for its declaration
ast-grep --pattern 'interface Message {
$$$
}'
# Also search for type Message = to catch type aliases
rg 'type Message ='
# Let's also look at the context where msg is defined
rg -B 10 'msg: Message'
Length of output: 98
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
36-38: Enhance version compatibility documentation.While the version compatibility information is clear, consider these improvements for better user experience:
- Use a table format for better readability
- Add migration steps for users upgrading from v1.9.0 to v2.0.0
- Document any breaking changes between versions
Consider this format:
-### Versions - -if you are using Genkit version `<v0.9.0` or lower, please use the plugin version `v1.9.0`. If you are using Genkit `>=v0.9.0`, please use the plugin version `v2.0.0`. +### Version Compatibility + +| Genkit Version | Plugin Version | Notes | +|----------------|----------------|-------| +| < v0.9.0 | v1.9.0 | Legacy version | +| >= v0.9.0 | v2.0.0 | Current version | + +#### Migration Guide + +When upgrading from v1.9.0 to v2.0.0: +1. Update your Genkit dependency to v0.9.0 or higher +2. [Add migration steps here] + +#### Breaking Changes + +- [List breaking changes here]
Documentation needs updates to reflect current implementation
The documentation requires several updates to match the actual implementation:
- The import statement in examples is incorrect:
import {github, openAIGpt4o} from "genkitx-github"; // incorrectShould be:
import {github, openAIGpt4o} from "@xavidop/genkitx-github"; // correct
The troubleshooting section needs updates:
- The note about GPT
o1-previewando1-miniis outdated as the implementation shows they support multiturn conversations- The Cohere models issue link is outdated as the implementation shows response format handling for non-Cohere models
The supported models section needs expansion to list all the available models found in the implementation:
- OpenAI models (GPT-4o, GPT-4o-mini, etc.)
- Meta Llama models (3.x series)
- Cohere models (Command-r series)
- Mistral models
- AI21 models
- Microsoft Phi models
🔗 Analysis chain
Line range hint
42-185: Verify documentation accuracy post-refactor.Since this PR involves a major refactor for Genkit v0.9+ compatibility, please verify if the following sections need updates:
- Code examples (they might need version-specific variants)
- Configuration section (might need version-specific instructions)
- Troubleshooting section (might need new known issues)
Let's verify if the imports and API usage in the documentation match the actual implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the documented imports and API patterns match the implementation # Check if the documented imports exist in the codebase echo "Checking documented imports..." rg "export.*\b(github|openAIGpt4o)\b" src/ # Check if the documented API patterns exist echo "Checking documented API patterns..." rg "configureGenkit|defineFlow|defineTool" src/Length of output: 402
Script:
#!/bin/bash # Let's verify the actual API patterns and types in the codebase # Check the actual plugin configuration pattern echo "Checking plugin configuration pattern..." ast-grep --pattern 'export function github($$$) { $$$' -f ts # Check model usage patterns echo "Checking model usage patterns..." rg -A 5 "modelRef|generate" src/ # Check if there are any type definitions that might affect the documentation echo "Checking type definitions..." fd -e ts -e d.ts --exec cat {} \;Length of output: 38320
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/github_llms.ts (1)
Line range hint
814-824: Add error handling for JSON parsing in streamingThe streaming implementation should handle potential JSON parsing errors to prevent crashes.
for await (const event of sseStream) { if (event.data === "[DONE]") { break; } - for (const choice of JSON.parse(event.data).choices) { - const c = fromGithubChunkChoice(choice); - streamingCallback({ - index: 0, - content: [{ ...c, custom: c.custom as Record<string, any> }], - }); + try { + const data = JSON.parse(event.data); + for (const choice of data.choices) { + const c = fromGithubChunkChoice(choice); + streamingCallback({ + index: 0, + content: [{ ...c, custom: c.custom as Record<string, any> }], + }); + } + } catch (error) { + console.error('Error parsing streaming response:', error); + throw new Error('Failed to parse streaming response'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/github_llms.ts(9 hunks)
🔇 Additional comments (4)
src/github_llms.ts (4)
19-34: LGTM: Import changes align with Genkit upgrade
The reorganization of imports from 'genkit' and 'genkit/model' properly aligns with the migration to Genkit version 0.9+.
665-665: LGTM: Type definitions properly aligned with Genkit API
The updated type definitions using ModelResponseData improve type safety and correctly align with the new Genkit API.
Also applies to: 692-692, 714-724
792-793: LGTM: Function signature updated for Genkit compatibility
The addition of the ai: Genkit parameter aligns with the new Genkit API requirements.
600-600: Verify consistent usage of Message.text property
The change from msg.text() to msg.text reflects an API change in the Message class. Let's verify that this change has been consistently applied across the codebase.
Also applies to: 608-608, 637-637
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/depsreview.yaml (1)
14-14: Add newline at end of file.YAML files should end with a newline character to comply with POSIX standards.
- allow-licenses: BSD-2-Clause, BSD-3-Clause, MIT, Apache-2.0, MPL-2.0, ISC, LGPL-3.0, 0BSD, BSD-3-Clause-Clear + allow-licenses: BSD-2-Clause, BSD-3-Clause, MIT, Apache-2.0, MPL-2.0, ISC, LGPL-3.0, 0BSD, BSD-3-Clause-Clear +🧰 Tools
🪛 yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/depsreview.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/depsreview.yaml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/depsreview.yaml (1)
14-14: LGTM: License addition is appropriate.
The addition of BSD-3-Clause-Clear to the allowed licenses list is consistent with common open-source licensing practices and aligns with the dependency changes in this PR.
🧰 Tools
🪛 yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/depsreview.yaml (2)
14-14: Improve readability of license list.The license list should have consistent spacing after commas for better readability.
- allow-licenses: BSD-2-Clause, MIT, Apache-2.0, MPL-2.0, ISC, LGPL-3.0, 0BSD, BSD-3-Clause, BSD-3-Clause AND BSD-3-Clause-Clear + allow-licenses: BSD-2-Clause, MIT, Apache-2.0, MPL-2.0, ISC, LGPL-3.0, 0BSD, BSD-3-Clause, BSD-3-Clause, BSD-3-Clause-Clear🧰 Tools
🪛 yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
14-14: Add newline at end of file.Add a newline character at the end of the file to comply with YAML formatting standards.
allow-licenses: BSD-2-Clause, MIT, Apache-2.0, MPL-2.0, ISC, LGPL-3.0, 0BSD, BSD-3-Clause, BSD-3-Clause AND BSD-3-Clause-Clear +🧰 Tools
🪛 yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
src/github_llms.ts (2)
Line range hint
714-724: Consider improving type safety infromGithubChunkChoice.The function uses
anytype for its parameter, which could lead to runtime errors. Consider defining a proper interface for the chunk choice parameter.-function fromGithubChunkChoice(choice: any): ModelResponseData { +interface GithubChunkChoice { + content?: string; + finishReason?: string; + delta?: { + content?: string; + }; +} +function fromGithubChunkChoice(choice: GithubChunkChoice): ModelResponseData {
Line range hint
792-848: Add error handling for streaming response.While the changes to use
ai.defineModelare correct, the streaming response handling could be improved with proper error handling.if (streamingCallback) { body.body.stream = true; response = await client.path("/chat/completions").post(body); const stream = response.body; const sseStream = createSseStream(stream as any); + try { for await (const event of sseStream) { if (event.data === "[DONE]") { break; } - for (const choice of JSON.parse(event.data).choices) { + const parsedData = JSON.parse(event.data); + if (!parsedData.choices) { + throw new Error('Invalid response format: missing choices'); + } + for (const choice of parsedData.choices) { const c = fromGithubChunkChoice(choice); streamingCallback({ content: [{ ...c, custom: c.custom as Record<string, any> }], }); } } + } catch (error) { + throw new Error(`Error processing streaming response: ${error.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/depsreview.yaml(1 hunks)package.json(1 hunks)src/github_llms.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 yamllint
.github/workflows/depsreview.yaml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
.github/workflows/depsreview.yaml (1)
12-12: LGTM! Good practice to pin to specific version.
Pinning to v4.3.3 instead of v4 ensures workflow stability and predictability.
src/github_llms.ts (2)
19-34: LGTM: Import changes align with Genkit upgrade.
The reorganization of imports to use the new genkit library is correct and consistent with the PR's objective of upgrading to Genkit version 0.9.
600-600: Verify the message text access pattern changes.
The changes from msg.text() to msg.text reflect an API change in the new Genkit version. Let's verify that this pattern is consistently applied across the codebase.
Also applies to: 608-608, 637-637
✅ Verification successful
The previous search might have been too specific. Let's broaden the search to find all message text access patterns and verify the consistency of the changes.
Message text access pattern is consistent across the codebase
The verification shows that all occurrences of msg.text in the codebase are using the property access pattern (not the method call pattern msg.text()). The changes are consistently applied in all three locations in src/github_llms.ts (lines 600, 608, and 637).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining occurrences of the old pattern
rg 'msg\.text\(\)'
Length of output: 20
Script:
#!/bin/bash
# Search for any occurrences of msg.text (both as property and method)
rg -A 2 'msg\.text'
# Also search for potential message text access patterns
ast-grep --pattern 'msg.text'
Length of output: 554
Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 8.13.0 to 8.14.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.14.0/packages/typescript-eslint) --- updated-dependencies: - dependency-name: typescript-eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Xavier Portilla Edo <xavi_tb@hotmail.com>
xavidop
changed the title
Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 8.13.0 to 8.14.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.14.0/packages/typescript-eslint) --- updated-dependencies: - dependency-name: typescript-eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Xavier Portilla Edo <xavi_tb@hotmail.com>
xavidop
changed the title
|
|
🎉 This PR is included in version 1.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This pull request is related to:
I have checked the following:
Description:
Preparing the plugin for Genkit version > 0.9. Change the plugin API to be ready for the new GenkitAPI
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Genkitlibrary, improving clarity and efficiency in handling models and messages.