-
Notifications
You must be signed in to change notification settings - Fork 633
feat(py):mcp-sample #4072
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?
feat(py):mcp-sample #4072
Conversation
Summary of ChangesHello @MengqinShen, 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 introduces foundational support for the Model Context Protocol (MCP) within the Python Genkit framework, significantly enhancing its ability to interact with external, dynamically provided tools and resources. The changes span core API modifications to accommodate resource grounding and dynamic action resolution, alongside a robust sample application that showcases practical integrations with various MCP clients and servers. This feature aims to make Genkit more extensible and adaptable to diverse operational environments by allowing seamless discovery and utilization of external capabilities. 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. 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.
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 introduces support for resources in the Python Genkit library, which can be used for grounding in generation requests. This is a significant feature that touches many parts of the core library, including generate, prompt, action, and registry. The changes also include making the action registry lookups asynchronous to support dynamic action providers, which is a major architectural change. A new sample application for MCP (mcp-sample) is also added to demonstrate these new capabilities.
My review found a critical issue in the compat-oai plugin where a method call was changed to a non-existent method, which will cause a runtime error. There are also some medium-severity issues related to code style (local imports) and redundancy in the MCP server setup.
Overall, this is a substantial and important feature addition. The core logic seems well-implemented, but the side-effects on other plugins and some code style issues need to be addressed.
| result = [] | ||
| for tool_definition in tools: | ||
| action = self._registry.registry.lookup_action(ActionKind.TOOL, tool_definition.name) | ||
| action = self._registry.registry.get_action(ActionKind.TOOL, tool_definition.name) |
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.
This change from lookup_action to get_action is incorrect, as the Registry class does not have a get_action method. This will cause an AttributeError.
The correct method is lookup_action, which has been made async in this PR. This means that this function _get_tools_definition and its callers (_get_openai_request_config and generate) must be converted to async to correctly await the lookup_action call.
Additionally, assuming self._registry is the GenkitRegistry instance, the call should be await self._registry.lookup_action(...), not self._registry.registry.get_action(...).
Note: The suggested code below will only work after you convert _get_tools_definition and its callers to async methods.
| action = self._registry.registry.get_action(ActionKind.TOOL, tool_definition.name) | |
| action = await self._registry.lookup_action(ActionKind.TOOL, tool_definition.name) |
|
|
||
| resources: list[Action] = [] | ||
| if request.resources: | ||
| from genkit.blocks.resource import lookup_resource_by_name |
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.
| from genkit.blocks.resource import lookup_resource_by_name | ||
|
|
||
| for res_name in options.resources: | ||
| res_action = await lookup_resource_by_name(registry, res_name) | ||
| if res_action is None: | ||
| raise GenkitError(status='NOT_FOUND', message=f'Unable to resolve resource {res_name}') | ||
| resources.append(res_action) | ||
|
|
||
| from genkit.blocks.generate import to_tool_definition |
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.
There are a couple of local imports within this function (lookup_resource_by_name and to_tool_definition). It's generally better practice to have all imports at the top of the file for readability and to avoid potential issues, unless it's to resolve a circular dependency. Consider moving these imports to the top of the file.
| for kind in [ActionKind.TOOL, ActionKind.PROMPT, ActionKind.RESOURCE]: | ||
| actions = self.ai.registry.get_actions_by_kind(kind) | ||
| for action in actions.values(): | ||
| if kind == ActionKind.TOOL and action not in self.tool_actions: | ||
| self.tool_actions.append(action) | ||
| self.tool_actions_map[action.name] = action |
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.
This loop appears to be redundant and incomplete. The preceding loop (lines 139-154) already iterates over self.ai.registry._entries and populates self.tool_actions, self.prompt_actions, and self.resource_actions.
This new loop iterates over the same actions (since get_actions_by_kind reads from _entries) and re-adds tool actions, which is inefficient due to the action not in self.tool_actions check.
Furthermore, this loop iterates over PROMPT and RESOURCE kinds but has no logic to handle them, making it incomplete. The preceding loop seems to handle all kinds correctly. This block should probably be removed entirely, or updated to correctly handle all action kinds if it's intended to replace the first loop.
Description here... Help the reviewer by:
Checklist (if applicable):