feat: make call_tools public - #1544
Conversation
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
23167a9 to
988d217
Compare
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
Looks good, will give others a chance to weigh in since we had a fair amount of discussion.
| from mellea.stdlib import call_tools | ||
| from mellea.stdlib.context import SimpleContext | ||
|
|
||
| result, ctx = instruct("...", context, backend, tool_calls=True) | ||
| tool_messages = call_tools(result, backend) | ||
| ctx = ctx.add(tool_messages) |
There was a problem hiding this comment.
| from mellea.stdlib import call_tools | |
| from mellea.stdlib.context import SimpleContext | |
| result, ctx = instruct("...", context, backend, tool_calls=True) | |
| tool_messages = call_tools(result, backend) | |
| ctx = ctx.add(tool_messages) | |
| from mellea.stdlib.functional import call_tools | |
| result, ctx = instruct("...", context, backend, tool_calls=True) | |
| tool_messages = call_tools(result, backend) | |
| for tool_message in tool_messages: | |
| ctx = ctx.add(tool_message) |
Import seems to not match, the example in call_tools_basic.py seems to require the loop for the tool message ctx assign.
| @pytest.mark.asyncio | ||
| async def test_acall_tools_executes_all_parallel_same_name_calls(backend): | ||
| """Verify _acall_tools() executes all parallel same-name tool calls. | ||
| async def testacall_tools_executes_all_parallel_same_name_calls(backend): |
There was a problem hiding this comment.
I think the names should still be prefixed with test_ here - ie test_acall_etc
| @patch("mellea.stdlib.functional.aact", new_callable=AsyncMock) | ||
| async def test_atransform_persists_chosen_tool_message_in_context( | ||
| mock_aact, mock_acall_tools | ||
| mock_aact, mockacall_tools |
There was a problem hiding this comment.
I think these two instances should remain mock_acall_tools as well.
| if step.tool_calls is not None: | ||
| # Code below assumes the tool is called here. | ||
| tool_responses = mfuncs._call_tools(step, backend=backend) | ||
| tool_responses = mfuncs.call_tools(step, backend=backend) |
There was a problem hiding this comment.
I think in the discussion it was mentioned to switch to acall_tools
| tool_responses = mfuncs.call_tools(step, backend=backend) | |
| tool_responses = await mfuncs.acall_tools(step, backend=backend) |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
@AngeloDanducci Thanks for review. All comments are addressed. |
|
Not a big deal, but there is a stale reference to _call_tools in a comment in test_tracing_tools.py Could be a follow-up, but might be nice to catch now |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
May be worth adding the new how-to docs to the sidebar?
Otherwise LGTM.
462c6ea
3 participants
Pull Request
Issue
Fixes #1546
Description
fix discussion: #1460
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.