-
Notifications
You must be signed in to change notification settings - Fork 67
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
Added --jinja to llama-run command #625
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Engel <[email protected]>
Reviewer's Guide by SourceryThis pull request adds the Sequence diagram for llama-run command with Jinja template supportsequenceDiagram
participant User
participant Ramalama
participant LLaMaCpp
User->>Ramalama: ramalama run model_name
activate Ramalama
Ramalama->>Ramalama: build_exec_args_run()
Note over Ramalama: Add --jinja flag
Ramalama->>LLaMaCpp: llama-run with --jinja flag
activate LLaMaCpp
LLaMaCpp->>LLaMaCpp: Apply Jinja template
LLaMaCpp-->>Ramalama: Model response
deactivate LLaMaCpp
Ramalama-->>User: Formatted output
deactivate Ramalama
Flow diagram for model execution with Jinja templatesflowchart TD
A[Start] --> B[Parse command args]
B --> C[Build exec args]
C --> D{Is --jinja flag added?}
D -->|Yes| E[Apply Jinja template]
D -->|No| F[Use raw output]
E --> G[Return formatted response]
F --> G
G --> H[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @engelmi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Is there a way to check the model and know that it needs --jinja? So we couldset it automatically? How would a non expert know that they need that flag? |
Tagging @ochafik who wrote minja, might have some advice when using the flag is most appropriate |
My initial assumption was that we don't have to distinguish, but can always use the
We could use the chat template of the model to determine if its plain/jinja/ollama/etc.. The template can either be queried separately or is embedded into the model. Thinking more about it, this is probably the way to go. Looking forward to the feedback from @ochafik |
In ggerganov/llama.cpp#11016
llama.cpp
the support for Jinja templates was added, incl. a default template. Simply appending the new--jinja
to the llama-run command in ramalama gets the granite-code running again. However, this only works for the ollama model, the model pulled from huggingface still seems broken.Trying the
--jinja
also for models with no jinja templates seems to work as well, therefore it seems fine to use--jinja
in general:Summary by Sourcery
New Features: