feat(thinking): add tag selection API with include/exclude filtering (#1790) - #1795
feat(thinking): add tag selection API with include/exclude filtering (#1790)#1795Abtiotm wants to merge 1 commit into
Conversation
f1b6883 to
77c9c4c
Compare
|
@Abtiotm - thank you for your contribution! Could you please comment on one inquiry in the issue? But what IF a user-created prompt with reasoning includes tag A, but some-tag=tag B? Thank you. |
|
|
||
| override fun contribution(): String { | ||
| return buildString { | ||
| append("You MUST generate reasoning inside <$tag>...</$tag> tags ") |
There was a problem hiding this comment.
Should it be a system prompt?
|
And what is the proper interpretation of and also needs to be added to the respective asciidoc (.adoc) in the thinking section. |
| } | ||
| } | ||
|
|
||
| override fun promptContribution(): PromptContribution { |
There was a problem hiding this comment.
is this override needed?
promptContribution() duplicates the default impl (PromptContributor.kt:85-91), except it forces role = "thinking_tag_instruction". But that role is never read for this contributor.
may be just you just to provide location and role data via overriding those properties: override val role = "thinking_tag_instruction". ?
or maybe the custom role is not need as It's never read for this contributor: the only callers of promptContribution() (OperationContext.kt:98, ActionContext.kt:169) run at PromptRunner build time, while the injector is appended later via withPromptContributors
| get() = PromptContributionLocation.END | ||
|
|
||
| override fun contribution(): String { | ||
| return buildString { |
There was a problem hiding this comment.
agree with @igordayen what is user mispell tag ?
tag is interpolated raw ("<$tag>...</$tag>"). "", "my tag", "a><b" all produce a malformed instruction. Add a require(...) in init matching [a-zA-Z][a-zA-Z0-9_-]*
so the failure surfaces at the call site instead of as a silent empty result.
and the injection side accepts anything the extraction side can't read:
""→<></>, meaningless instruction"my tag"→ model complies, closing tag fails the regex →thinkingBlockscomes back empty, no error, after a paid LLM call (diificult to debug for user)"a><b"→ arbitrary markup injected into the system prompt
| val result = operations.createObjectIfPossible(messages, outputClass) | ||
|
|
||
| // Then: Should have called withPromptContributors with ThinkingTagInjector | ||
| verify { mockDelegate.withPromptContributors(any()) } |
There was a problem hiding this comment.
the test doesn't check what its name promises
verify { mockDelegate.withPromptContributors(any()) } any() accepts anything. Change ThinkingTagInjector(thinkingTag) to ThinkingTagInjector("think") at DelegatingThinking.kt:59 - the caller's tag is silently dropped and this test still passes
The assertEquals(expectedResponse, result) lines don't help either: expectedResponse is what the test told the mock to return.
you need to capture the argument instead so we are sure that we have the good tag at the end
| delegateForTag().evaluateConditionWithThinking(condition, context, confidenceThreshold) | ||
|
|
||
| private fun delegateForTag(): PromptExecutionDelegate { | ||
| if (thinkingTag == null) return delegate |
There was a problem hiding this comment.
this answer @igordayen question Bare thinking() injects nothing; only thinking(tag) adds the instruction.
Good call - backward compatible , so update the doc , otherwise people need to read the source code to know how it works
also : PromptRunner.kt:502
fun thinking(tag: String): Thinking = thinking() While updating the docs, PromptRunner.kt:502 (thinking(tag) = thinking()) could use a line noting that implementations overriding thinking() should override this too.
|
Regarding the tag A / tag B case: If the user prompt requests tag A while We are intentionally not adding prompt conflict detection in this PR, as reliably inferring tag intent from arbitrary natural-language prompts is outside its scope. |
|
@Abtiotm - could you please comment on the comments and mark "resolved" as needed? Thank you |
|
@Abtiotm @igordayen I think this is mixing different types of thinking/reasoning. Please see my comment on the original issue (#1790) why I think this is a bad idea and how I think this type of application level reasoning/motivation should be implemented. |
|
@Abtiotm To evaluate the solution: Could you please plug in the example I posted on the issue itself and validate the output? Test 1: Instead of my system prompt, use yours, auto-injected. As we learned from philosophy: experiment is the criterion of truth:) - Pierre Duhem. Thank you. |
|
Thanks @jorander, @igordayen, and @azanux for the feedback. I agree that native/internal model thinking and application-level reasoning or motivation are different concepts. The intention of this PR is not to expose or redefine native model thinking. It only adds an opt-in prompt instruction for the tagged, visible reasoning blocks that the existing However, I understand the concern that naming this API Before making further implementation changes, could the maintainers please confirm which direction is preferred?
If option 1 is accepted, I will address the current review comments by:
I will avoid expanding the API further until the intended semantic boundary is confirmed. |
|
@Abtiotm - please see the discussion on the issue. If I'm reading the discussion correctly, it's largely pointing to Option 1. |
|
@Abtiotm is PR intended for Embabel 1.5.0 (Boot 4.1 / Spring AI 2.0.0) or Embabel 1.0.1 (Boot 3.5, Spring AI 1.1.x) |
|
@Abtiotm - could you please check the issue? We reached consensus. Thank you |
|
@Abtiotm - there is not much activity on PR recently. What is your timeline? thanks |
…mbabel#1790) Add ThinkingTagSelection with include/exclude filtering for XML-style thinking blocks. Non-TAG blocks (prefix/untagged) are always retained. Missing tags trigger a WARN log to make silent hasThinking()==false diagnosable. thinking(tag) convenience shorthand for single-tag extraction.
77c9c4c to
7f68fca
Compare
|
@Abtiotm - thanks for moving this forward. I just realized that there would be yet another option to express the same, namely through: It would be good to have consistency in API usage. @jorander @azanux @arnabnandy7 Another approach - to keep the current design "as-is" but internally convert it into LLMOptions. You may not need to change "create"-API signatures if you convert to LLMOptions.Thinking. Thank you |
Summary
Add tag selection to thinking extraction:
thinking(include, exclude)selects which XML-style tags are extracted as reasoning blocks, per the consensus reached in #1790. Previously every XML tag was dynamically detected, so ordinary markup like<div>...</div>could surface as a thinking block.Changes
thinking(tag)is a shorthand forthinking(include = setOf(tag)). Callingthinking()without arguments preserves default dynamic detectionhasThinking() == falsediagnosableUsage
Tag selection does not modify the prompt — the LLM only produces reasoning inside
<tag>...</tag>if the prompt asks for it.Testing
Added/updated tests in DelegatingThinkingTest (tag selection delegation + filtering), LlmInteractionSerializationTest (ThinkingTagSelection serialization + missing-tag warning), ChatClientLlmOperationsThinkingTest, DelegatingStreamingPromptRunnerTest. All pass.
Closes #1790