Remove default template and unify template loading #3722
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request removes the default chat template fallback mechanism and unifies template loading to rely on model-provided templates. The changes transition from using a hardcoded default template to requiring proper chat templates in model files, with support for creating dummy templates during model preparation when needed.
Key Changes:
- Removed default template fallback logic in both Python and C++ builds
- Updated model preparation scripts to create dummy chat templates for models lacking them
- Modified tests to copy proper chat templates and updated expectations to match new behavior
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/llm/servable_initializer.hpp | Removed declaration of loadDefaultTemplateProcessorIfNeeded method for C++ builds |
| src/llm/servable_initializer.cpp | Eliminated default template constant and fallback loading logic; simplified to use only model-provided templates |
| src/llm/servable.cpp | Added exception handling for chat template application failures |
| src/test/llm/llmtemplate_test.cpp | Updated tests to copy real tokenizer files and chat templates; adjusted expected outputs to match new template behavior |
| src/test/llm/llmnode_test.cpp | Updated test helper methods to support chat template application and adjusted test expectations |
| src/test/llm/assisted_decoding_test.cpp | Modified test helper to apply chat templates when needed and uncommented previously skipped assertions |
| windows_prepare_llm_models.bat | Added logic to create dummy chat template file if not present after model download |
| prepare_llm_models.sh | Renamed model variable and added dummy chat template creation for models without templates |
Comments suppressed due to low confidence (1)
windows_prepare_llm_models.bat:1
- Inconsistent Jinja2 delimiter syntax: line 85 uses
{%instead of{%%while other lines use{%%. This inconsistency may cause template parsing issues.
::
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Intermediate change for merging: #3722
mzegla
force-pushed
the
remove_default_template
branch
2 times, most recently
from
d63d874 to
d1ca964
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (errorFound) | ||
| return; | ||
| // At this point tokenizer cannot be uninitialized as we need to access its methods for prepare for chat template processing | ||
| if (properties->tokenizer == ov::genai::Tokenizer()) { |
There was a problem hiding this comment.
[nitpick] Comparing tokenizer equality against a default-constructed instance may not reliably detect uninitialized state. Consider adding an explicit is_initialized() or is_valid() method to the Tokenizer class, or check for null/empty internal state instead.
| if (properties->tokenizer == ov::genai::Tokenizer()) { | |
| // Use a more robust check for tokenizer initialization | |
| if (properties->tokenizer.get_vocab_size() == 0) { |
mzegla
force-pushed
the
remove_default_template
branch
from
ff2925f to
fa03b7a
Compare
No description provided.