-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: add support for removing thinking tags from input text in BeanOutputConverter #4667
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
base: main
Are you sure you want to change the base?
Conversation
…utputConverter Signed-off-by: liugddx <liugddx@gmail.com>
Signed-off-by: liugddx <liugddx@gmail.com>
Thanks @liugddx for your contribution on this issue! Just a heads-up: there’s already an open PR (#3927) addressing the same topic. I tested the changes proposed here, but unfortunately, they didn’t work as expected for the qwen3:14b model running locally on my Ollama instance. Could you take a look at this comment, which suggests a slightly different approach? It would be great to find a solution that works for both qwen3 and nova models. |
…abilities Signed-off-by: liugddx <liugddx@gmail.com>
…abilities Signed-off-by: liugddx <liugddx@gmail.com>
Thank you for your reply. I have already made it compatible with Qwen. If possible, please give it another try. |
* This allows for a flexible pipeline of text cleaning operations. | ||
* | ||
* @author liugddx | ||
* @since 1.0.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.
It will be probably 1.1.0. To be confirmed.
private static ResponseTextCleaner createDefaultTextCleaner() { | ||
return CompositeResponseTextCleaner.builder() | ||
.addCleaner(new WhitespaceCleaner()) | ||
.addCleaner(new ThinkingTagCleaner()) |
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 am not sure that ThinkingTagCleaner
should be added by default, especially for the non-thinking models.
|
||
String result = text; | ||
for (Pattern pattern : this.patterns) { | ||
result = pattern.matcher(result).replaceAll(""); |
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.
You could consider breaking once the replacements have been done.
I have performed new tests with your recent modifications and it works well. Well done! I have added a few review comments. As a contributor myself, I think it would be good to check with a member of this project to ensure it aligns with the project’s long-term goals. Let me know if you’d like to discuss any of my comments! |
Close #4650
Thank you for taking time to contribute this pull request!
You might have already read the contributor guide, but as a reminder, please make sure to:
git commit -s
) per the DCOmain
branch and squash your commitsFor more details, please check the contributor guide.
Thank you upfront!