-
Notifications
You must be signed in to change notification settings - Fork 609
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
fix: Fixing tool prompt format #196
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Daniele Martinoli <[email protected]>
@@ -58,7 +58,7 @@ def run_main(host: str, port: int, disable_safety: bool = False): | |||
}, | |||
toolgroups=["builtin::rag"], | |||
tool_choice="auto", | |||
tool_prompt_format="json", | |||
tool_prompt_format="python_list", |
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.
You should be able to just remove this since we added meta-llama/llama-stack#1214
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.
I see, thank you! I will update it asap
BTW: is there any way to be informed of the ongoing/planned changes before pushing PRs that are already out-of-date? 😬
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.
I don't know if there's a better way but I just look at open PRs periodically and check github notifications.
BTW I do plan to do a full clean up of tool_prompt_format as well.
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.
BTW: I tried to remove the prompt format but resolve_model fails to resolve the model so the default prompt is still json
😞
These are the models I'm using:
% llama-stack-client models list
Available Models
┏━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┓
┃ model_type ┃ identifier ┃ provider_resource_id ┃ metadata ┃ provider_id ┃
┡━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━┩
│ embedding │ all-MiniLM-L6-v2 │ all-minilm:latest │ {'embedding_dimension': │ ollama │
│ │ │ │ 384.0} │ │
├────────────┼──────────────────┼──────────────────────┼──────────────────────────┼─────────────┤
│ llm │ llama3.2:1b │ llama3.2:1b │ │ ollama │
└────────────┴──────────────────┴──────────────────────┴──────────────────────────┴─────────────┘
Total models: 2
Changing the check to compare lower-cased strings seems to work, but I'm not sure it may break something else:
for m in all_registered_models():
if descriptor.lower in (m.descriptor().lower(), m.huggingface_repo):
return m
return None
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.
ok let's get this in first
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.
@dmartinol where are you getting llama3.2:1b
? It seems to be in the wrong format
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.
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.
ollama run llama3.2:1b
, is that wrong?
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.
anyhow meta-llama/llama-stack#1360 should fix this
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.
I will try again and clear the setting.
What does this PR do?
Closes #195
Feature/Issue validation/testing/test plan
Start llama-stack server, then:
Before submitting
Pull Request section?
to it if that's the case.
Thanks for contributing 🎉!