Skip to content

fix: graceful fallback when model has no chat_template (MedGemma)#271

Open
jackneil wants to merge 1 commit intowaybarrios:mainfrom
jackneil:pr/medgemma-chat-template
Open

fix: graceful fallback when model has no chat_template (MedGemma)#271
jackneil wants to merge 1 commit intowaybarrios:mainfrom
jackneil:pr/medgemma-chat-template

Conversation

@jackneil
Copy link
Copy Markdown
Contributor

@jackneil jackneil commented Apr 9, 2026

Summary

  • Models like MedGemma have apply_chat_template() inherited but no chat_template configured
  • Previously crashed with ValueError: Cannot use apply_chat_template when no chat_template is set
  • Server returned HTTP 200 then dropped the connection mid-stream
  • Now catches ValueError and falls back to plain-text prompt format in both BatchedEngine and SimpleEngine

Test plan

  • 4 unit tests verifying fallback behavior
  • No regressions in existing tests (112 passing)
  • End-to-end with MedGemma model

🤖 Generated with Claude Code

Models like MedGemma have apply_chat_template() as an inherited method
but no chat_template configured, causing ValueError on every request.
Now catches ValueError and falls back to plain-text prompt format
in both BatchedEngine and SimpleEngine paths.

Fixes: MedGemma crashes with "Cannot use apply_chat_template"

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Thump604
Copy link
Copy Markdown
Collaborator

One correctness concern before merge: the new fallback paths catch broad TypeError / ValueError from apply_chat_template() and silently drop to plain-text prompts.

That is fine for the specific MedGemma case (no chat_template configured), but it also masks unrelated bugs like template argument mismatches or other real template failures. In those cases we would silently change behavior instead of surfacing the regression.

I think the fallback should be narrowed to the known missing-template case only — e.g. catch ValueError with the expected "no chat_template" / "Cannot use apply_chat_template when no chat_template is set" class of message — and let other exceptions surface normally.

Copy link
Copy Markdown
Collaborator

@Thump604 Thump604 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked this against current main, and I do not think it is merge-ready in its current shape.

Two blockers:

  1. The fallback is still too broad. Catching generic TypeError / ValueError from apply_chat_template() will silently mask unrelated template regressions. This should be narrowed to the known missing-template case only (the no chat_template configured / Cannot use apply_chat_template when no chat_template is set class of failure).

  2. More importantly, this branch does not actually cover the real SimpleEngine.chat() crash path on current main. The non-streaming simple-engine path still delegates straight into self._model.chat(...) before any of this local fallback logic runs, so a model/processor with apply_chat_template() present but no configured chat_template can still fail before reaching the new fallback code. The current diff mainly helps the local prompt-building/accounting paths, not the full execution path.

Given that the branch is also conflicting, I would rather see a small current-main replacement that fixes the real execution path and adds a regression against that path specifically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants