mattcasters commented on code in PR #8840:
URL: https://github.com/apache/hop/pull/8840#discussion_r4237451492
##########
plugins/tech/ai/src/main/java/org/apache/hop/ai/metadata/AiProvider.java:
##########
@@ -359,6 +387,11 @@ public String test(IVariables variables) throws
HopException {
return AiChatFactory.healthCheck(this, variables);
}
+ /** The Thinking settings, by the label the editor shows. */
+ public List<String> getThinkingChoices(ILogChannel log, IHopMetadataProvider
metadataProvider) {
+ return List.of(AiThinking.descriptions());
Review Comment:
**[nit]** `getThinkingChoices` fills the combo with localized descriptions,
and `GuiCompositeWidgets` stores that text on the String field.
`AiThinking.lookup` accepts the label or the code. The English labels
(`Default`, `Off`, `On`) match the codes ignoring case, so a saved provider
works today. A translation of `AiThinking.OFF` would be what the editor writes.
On a runtime whose descriptions differ, lookup returns null and
`AiProviderSettings.think` leaves `think` unset, so Thinking Off silently stays
on. The transform side does not have this problem: `OllamaThink` is stored by
code.
**Suggestion:** Keep the String field so a variable such as
`'${AI_THINKING}'` still works, but store the code (`OFF`, `ON`, `DEFAULT`) for
a recognized choice. Lookup can keep accepting both the code and the
description.
##########
plugins/transforms/languagemodelchat/src/main/java/org/apache/hop/pipeline/transforms/languagemodelchat/internals/LanguageModelFacade.java:
##########
@@ -199,6 +200,9 @@ private ChatModel createOllamaModel() {
.numPredict(numPredict)
.numCtx(numCtx)
.responseFormat(toResponseFormat(format))
+ // Null leaves think unset, so the model decides. The reasoning never
reaches the output.
Review Comment:
**[suggestion]** The comment here, the same comment in
`AiChatModelFactory.ollamaModel`, the AI provider tooltip, and both manual
pages say the reasoning is never written to an output field no matter how
Thinking is set. That holds for Off and On, not for Default. In
langchain4j-ollama 1.20 a null `think` (Default, and every provider or
transform saved before this option) makes a reasoning model prepend
`<think>...</think>` to the response content. Hop writes `aiMessage().text()`
and does not read `AiMessage.thinking()`. `returnThinking(false)` is already
the Ollama builder default and only suppresses that separate field, so it does
not remove those tags. Default therefore still puts the reasoning in the
output, which is the old behavior, but the new text says otherwise.
**Suggestion:** Say that Off and On do not put reasoning in the answer text,
and that Default leaves it to the model, which can include `<think>` tags in
the answer. Keep Default from stripping tags unless changing existing output is
intentional.
##########
integration-tests/ai/0006-structured-extract-thinking-off.hpl:
##########
@@ -0,0 +1,245 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+-->
+<pipeline>
+ <info>
+ <name>0006-structured-extract-thinking-off</name>
+ <name_sync_with_filename>Y</name_sync_with_filename>
+ <description>Structured extract with a thinking model, through an AI
provider whose Thinking
+ option is Off. This is the path Extract graph, Structured extract and
the AI Assistant build
+ their model on. A thinking model held to a JSON schema must still return
both required
+ fields typed and non null with think set to false.</description>
Review Comment:
**[suggestion]** The pipeline description and
`integration-tests/ai/README.md` say this test checks that Structured extract
runs with `think` set to false. The only assertions are that `severity` and
`customer_impact` are not null. A qwen3 answer that is still reasoning can pass
that: with `think` true, Ollama puts the JSON in `content` and the reasoning in
a separate field, and Hop only reads `content`. `0005-thinking` does observe
the flag, but only on Language Model Chat's inline option (token counts).
`AiChatModelFactoryTest` checks the provider body for `"think":false`, but only
via `chat(String)`, not a `ChatRequest` that also sets a JSON schema, which is
the request Structured extract actually sends.
**Suggestion:** Assert something that fails when the provider flag is
dropped on that request, for example output tokens under the same ceiling as
`0005`, or a unit test that chats with a response format and still sees
`"think":false` in the body.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
