Skip to content

feat: OllamaChatGenerator - add Toolset support #1765

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

Merged
merged 8 commits into from
May 22, 2025

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented May 19, 2025

Why:

Enhance OllamaChatGenerator by allowing it to accept a Toolset instance in addition to a list of Tool objects. This brings it in line with other chat generators and improves consistency and flexibility in tool usage.

What:

  • Updated OllamaChatGenerator to support tools parameter as either a List[Tool] or a Toolset
  • Adjusted serialization logic to handle Toolset using serialize_tools_or_toolset
  • Modified deserialization to support both formats using deserialize_tools_or_toolset_inplace

How can it be used:

The OllamaChatGenerator can now be used with a Toolset for better tool management and compatibility across generators:

toolset = Toolset(tools)
generator = OllamaChatGenerator(model="qwen3:4b", tools=toolset)

It also allows seamless deserialization and execution with Toolset as part of the component configuration.

How did you test it:

  • Added tests for:

    • Initialization with Toolset
    • Serialization and deserialization of Toolset-based components
    • run() execution flow using a Toolset and mocked response from Ollama client
    • Verified backward compatibility with plain list of tools

Manually:

  • I've tested qwen3:4b with itinerary agent and bunch of MCPToolset and it kinda worked. Well in some basic form expected from such a relatively small model running on my local CPU.

Notes for the reviewer:

Please confirm:

  • Serialization/deserialization logic is sound and consistent with other generators
  • ToolCall.id fallback generation doesn't break traceability or downstream logic
  • Toolset support is fully covered by new test cases without affecting existing behavior

Note:

@github-actions github-actions bot added integration:ollama type:documentation Improvements or additions to documentation labels May 19, 2025
@vblagoje vblagoje marked this pull request as ready for review May 21, 2025 08:09
@vblagoje vblagoje requested a review from a team as a code owner May 21, 2025 08:09
@vblagoje vblagoje requested review from mpangrazzi and anakin87 and removed request for a team May 21, 2025 08:09
@vblagoje
Copy link
Member Author

vblagoje commented May 21, 2025

@mpangrazzi let's have @anakin87 look at these changes - he's familiar with the code and the context of deepset-ai/haystack#9418

@vblagoje vblagoje removed the request for review from mpangrazzi May 21, 2025 08:10
@sjrl
Copy link
Contributor

sjrl commented May 21, 2025

@vblagoje lets make sure to pin Haystack in the deps to when we added Toolset

@anakin87
Copy link
Member

anakin87 commented May 21, 2025

Looks good in general.

I would only update this aspect in the PR description

Automatically generates UUIDs for ToolCall.id if missing (e.g., in Ollama tool responses)

I understand that this is something you had to introduce to make Langfuse work, but in the current version of this PR we are not introducing this change.

@vblagoje
Copy link
Member Author

Looks good in general.

I would only update this aspect in the PR description

Automatically generates UUIDs for ToolCall.id if missing (e.g., in Ollama tool responses)

I understand that this is something you had to introduce to make Langfuse work, but in the current version of this PR we are not introducing this change.

Right, spot on! I had to remove it pending a proper resolution of deepset-ai/haystack#9418

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from pinning Haystack (see #1765 (comment)), looks good

@vblagoje
Copy link
Member Author

Apart from pinning Haystack (see #1765 (comment)), looks good

It is already @sjrl and @anakin87 . See the project toml

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. Looks good!

@vblagoje vblagoje merged commit 9c3b13c into main May 22, 2025
6 checks passed
@vblagoje vblagoje deleted the ollama_chat_generator_toolset branch May 22, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:ollama type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Toolset support in OllamaChatGenerator
3 participants