Feat/adding prompt sample test #3963
Conversation
…st based on code review
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 significantly refines the prompt management system within the Genkit Python framework. It introduces robust capabilities for defining and loading prompts from external files, supporting advanced features like prompt variants and reusable partials. The core Highlights
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
|
|
This branch includes some previous changes that have been merged already. Will create a new branch and new PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for defining, loading, and looking up prompts, including support for file-based prompts, variants, and partials. It also adds comprehensive tests for these new features. My review focuses on a critical bug in the lazy-loading implementation for prompts which could lead to memory leaks and incorrect behavior, as well as some documentation inconsistencies. The changes are otherwise well-structured and the addition of tests is a great practice.
| if action: | ||
| # First check if we've stored the ExecutablePrompt directly | ||
| if hasattr(action, '_executable_prompt') and action._executable_prompt is not None: | ||
| return action._executable_prompt | ||
| elif hasattr(action, '_async_factory'): | ||
| # Otherwise, create it from the factory (lazy loading) | ||
| # This will also set _executable_prompt on the action for future lookups | ||
| executable_prompt = await action._async_factory() | ||
| # Store it on the action for future lookups (if not already stored) | ||
| if not hasattr(action, '_executable_prompt') or action._executable_prompt is None: | ||
| action._executable_prompt = executable_prompt | ||
| return executable_prompt | ||
| else: | ||
| # Fallback: try to get from metadata | ||
| factory = action.metadata.get('_async_factory') | ||
| if factory: | ||
| executable_prompt = await factory() | ||
| # Store it on the action for future lookups | ||
| if not hasattr(action, '_executable_prompt') or action._executable_prompt is None: | ||
| action._executable_prompt = executable_prompt | ||
| return executable_prompt | ||
| # Last resort: this shouldn't happen if prompts are loaded correctly | ||
| raise GenkitError( | ||
| status='INTERNAL', | ||
| message=f'Prompt action found but no ExecutablePrompt available for {name}', | ||
| ) | ||
|
|
There was a problem hiding this comment.
The implementation of lookup_prompt has a critical bug related to weakref that can cause memory leaks and return incorrect objects.
return action._executable_promptreturns theweakref.refobject itself, not theExecutablePromptinstance it refers to. You should call the weakref object to get the underlying prompt:action._executable_prompt().- The code that re-assigns
action._executable_prompt = executable_promptafter creating it from the factory creates a strong reference, which defeats the purpose of usingweakrefand will lead to a memory leak due to a circular reference. The factory functioncreate_prompt_from_filealready correctly sets the weakref.
This suggested change corrects the weakref handling and simplifies the logic to prevent these issues.
if action:
# First check if we have a live weak reference to the ExecutablePrompt
if hasattr(action, '_executable_prompt') and action._executable_prompt:
prompt = action._executable_prompt()
if prompt:
return prompt
# If not, create it from the factory (lazy loading).
# The factory is responsible for setting the weakref on the action.
if hasattr(action, '_async_factory'):
return await action._async_factory()
# Fallback for safety, though _async_factory should exist.
factory = action.metadata.get('_async_factory')
if factory:
return await factory()
# Last resort: this shouldn't happen if prompts are loaded correctly
raise GenkitError(
status='INTERNAL',
message=f'Prompt action found but no ExecutablePrompt available for {name}',
)| registry: The registry to look up the prompt from. | ||
| name: The name of the prompt. | ||
| variant: Optional variant name. | ||
| dir: Optional directory parameter (accepted for compatibility but not used). |
There was a problem hiding this comment.
The docstring for the prompt method is inaccurate. It lists registry and dir as parameters, but they are not part of the method's signature. The method uses self.registry instead. This can be misleading for developers using this method. Please update the docstring to match the method signature.
| registry: The registry to look up the prompt from. | |
| name: The name of the prompt. | |
| variant: Optional variant name. | |
| dir: Optional directory parameter (accepted for compatibility but not used). | |
| name: The name of the prompt. | |
| variant: Optional variant name. |
| if self._name is None: | ||
| raise GenkitError( | ||
| status='FAILED_PRECONDITION', | ||
| message='Prompt name not available. This prompt was not created via define_prompt_async() or load_prompt().', |
There was a problem hiding this comment.
The error message here mentions define_prompt_async(), but there is no such function defined in the codebase. It seems this should refer to define_prompt() instead.
| message='Prompt name not available. This prompt was not created via define_prompt_async() or load_prompt().', | |
| message='Prompt name not available. This prompt was not created via define_prompt() or load_prompt().', |
| Exception: If a tool name provided in options cannot be found in | ||
| the registry. | ||
| GenkitError: If the options do not contain any messages. |
There was a problem hiding this comment.
The docstring for to_generate_request indicates that it raises an Exception if a tool is not found. However, the implementation raises a GenkitError. It's best to keep the docstring consistent with the implementation and consolidate the Raises section.
| Exception: If a tool name provided in options cannot be found in | |
| the registry. | |
| GenkitError: If the options do not contain any messages. | |
| GenkitError: If a tool name provided in options cannot be found in | |
| the registry, or if the options do not contain any messages. |
2 participants
Description here... Help the reviewer by:
Checklist (if applicable):