-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: support both <think> and <thinking> tags for LM Studio GPT-OSS models #6751
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
Conversation
…odels - Created MultiTagXmlMatcher utility to handle multiple XML tag names - Updated LM Studio handler to parse both <think> and <thinking> tags - Added comprehensive tests for the new functionality - Fixes #6750
| if (char === "<") { | ||
| // Emit any text before the tag | ||
| if (i > this.lastEmittedIndex) { | ||
| this.emit(false, this.buffer.substring(this.lastEmittedIndex, i)) |
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.
When a '<' is encountered in TEXT state, the code unconditionally emits the preceding text. This can duplicate text that’s being collected as part of a matched tag. Consider emitting plain text only when depth is 0 (i.e. not inside a matching tag).
| constructor( | ||
| private tagNames: string[], | ||
| private transform?: (chunks: XmlMatcherResult) => Result, | ||
| private position = 0, |
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.
The constructor parameter 'position' is declared but never used. Consider removing it if not needed.
| private position = 0, |
|
Does not use tags. Returns: |
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 reviewed my own code and found bugs I didn't know I wrote.
|
|
||
| // If we're inside a matched tag, collect the content | ||
| if (this.depth > 0 && this.state === "TEXT" && i >= this.lastEmittedIndex) { | ||
| this.matchedContent += char |
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.
Is this intentional? The character-by-character content collection in the matched tag could be inefficient for large responses. Consider buffering larger chunks for better performance:
| this.matchedContent += char | |
| // If we're inside a matched tag, collect the content | |
| if (this.depth > 0 && this.state === "TEXT") { | |
| const remainingContent = this.buffer.substring(i) | |
| this.matchedContent += remainingContent | |
| i = this.buffer.length - 1 | |
| } |
| this.lastEmittedIndex = i + 1 | ||
| this.matchedContent = "" // Reset matched content | ||
| } | ||
| this.depth++ |
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.
Could we handle nested tags of the same type? The current implementation might not correctly parse cases like <think>outer <think>inner</think> outer</think>. The depth tracking seems to assume all matched tags are the same, but with multiple tag names, nested different tags would work while nested same tags might not.
| const emptyBlocks = allResults.filter((r) => r.matched && r.data === "") | ||
| expect(emptyBlocks.length).toBeGreaterThan(0) | ||
| }) | ||
| }) |
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.
Could we add tests for streaming behavior with partial chunks? For example, when a tag is split across chunks like receiving <thi in one chunk and nking>content</thinking> in the next. This would ensure the matcher handles real-world streaming scenarios correctly.
| } else if (char !== "/" || this.currentTag.length > 0) { | ||
| this.currentTag += char | ||
| } else { | ||
| this.currentTag += char |
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.
This code appears to be duplicated. Could we simplify it to:
| this.currentTag += char | |
| } else { | |
| this.currentTag += char | |
| } |
| constructor( | ||
| private tagNames: string[], | ||
| private transform?: (chunks: XmlMatcherResult) => Result, | ||
| private position = 0, |
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.
Is the position parameter needed? It's accepted in the constructor but never used in the implementation, unlike the original XmlMatcher. If it's not needed, we could remove it to avoid confusion.
| @@ -0,0 +1,141 @@ | |||
| import { XmlMatcherResult } from "./xml-matcher" | |||
|
|
|||
| /** | |||
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.
Could we add JSDoc with usage examples? It would help developers understand how to use this with different tag combinations:
| /** | |
| /** | |
| * A multi-tag XML matcher that can match multiple tag names. | |
| * This is useful for handling different thinking tag formats from various models. | |
| * | |
| * @example | |
| * // Match both <think> and <thinking> tags | |
| * const matcher = new MultiTagXmlMatcher(['think', 'thinking']) | |
| * const results = matcher.update('<think>Hello</think> world <thinking>Hi</thinking>') | |
| */ |
|
Not a proper fix, closing for now |
This PR fixes the issue where LM Studio GPT-OSS models were showing raw model responses instead of properly formatted thinking blocks.
Problem
LM Studio GPT-OSS models use
<thinking>tags for their reasoning content, but the current implementation only looks for<think>tags. This causes the thinking blocks to not be parsed correctly, resulting in the raw response being displayed.Solution
MultiTagXmlMatcherutility that can handle multiple XML tag namesMultiTagXmlMatcherwith both["think", "thinking"]tagsTesting
MultiTagXmlMatchercovering various scenarios<think>and<thinking>tag parsingFixes #6750
Important
Adds support for
<thinking>tags in LM Studio GPT-OSS models by introducingMultiTagXmlMatcherand updatingLmStudioHandler.LmStudioHandlerinlm-studio.tsto useMultiTagXmlMatcherfor parsing both<think>and<thinking>tags.MultiTagXmlMatcherinmulti-tag-xml-matcher.tsto handle multiple XML tags.MultiTagXmlMatcherinmulti-tag-xml-matcher.spec.ts.lmstudio.spec.tsto test<think>and<thinking>tag handling inLmStudioHandler.This description was created by
for 924a793. You can customize this summary. It will automatically update as commits are pushed.