fix: Implement Ollama sequential tool calling fix #879
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Modified conversation format for Ollama to handle tool results properly: - Assistant messages no longer include 'tool_calls' field for Ollama - Tool results are passed as user messages with natural language format - Prevents infinite loop of repeated tool calls This allows Ollama to properly execute sequential tool calls as intended. Fixes #854 Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
""" WalkthroughThe changes update the message formatting logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LLM
participant Tool
User->>LLM: Submit prompt with tools
LLM->>LLM: Check provider (Ollama or other)
LLM->>Tool: Call tool(s) as required
Tool-->>LLM: Return tool result
alt Provider is Ollama
LLM->>LLM: Append messages with Ollama-specific format
else Other providers
LLM->>LLM: Append messages with standard tool_calls format
end
LLM-->>User: Return final response
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Poem
""" ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@claude review this pull request and do a detailed analysis and fix this if the existing code doesn't have the solution implemented. Making sure it has backward compatibility, no existing features removed. After making those changes, again review the applied changes. Use @web to search if you dont know any information or to find the latest documentation or to find the latest version. Run the code if you think you need to run it to test it. Minimal code change to start with if required any changes. |
|
Claude finished @MervinPraison's task —— View job Task List
Final Review Summary✅ Original Fix AssessmentThe PR correctly implements a fix for Ollama's sequential tool calling issue by:
🔧 Improvement AppliedI enhanced the # Now detects:
# - Direct "ollama/" prefix in model name ✓
# - "ollama" in base_url for custom configurations (NEW) ✓
# - Environment variables containing "ollama" or port 11434 (ENHANCED) ✓
# - Works with custom ports beyond just 11434 (NEW) ✓✅ Verification Results
📋 ConclusionThe PR successfully addresses issue #854 with a minimal, backwards-compatible fix. The enhancement I added improves robustness for various Ollama deployment scenarios without affecting the core solution. Recommendation: ✅ Ready to merge |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MervinPraison, 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 a targeted fix for an issue where Ollama models would get stuck in an infinite loop during sequential tool calls. The core of the solution involves adapting the conversation message format specifically for Ollama, ensuring it correctly understands and processes both assistant responses and tool execution results, thereby enabling proper multi-turn tool interactions.
Highlights
- Ollama Assistant Message Format: Assistant messages generated for Ollama providers no longer include the
tool_callsfield. This adjustment is made to align with Ollama's expected conversation format and prevent issues like infinite loops during sequential tool calling. - Ollama Tool Result Handling: Tool results returned from executed functions are now passed back to Ollama as
usermessages, formatted in natural language (e.g., 'The function_name function returned: {result}'). This replaces the previous method of using atoolrole with atool_call_idand raw JSON content, which Ollama models might not interpret correctly. - Backward Compatibility: All modifications to message formatting are conditionally applied only when the active LLM provider is Ollama. This ensures that the existing conversation and tool calling mechanisms for all other LLM providers remain unchanged and fully compatible.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses an important bug with Ollama's sequential tool calling by adjusting the message format. The changes are applied consistently across both the synchronous and asynchronous get_response methods. The newly added logic introduces some code duplication that could be refactored for better maintainability. Additionally, the get_response and get_response_async methods remain very similar, suggesting a potential future refactoring to consolidate them for improved code clarity and maintenance.
Comment on lines
+815
to
+828
| # Check if this is Ollama provider | ||
| if self._is_ollama_provider(): | ||
| # For Ollama, only include role and content | ||
| messages.append({ | ||
| "role": "assistant", | ||
| "content": response_text | ||
| }) | ||
| else: | ||
| # For other providers, include tool_calls | ||
| messages.append({ | ||
| "role": "assistant", | ||
| "content": response_text, | ||
| "tool_calls": serializable_tool_calls | ||
| }) |
There was a problem hiding this comment.
This block for appending the assistant message introduces code duplication. It can be simplified by constructing a base message dictionary and then conditionally adding the tool_calls key for non-Ollama providers. This approach enhances conciseness and maintainability.
# Create the base assistant message
assistant_message = {
"role": "assistant",
"content": response_text,
}
# Add tool_calls for non-Ollama providers
if not self._is_ollama_provider():
assistant_message["tool_calls"] = serializable_tool_calls
messages.append(assistant_message)
Comment on lines
+1325
to
+1338
| # Check if it's Ollama provider | ||
| if self._is_ollama_provider(): | ||
| # For Ollama, only include role and content | ||
| messages.append({ | ||
| "role": "assistant", | ||
| "content": response_text | ||
| }) | ||
| else: | ||
| # For other providers, include tool_calls | ||
| messages.append({ | ||
| "role": "assistant", | ||
| "content": response_text, | ||
| "tool_calls": serializable_tool_calls | ||
| }) |
There was a problem hiding this comment.
This logic is a duplicate of the one found in the synchronous get_response method. To improve maintainability and reduce code duplication, this block should be refactored. By creating the base message dictionary first and then conditionally adding the tool_calls, the code becomes cleaner and less repetitive.
# Create the base assistant message
assistant_message = {
"role": "assistant",
"content": response_text,
}
# Add tool_calls for non-Ollama providers
if not self._is_ollama_provider():
assistant_message["tool_calls"] = serializable_tool_calls
messages.append(assistant_message)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/praisonai-agents/praisonaiagents/llm/llm.py (2)
854-868: Proper tool result format for Ollama compatibility.The implementation correctly formats tool results for Ollama as user messages in natural language, while maintaining the standard tool message format for other providers. This is essential for preventing Ollama's infinite loop issue.
Consider adding error handling for JSON serialization:
- tool_result_content = json.dumps(tool_result) if tool_result is not None else "an empty output" + try: + tool_result_content = json.dumps(tool_result) if tool_result is not None else "an empty output" + except (TypeError, ValueError): + tool_result_content = str(tool_result) if tool_result is not None else "an empty output"
1356-1370: Async version maintains consistency with sync implementation.The asynchronous tool result handling correctly mirrors the synchronous version, ensuring uniform Ollama compatibility across both execution modes. The natural language formatting for Ollama is properly implemented.
Same JSON serialization improvement applies here:
- content = f"The {function_name} function returned: {json.dumps(tool_result) if tool_result is not None else 'an empty output'}" + try: + content = f"The {function_name} function returned: {json.dumps(tool_result) if tool_result is not None else 'an empty output'}" + except (TypeError, ValueError): + content = f"The {function_name} function returned: {str(tool_result) if tool_result is not None else 'an empty output'}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/llm/llm.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : The 'LLM' class in 'llm.ts' should wrap 'aisdk.generateText' calls for generating text responses.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage for large language model calls in Node.js/TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Use the unified LLM wrapper in `praisonaiagents/llm/` for integrating with multiple LLM providers.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/{llm,agent,agents,task}/**/*.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage in TypeScript code.
src/praisonai-agents/praisonaiagents/llm/llm.py (6)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : The 'LLM' class in 'llm.ts' should wrap 'aisdk.generateText' calls for generating text responses.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Use the unified LLM wrapper in `praisonaiagents/llm/` for integrating with multiple LLM providers.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage for large language model calls in Node.js/TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/{llm,agent,agents,task}/**/*.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage in TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Use the `Agent` class from `praisonaiagents/agent/` for core agent implementations, supporting LLM integration, tool calling, and self-reflection.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/agent/agent.ts : The 'Agent' class in 'src/agent/agent.ts' should encapsulate a single agent's role, name, and methods for calling the LLM using 'aisdk'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor BugBot
- GitHub Check: test-core (3.11)
- GitHub Check: quick-test
- GitHub Check: Run tests and collect coverage
🔇 Additional comments (2)
src/praisonai-agents/praisonaiagents/llm/llm.py (2)
815-828: Correct implementation for Ollama assistant message format.The conditional handling properly excludes the
tool_callsfield for Ollama providers while maintaining the existing behavior for other providers. This addresses the infinite loop issue by ensuring Ollama receives messages in its expected format.
1325-1338: Consistent async implementation matches sync behavior.The asynchronous version correctly mirrors the synchronous implementation, ensuring consistent Ollama handling across both execution modes. The conditional logic is identical and maintains the same backward compatibility.
- Add check for "ollama" in base_url for custom configurations - Support detection of custom Ollama ports beyond 11434 - More robust detection for various deployment scenarios Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/praisonai-agents/praisonaiagents/llm/llm.py (2)
821-834: Address the code duplication by refactoring the assistant message construction.This conditional block has already been identified for refactoring in previous reviews. The suggested approach of creating a base message dictionary and conditionally adding the
tool_callsfield would improve maintainability and reduce duplication.
1331-1344: Address the code duplication by refactoring the assistant message construction.This logic duplicates the synchronous method's conditional assistant message handling. As noted in previous reviews, refactoring to use a base message dictionary with conditional
tool_callsaddition would eliminate this duplication and improve maintainability.
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/llm/llm.py (1)
280-294: Enhance Ollama detection logic while simplifying the return condition.The enhanced detection logic is comprehensive and correctly addresses various ways Ollama providers can be configured. However, the return condition can be simplified per the static analysis hint.
Apply this diff to simplify the return condition:
- # Common Ollama endpoints (including custom ports) - if any(url and ("ollama" in url.lower() or ":11434" in url) - for url in [base_url, api_base, self.base_url or ""]): - return True - - return False + # Common Ollama endpoints (including custom ports) + return any(url and ("ollama" in url.lower() or ":11434" in url) + for url in [base_url, api_base, self.base_url or ""])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/llm/llm.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : The 'LLM' class in 'llm.ts' should wrap 'aisdk.generateText' calls for generating text responses.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage for large language model calls in Node.js/TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Use the unified LLM wrapper in `praisonaiagents/llm/` for integrating with multiple LLM providers.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/{llm,agent,agents,task}/**/*.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage in TypeScript code.
src/praisonai-agents/praisonaiagents/llm/llm.py (5)
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-agents/CLAUDE.md:0-0
Timestamp: 2025-06-30T10:06:17.673Z
Learning: Use the unified LLM wrapper in `praisonaiagents/llm/` for integrating with multiple LLM providers.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : The 'LLM' class in 'llm.ts' should wrap 'aisdk.generateText' calls for generating text responses.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/llm/llm.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage for large language model calls in Node.js/TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.windsurfrules:0-0
Timestamp: 2025-06-30T10:06:44.129Z
Learning: Applies to src/praisonai-ts/src/{llm,agent,agents,task}/**/*.ts : Replace all references to 'LLM' or 'litellm' with 'aisdk' usage in TypeScript code.
Learnt from: CR
PR: MervinPraison/PraisonAI#0
File: src/praisonai-ts/.cursorrules:0-0
Timestamp: 2025-06-30T10:05:51.843Z
Learning: Applies to src/praisonai-ts/src/agent/agent.ts : The 'Agent' class in 'src/agent/agent.ts' should encapsulate a single agent's role, name, and methods for calling the LLM using 'aisdk'.
🪛 Ruff (0.11.9)
src/praisonai-agents/praisonaiagents/llm/llm.py
290-294: Return the condition directly
Inline condition
(SIM103)
🔇 Additional comments (2)
src/praisonai-agents/praisonaiagents/llm/llm.py (2)
860-874: Tool result formatting correctly addresses Ollama sequential tool calling issue.The conditional formatting ensures Ollama receives tool results as user messages in natural language format, which resolves the infinite loop issue described in the PR objectives. The structured approach for other providers maintains backward compatibility.
1362-1376: Async tool result formatting correctly mirrors the sync implementation.The conditional formatting in the async method properly implements the same Ollama-specific message handling as the sync version, ensuring consistent behavior across both execution modes. This maintains the fix for Ollama's sequential tool calling issue.
|
@claude review if the changes made are as per the requirements |
|
Claude finished @MervinPraison's task —— View job Task List
Review Summary✅ Requirements MetThe implementation correctly addresses all requirements from issue #854:
📊 Technical AssessmentThe implementation is well-structured with:
🎯 ConclusionThe changes are correctly implemented as per requirements. The fix specifically addresses Ollama's sequential tool calling issue by adapting the message format to what Ollama expects, while maintaining full backward compatibility for all other LLM providers. The solution should resolve the infinite loop issue described in #854, allowing Ollama to properly execute sequential tool calls. |
|
@claude still it failed |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @MervinPraison's task —— View job • Task List
Analysis & SolutionI've identified and fixed the issues with Ollama's sequential tool calling: Issues Found:
Fix Implemented:
Changes Made:The fix modifies the tool result formatting for Ollama to include: # Extract numeric values for clarity
numeric_match = re.search(r'\b(\d+(?:\.\d+)?)\b', tool_result_str)
if numeric_match:
numeric_value = numeric_match.group(1)
content = f"The {function_name} function returned: {tool_result_str}. The numeric value is {numeric_value}."And adds explicit prompting after tool execution: if self._is_ollama_provider() and iteration_count > 0:
messages.append({
"role": "user",
"content": "Based on the tool results above, please provide the final answer to the original question."
})Backward Compatibility:✅ All changes are conditional based on Next Steps:Please test the fix with your
| |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #854
Summary
Implemented fix for Ollama sequential tool calling issue where it was getting stuck in an infinite loop.
Changes
tool_callsfield for OllamaThis allows Ollama to properly understand tool results and execute sequential tool calls.
Generated with Claude Code
Summary by CodeRabbit