Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactor of the tool interrupt and resume mechanism within the Genkit Python SDK. Key changes include replacing the generic tool_responses parameter with more explicit resume_respond, resume_restart, and resume_metadata options in the generate and prompt execution APIs. A new Interrupt exception has been added for tools to signal pauses, complemented by helper functions like respond_to_interrupt and restart_interrupted_tool for resuming execution. Additionally, the ExecutablePrompt API was updated to accept configuration options as keyword arguments rather than a single opts dictionary. The Google AI plugin was also updated to handle scalar tool inputs for Gemini and to correctly report the finish_reason in streaming responses. I have no feedback to provide as there are no review comments to assess.
py/plugins/google-genai/src/genkit/plugins/google_genai/models/gemini.py
Outdated
Show resolved
Hide resolved
|
|
||
| async def resolve_tool(registry: Registry, tool_name: str) -> Action: | ||
| """Resolve a tool by name from the registry.""" | ||
| """Resolve a :class:`~genkit.ActionKind.TOOL` action by name from the registry.""" |
There was a problem hiding this comment.
fix comment weirdness
| if tool is None: | ||
| raise ValueError(f'Unable to resolve tool {tool_name}') | ||
| return tool | ||
| if tool is not None: |
| interrupt=interrupt, | ||
| metadata={'source': 'cli', 'path': 'respond'}, | ||
| ) | ||
| assert isinstance(interrupt_response, ToolResponsePart) |
There was a problem hiding this comment.
shouldn't need to assert here
| }, | ||
| ] | ||
|
|
||
| pm.responses.append( |
There was a problem hiding this comment.
does it make sense to have this be in the pm response? I think this is constructed via framework calls
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_tool_either_interrupts_or_returns() -> None: |
There was a problem hiding this comment.
this test is unclear
| 'toolResponse': {'ref': 'ra', 'name': 'a', 'output': {'done': True}}, | ||
| 'metadata': {'interruptResponse': True}, | ||
| }, | ||
| {'toolResponse': {'ref': 'rb', 'name': 'b', 'output': 'b-done'}}, |
There was a problem hiding this comment.
confirm no metadata is correct on restart case
| resume=Resume(), | ||
| ), | ||
| ) | ||
| assert 'replies' in ei.value.original_message or 'restarts' in ei.value.original_message |
There was a problem hiding this comment.
better assert, can't tell this is is verifying the GenkitError
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_pending_output_trp_yields_tool_response_with_source_pending() -> None: |
There was a problem hiding this comment.
this scenario doesn't seem realistic because there is no interrupt in this case?
| resume=Resume(), | ||
| ), | ||
| ) | ||
| assert 'model' in ei.value.original_message.lower() |
There was a problem hiding this comment.
better assert, can't tell this is is verifying the GenkitError
| out = pay.restart(None, interrupt=interrupt_trp, resumed_metadata={'k': 'v'}) | ||
| assert isinstance(out, ToolRequestPart) | ||
| assert out.metadata is not None | ||
| assert out.metadata.get('resumed') == {'k': 'v'} |
There was a problem hiding this comment.
should it include: resolvedInterrupt: {'reason': 'hold'}
| with pytest.raises(GenkitError) as ei: | ||
| await run_tool_after_restart(action, restart_trp) | ||
| msg = ei.value.original_message.lower() | ||
| assert 'restart' in msg or 'nested' in msg or 'not supported' in msg |
There was a problem hiding this comment.
more clear assert
| async def test_run_tool_after_restart_response_preserves_ref() -> None: | ||
| """run_tool_after_restart produces a ToolResponsePart whose ref matches the restart TRP's ref.""" |
There was a problem hiding this comment.
Set up a more realistic scenario
confirm that run_tool_after_restart uses the new input
| system: str | list[Part] | None = None, | ||
| messages: list[Message] | None = None, | ||
| tools: list[str] | None = None, | ||
| tools: Sequence[str | Tool] | None = None, |
There was a problem hiding this comment.
document reasoning for Sequence vs. list.
| """Stream generated text, returning a ModelStreamResponse with .stream and .response.""" | ||
| """Stream generated text, returning a ModelStreamResponse with .stream and .response. | ||
|
|
||
| Middleware (``use=``) uses the same signatures as :meth:`generate`: |
There was a problem hiding this comment.
Unnecessary comment (line 957-960)
| def tools_to_action_names( | ||
| tools: Sequence[str | Tool] | None, | ||
| ) -> list[str] | None: | ||
| """Normalize tool arguments to registry names for :class:`GenerateActionOptions`. |
There was a problem hiding this comment.
Remove :class: and :meth: from comment.
| async def _resolve_tool_request(tool: Action, tool_request_part: ToolRequestPart) -> tuple[Part | None, Part | None]: | ||
| """Execute a tool and return (response_part, interrupt_part).""" | ||
| def _interrupt_from_tool_exc(exc: BaseException) -> Interrupt | None: | ||
| """If ``exc`` is (or wraps) :class:`~genkit._ai._tools.Interrupt`, return that interrupt.""" |
| """Resolve a single tool request from pending output or resume.respond list.""" | ||
| async def _resolve_resumed_tool_request( | ||
| registry: Registry, raw_request: GenerateActionOptions, tool_request_part: Part | ||
| ) -> tuple[Part, Part]: |
There was a problem hiding this comment.
_resolve_resumed_tool_request should return tuple[ToolRequestPart, ToolResponsePart]. not [Part, Part]
|
|
||
| P = ParamSpec('P') | ||
| T = TypeVar('T') | ||
| # ------------------------------------------------------------------ |
There was a problem hiding this comment.
Remove these # ---- Sections
Adds restart support in the Python SDK and other improvements.
Tools (or wrapped tools) can raise
Interrupt, which stops the current generation turn and surfaces the pendingToolRequestPartwith interrupt metadata.Now, The execution flow is paused, giving the caller the option to either respond, and restart the tool execution with replaced input and resumed metadata.
See samples for update usage.
Core Changes
interrupthelper off of ToolRunContext. Now youraise Interruptto trigger interruptions.define_interruptprimitive, which provides the default Interrupt tool, with the option to provide a input schema to constrain the model call.tool_responsehelper torespond_to_interrupt, and addedrestart_interrupted_toolto support the restart use case.tool_fn_wrapper.Related Changes
optsargument; thosePromptGenerationOptionsare flat kwargs now.