fix: preserve generated chat parser during response parsing - #22
Merged
Conversation
Extract the response-parsing step of Model::generate() into a parse_response() helper so it can be exercised without loading a GGUF, and cover it with a regression test that renders a real chat template, feeds a canned <tool_call> response and asserts the tool call is parsed. The test fails without the parser/generation_prompt fix. Also, while here: - skip loading an empty parser definition; common_peg_arena::load() throws on an empty string, which is what the legacy (non-jinja) template path produces - cache the deserialized parser arena between turns instead of re-parsing the definition on every generation - wrap parse failures in ModelError; now that a real parser is loaded, llama.cpp throws when output does not match the template's format, and that previously escaped the agent loop as a bare std::runtime_error Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI configures CMAKE_BUILD_TYPE=Release, which defines NDEBUG and turned every assert()-based assertion in the test suite into a no-op. Throw instead, so the tests actually guard something on CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A hung ChatParserTests held a Windows runner for the full 6h default job limit, and nothing cancelled superseded runs, so three of them stacked up. - timeout-minutes on every job, and ctest --timeout 300 plus a per-test TIMEOUT of 120s, so a hang fails fast and names the test - concurrency group with cancel-in-progress - ctest --output-on-failure so the failure is diagnosable from the log - put bin/<config> on PATH for Windows tests; multi-config generators put DLLs there rather than in bin/ - trace the phases of the parser test on stderr, to identify where it stalls on Windows Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
thanks for the catch @oglego 🙏 |
Contributor
Author
|
Happy to help! Thanks for all of the enhancements to it! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
While going through the
examples/context-engineering/README.mdexample, I noticed tool calling wasn't working as documented:I dug into this and found that there was a complete refactoring of the parser architecture in llama.cpp (PR #18675), which impacts tool calling here - see
deps/llama.cpp/docs/autoparser.mdfor background.The PEG parser needs to be loaded via
params.parser;deps/llama.cpp/tests/test-chat.cpphas examples of this.generation_promptalso needs to be passed through - without it, I think llama.cpp parses the raw response alone instead ofgeneration_prompt + response, which is what the derived grammar actually expects.Change
The only update needed to resolve this:
Testing
Verified end-to-end with a multi-turn conversation against
granite-4.0-micro-Q8_0.gguf- tool calls parse and execute correctly:Let me know if I am missing something on this, and thanks in advance for the review!