-
Notifications
You must be signed in to change notification settings - Fork 199
feat: LlamaStackChatGenerator update tools param to ToolsType
#2436
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
Conversation
|
Ok after a lot of small fixes - this one should be ready now @anakin87 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from the ToolsType point of view.
However, I'd appreciate it if @Amnah199 could take a look at other changes, since she is more familiar with this integration.
| curl -f http://localhost:8321/v1/models || { cat server.log; exit 1; } | ||
| echo "Llama Stack Server started successfully." | ||
| set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amnah199 please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what was happening is this look at the date stamp - 2 days ago. And it seems like llama stack doesn't have build command any more. So I switched to run command because that one made sense. Using run made a difference and the server started properly.
Then I faced this issue of names. Tests would fail saying there is no such model. I then switched to full naming schema and then everything started to work. cc @anakin87 @Amnah199
| def test_init_default(self): | ||
| component = LlamaStackChatGenerator(model="llama3.2:3b") | ||
| assert component.model == "llama3.2:3b" | ||
| component = LlamaStackChatGenerator(model="ollama/llama3.2:3b") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing the way we pass the model name? Is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be - it was failing for the "llama3.2:3b". I looked at the CI run, the model was booted and ready. Then I tried this naming schema and it worked
Yes makes sense. @Amnah199 I spent half the day yesterday to get this integration up and running because the old llama stack build command doesn't work anymore. Then I somehow made it run with lamma stack run but the model wasn't recognized without full name. And then with full name it finally started to work. |
|
Ah sorry @anakin87 I clicked by accident thinking it was Amna |
|
@vblagoje Thanks for the updates. When we created this integration, llama stack was still under development so they were making rapid breaking changes. Hence, all the trouble you had to go through. |
|
If passing |
|
Ok I'll update the pydocs now as well. 🙏 @anakin87 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Please also open a PR to update https://haystack.deepset.ai/integrations/llama_stack
For some reason, docs are already correct.
That's crazy, premonition of a change! We were correct about their naming scheme even before them |
|
We need to verify and merge deepset-ai/haystack-integrations#368 cc @bilgeyucel |
Why:
Adopts Haystack's
ToolsTypeto enable flexible tool composition, developers can now mix individualToolobjects andToolsetinstances in a single listpart of:
What:
toolsparameter fromUnion[List[Tool], Toolset]toToolsTypein constructorToolsTypesupport2.19.0forToolsTypesupportHow can it be used:
How did you test it:
Notes for the reviewer:
ToolsType—eliminates duplicate handling codetest_live_run_with_mixed_tools()demonstrates end-to-end LLM tool invocation from mixed collections