feat: Add Tool.from_component #159

vblagoje · 2024-12-18T11:20:44Z

Why:

Automates the conversion of Haystack components into LLM tools.

fixes Add Tool.from_component for components with str/native python types as input haystack#8630

What:

Added component_schema.py, that converts component run parameters into a JSON tools schema format.
Modified tool.py to introduce a from_component method, enabling the creation of Tool instances from Haystack components. This makes tool creation and integration into pipelines more dynamic.
Added comprehensive unit tests in test_tool_component.py, validating the conversion and functionality of components as tools, including testing various data types and nested structures.

How can it be used:

Convert a component to a tool:

tool = Tool.from_component(
    component=myComponent,
    name="my_tool",
    description="A tool for processing data"
)

and the use it as a Tool following the established patterns.

How did you test it:

Conducted unit tests covering various component types such as simple return types, data classes, and Pydantic models.
Performed integration tests with pipelines utilizing both OpenAI and Anthropic backend models, verifying tool correctness and error management.

Notes for the reviewer:

Pay attention to the handling of nested data structures and nullable types to ensure they are mapped and invoked correctly.

coveralls · 2024-12-18T15:58:17Z

Pull Request Test Coverage Report for Build 12653556844

Details

0 of 0 changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.5%) to 83.634%

Files with Coverage Reduction	New Missed Lines	%
dataclasses/tool.py	2	98.26%

Totals
Change from base Build 12624157864:	0.5%
Covered Lines:	2136
Relevant Lines:	2554

💛 - Coveralls

anakin87

Thanks for the effort you are putting into this PR.

I do have some questions and suggestions.

The requirement was to support components with basic str or native Python types as input. This implementation appears to go beyond that, which is great for flexibility but might become hard to maintain.

What's the advantage of supporting Pydantic models here? Maybe I am just missing some reasonable use cases...

As I commented in the PR, if I remember correctly, one of the initial requirements was to enable the deserialization of Tools from YAML, which would be feasible if Tools are treated as components. Is it possible? If yes, can we add some tests to cover this?

Given the main goal of this PR, would it make sense to involve someone from the DC team in the review?

haystack_experimental/dataclasses/tool.py

anakin87 · 2024-12-19T15:29:10Z

haystack_experimental/dataclasses/tool.py

+            msg = (
+                "Component has been added in a Pipeline and can't be used to create a Tool. "
+                "Create Tool from a non-pipeline component instead."
+            )


Can you please explain this?

If I remember correctly, one of the requirements was about deserializing Tools from YAML (which should be feasible if Tools are components). I'm not totally sure...

Yes, I thought we can have a component declared but not be part of the pipeline. Maybe not, depending on that we can remove this check.

I still don't understand if this is a self-imposed limitation (I don't think so) or there are strong reasons to avoid that. Could you please explain this point further?

vblagoje · 2024-12-19T16:39:36Z

Thanks for the effort you are putting into this PR.

I do have some questions and suggestions.

The requirement was to support components with basic str or native Python types as input. This implementation appears to go beyond that, which is great for flexibility but might become hard to maintain.

What's the advantage of supporting Pydantic models here? Maybe I am just missing some reasonable use cases...

The TypeAdapter from pydantic takes care of range of json to object conversion be these objects strings or other native Python types, dataclasses or Pydantic objects.

It is in fact, harder and longer codebase to implement a version of Tool.from_component that only supports strings, native Python types and their lists as we need to detect cases we don't support, raise errors and so on.

TypeAdapter.validate_python takes conversion of all of these. Unit and integration tests for dataclasses and Pydantic models I included showcase how this is indeed possible with almost no code. We even support our own Document class as show in test examples.

Given these newfound benefits of TypeAdapter which enable proper conversion support with 10 lines of well tested pydantic code - I went for that solution although it was not required by the requirements.

As I commented in the PR, if I remember correctly, one of the initial requirements was to enable the deserialization of Tools from YAML, which would be feasible if Tools are treated as components. Is it possible? If yes, can we add some tests to cover this?

Yes, I need to review this part as well.

Given the main goal of this PR, would it make sense to involve someone from the DC team in the review?

Yes, @mathislucka is on PTO and he had the most context. Let's wait for him

anakin87

I like the simplification you have made.

I left other comments and asked Julian to take a look as well.

anakin87 · 2024-12-20T15:10:55Z

haystack_experimental/tools/__init__.py

+from .component_schema import create_tool_parameters_schema
+
+__all__ = ["create_tool_parameters_schema"]


I would not export this function here if possible - see Importing one component of a certain family/module leads to importing all components of the same family/module haystack#8650

I would prefer to make this method internal and also all others in component_schema.py. They should not be user-facing and if we make them internal, we are then free to change them at any time if needed.

Ok makes sense, will do 🙏

julian-risch

Tool.from_component functionality is great! I have some feedback about the tests and suggest we add defaults for tool name and tool description so that the from_component requires only a component as input for standard use.

For the tests:
I suggest replacing gpt-4o with gpt-4o-mini if that works. Also consider replacing claude-3-5-sonnet-20240620 with a simpler model if that works?

I understand that the long instructions like "Concatenate these documents: First one says 'Hello world' and second one says 'Goodbye world'. Set only content field of the document only. Do not set id, meta, score, embedding, sparse_embedding, dataframe, blob fields." are meant to test the implementation. Still, I suggest that we use Document instances as input directly too. For example, have one e2e test with a retriever as a tool from component and another component that takes documents as input as a second tool.
As the test for OpenAI and Anthropic are very similar, let's use parametrization for the tests, which will reduce lines of code. You might need to use pytest.skip() within the test to skip if api keys are not available then.
Last but not least, we should have a test case that covers loading tools from components defined in a pipeline yaml. That should be a YAML where a ToolInvoker is defined and it's init parameter for tools is a list of components. The ToolInvoker would need to call from_component internally then as we discussed offline.

julian-risch · 2025-01-06T11:19:45Z

haystack_experimental/dataclasses/tool.py

+        Create a Tool instance from a Haystack component.
+
+        :param component: The Haystack component to be converted into a Tool.
+        :param name: Name for the tool.


I don't want to make this PR more complex but have we considered using defaults for name and description? For example the component name as the default for name and parts of the component docstring extracted via docstring_parser for description?
That would also enable the ToolInvoker to call from_component internally if the input to its tool parameter is a list of components instead of tools, which I think is needed for defining tools as components in a yaml.

vblagoje · 2025-01-09T14:36:43Z

Abandoned and superseded by deepset-ai/haystack#8693

vblagoje added 7 commits December 18, 2024 11:48

Initial Tool.from_component

784a19c

Simplify types conversion with TypeAdapter

6d6f307

Pylint, small fixes

eb1496c

Improve warning when component run pydocs are missing

519d53f

Add Anthropic integration tests

7178e1d

Minor test fix

d83ca13

Merge branch 'main' into from_component

6341268

vblagoje added 4 commits December 19, 2024 10:57

Handle our own dataclasses (e.g. Document)

1c259f9

For dataclasses don't check required fields, add more itegration tests

34a0861

Small fix for better test

551a528

Make sure we are only using non-pipeline components for Tools

ce864dd

anakin87 reviewed Dec 19, 2024

View reviewed changes

anakin87 requested a review from julian-risch December 19, 2024 16:17

vblagoje added 3 commits December 20, 2024 14:42

Move modules around

931df70

Refactor and simplify tools schema creation

3b29458

Better naming

d8f722c

vblagoje marked this pull request as ready for review December 20, 2024 14:07

vblagoje requested a review from a team as a code owner December 20, 2024 14:07

vblagoje requested review from anakin87 and removed request for a team December 20, 2024 14:07

Rename module

2dbb8d2

anakin87 reviewed Dec 20, 2024

View reviewed changes

vblagoje added 3 commits December 20, 2024 17:17

PR feedback

b35c098

Merge branch 'main' into from_component

d2b7eb2

Add top_k to DocumentProcessor, update tests

9c840bf

julian-risch requested changes Jan 6, 2025

View reviewed changes

vblagoje added 2 commits January 6, 2025 14:42

Make name/description optional

150fb81

Switch to gpt-4o-mini

1444749

anakin87 mentioned this pull request Jan 8, 2025

Tool from Component YAML serde POC #164

Closed

vblagoje closed this Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Tool.from_component #159

feat: Add Tool.from_component #159

vblagoje commented Dec 18, 2024 •

edited

Loading

coveralls commented Dec 18, 2024 •

edited

Loading

anakin87 left a comment

anakin87 Dec 19, 2024

vblagoje Dec 19, 2024

anakin87 Dec 20, 2024

vblagoje commented Dec 19, 2024 •

edited

Loading

anakin87 left a comment

anakin87 Dec 20, 2024

vblagoje Dec 20, 2024

julian-risch left a comment

julian-risch Jan 6, 2025

vblagoje commented Jan 9, 2025

		from .component_schema import create_tool_parameters_schema

		__all__ = ["create_tool_parameters_schema"]

feat: Add Tool.from_component #159

feat: Add Tool.from_component #159

Conversation

vblagoje commented Dec 18, 2024 • edited Loading

Why:

What:

How can it be used:

How did you test it:

Notes for the reviewer:

coveralls commented Dec 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12653556844

Details

💛 - Coveralls

anakin87 left a comment

Choose a reason for hiding this comment

anakin87 Dec 19, 2024

Choose a reason for hiding this comment

vblagoje Dec 19, 2024

Choose a reason for hiding this comment

anakin87 Dec 20, 2024

Choose a reason for hiding this comment

vblagoje commented Dec 19, 2024 • edited Loading

anakin87 left a comment

Choose a reason for hiding this comment

anakin87 Dec 20, 2024

Choose a reason for hiding this comment

vblagoje Dec 20, 2024

Choose a reason for hiding this comment

julian-risch left a comment

Choose a reason for hiding this comment

julian-risch Jan 6, 2025

Choose a reason for hiding this comment

vblagoje commented Jan 9, 2025

vblagoje commented Dec 18, 2024 •

edited

Loading

coveralls commented Dec 18, 2024 •

edited

Loading

vblagoje commented Dec 19, 2024 •

edited

Loading