Add max_tool_calls parameter for automatic tool execution#629
Add max_tool_calls parameter for automatic tool execution#629HamzaYslmn wants to merge 1 commit intoollama:mainfrom
Conversation
guicybercode
left a comment
There was a problem hiding this comment.
Overview
This diff adds function calling/tool support and several new parameters (think, logprobs, top_logprobs) to the chat() method in both sync and async variants. The changes appear to align the Python client with newer Ollama API capabilities.
Strengths
1. Consistent API Design
- Both sync (
def chat) and async (async def chat) methods receive identical parameter additions and documentation updates. - Method overloads are properly maintained for type-checking with
streamvariations.
2. Tool Handling Abstraction
tools=list(_copy_tools(tools)),- Delegating tool normalization to
_copy_tools()keeps the mainchat()method clean. - Supports multiple input types:
Mapping,Tool, orCallable→ good developer ergonomics.
3. Improved Documentation
- Added practical example showing how to define and pass a tool function.
- Docstring references Google style guide for docstring conventions 👍.
4. Type Safety
- Proper use of
Optional[Union[...]]andJsonSchemaValuefor theformatparameter. - Return types correctly distinguish between
ChatResponseand iterator variants based onstream.
Areas for Improvement
1. Missing Parameter Documentation
The new parameters think, logprobs, and top_logprobs are added to the signature and payload but not documented in the docstring:
# Add to docstring:
# think: Enable reasoning/thinking mode (if supported by model).
# logprobs: Include log probabilities in the response.
# top_logprobs: Number of top log probabilities to return (requires logprobs=True).2. Tool Error Handling
The _copy_tools() helper converts callables via convert_function_to_tool(), but there's no visible error handling for:
- Functions with unsupported signatures
- Missing type hints on parameters
- Non-JSON-serializable return types
Recommendation: Add validation with clear error messages:
try:
yield convert_function_to_tool(unprocessed_tool)
except (TypeError, ValueError) as e:
raise RequestError(f"Failed to convert tool '{unprocessed_tool.__name__}': {e}")3. Mutable Default Argument Risk (Minor)
While not directly visible here, ensure messages and tools aren't using mutable defaults elsewhere. The use of _copy_messages() and _copy_tools() mitigates this, but explicit copying in the public API is a good pattern to maintain.
4. Async/Sync Code Duplication
The sync and async chat() implementations are nearly identical aside from await. Consider:
- Using a shared private helper method to reduce duplication
- Or leveraging a decorator pattern for request handling
5. Example Code in Docstring
The example shows:
client.chat(model='llama3.2', tools=[add_two_numbers], messages=[...])But doesn't show the full tool-calling workflow (model response → tool execution → follow-up message). Consider linking to a full tutorial or expanding the example.
Potential Bugs / Edge Cases
| Issue | Severity | Recommendation |
|---|---|---|
tools=None passed to list(_copy_tools(None)) |
Low | Ensure _copy_tools handles None gracefully (returns empty list) |
top_logprobs without logprobs=True |
Medium | Add validation: if top_logprobs is set, logprobs must be True |
| Tool name collisions | Low | Document that tool.function.name must be unique per request |
Style & Maintainability
- ✅ Follows existing code style (type hints, docstring format)
- ✅ Uses
model_dump(exclude_none=True)to keep payloads clean ⚠️ Consider adding# type: ignorecomments only where strictly necessary (none visible here)
Suggestions for Future Enhancements
- Add a
ToolTypedDict/Pydantic model in the public API for explicit tool definition (beyond auto-conversion from functions). - Support tool choice parameters (e.g.,
tool_choice="auto" | "none" | {"type": "function", "function": {"name": "..."}}) if the backend supports it. - Add integration tests for:
- Tool calling round-trip (sync + async)
- Error cases (invalid tool schema, model doesn't support tools)
- Deprecation path: If older Ollama versions don't support these params, add version checking with helpful warnings.
Final Verdict: Approve with Minor Revisions
The changes are well-structured, type-safe, and improve the library's functionality significantly. Before merging:
- ✅ Document the three new parameters in docstrings
- ✅ Add validation for
logprobs/top_logprobsdependency - ✅ Ensure
_copy_tools(None)returns[]and test edge cases - ✅ (Optional) Refactor sync/async duplication if maintainability becomes a concern
This pull request introduces automatic tool execution for chat interactions in the
ollamaclient, allowing tools to be executed in a loop up to a specified maximum number of times via a newmax_tool_callsparameter. It updates both synchronous and asynchronouschatmethods, enhances documentation and examples, and adds internal logic for executing tool calls. The changes improve usability for tool-augmented chat workflows and provide clearer guidance for both manual and automated tool handling.Automatic tool execution support:
max_tool_callsparameter to both sync and asyncchatmethods inollama/_client.py. When set, the client will automatically execute tool calls in a loop, feeding tool outputs back to the model, up to the specified number of iterations. If not set or set toNone, manual tool handling remains the default. [1] [2] [3] [4] [5] [6]chatmethods, including error handling for exceeding the maximum number of tool calls. [1] [2] [3] [4]Internal utilities:
_exec_toolhelper function to execute a tool call and serialize the result, handling errors if the tool is not found.Documentation and examples:
ollama/_client.pyto explain the newmax_tool_callsparameter and demonstrate both manual and automatic tool execution patterns. [1] [2]examples/async-tools.pyscript to showcase both auto and manual tool handling, and switched the example model toqwen3.5:4b. [1] [2]