[chatgpt] Add configurable request timeout#20408
Conversation
lsiepel
left a comment
There was a problem hiding this comment.
Thanks for providing this patch.
Besides these comments, could youalso add the parameter to the readme.md ?
|
|
||
| public int maxTokens = 500; | ||
|
|
||
| public int requestTimeout = 0; |
There was a problem hiding this comment.
i would not expect this on the channelconfiguration as the PR describes this is on a per thing base.
There was a problem hiding this comment.
From the PR description:
with an optional per-channel override.
The thing specifies the default value for all channels, but a channel can have its own.
There was a problem hiding this comment.
Ah, yes i see now. I would opt to make this a @Nullable and not default to 0
...binding.chatgpt/src/main/java/org/openhab/binding/chatgpt/internal/ChatGPTConfiguration.java
Outdated
Show resolved
Hide resolved
| scheduler.execute(() -> { | ||
| try { | ||
| Request request = httpClient.newRequest(modelUrl).timeout(REQUEST_TIMEOUT_MS, TimeUnit.MILLISECONDS) | ||
| Request request = httpClient.newRequest(modelUrl).timeout(DEFAULT_REQUEST_TIMEOUT_S, TimeUnit.SECONDS) |
There was a problem hiding this comment.
This does not respect the timeout set to the thing configuration, but uses the hard coded default.
There was a problem hiding this comment.
This is the one-time request for /v1/models which should complete in milliseconds. If you want, I can of course change this, but the thing-level timeout is primarily for the HLI completions.
There was a problem hiding this comment.
Yes i understand, With the provided value it is fine, but it mixes the default for a user configuration parameter and a setting that can be seen as more infrastructural related. Anyway, choose what you want, i have no strong opinion on this, just want to make sure you know.
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/i18n/chatgpt_de.properties
Show resolved
Hide resolved
bundles/org.openhab.binding.chatgpt/src/main/resources/OH-INF/i18n/chatgpt_fr.properties
Show resolved
Hide resolved
There was a problem hiding this comment.
file needs to be regenerated after other comments have been addressed.
| Request request = httpClient.newRequest(apiUrl).method(HttpMethod.POST) | ||
| .timeout(REQUEST_TIMEOUT_MS, TimeUnit.MILLISECONDS).header("Content-Type", "application/json") | ||
| .header("Authorization", "Bearer " + apiKey).content(new StringContentProvider(queryJson)); | ||
| return sendPrompt(queryJson, config != null ? config.requestTimeout : DEFAULT_REQUEST_TIMEOUT_S); |
There was a problem hiding this comment.
In the case requesttimeout is 0, this will gives issues
There was a problem hiding this comment.
thing-types.xml specifies min="1".
There was a problem hiding this comment.
Yes, but that does not enforce the value to be min="1" as we have file based / unmanaged configurations that can have any value, so a sanity check is needed.
There was a problem hiding this comment.
What's the pattern then? Throw an exception or use the default value and log a warning?
There was a problem hiding this comment.
Usually a binding verifies in the initialize method if all configuration parameters are valid. If not, the thing state is set accordingly.
...enhab.binding.chatgpt/src/main/java/org/openhab/binding/chatgpt/internal/ChatGPTHandler.java
Show resolved
Hide resolved
| <label>Request Timeout</label> | ||
| <description>Timeout in seconds for this channel. Overrides the thing-level timeout. Set to 0 to use the thing | ||
| default.</description> | ||
| <default>0</default> |
There was a problem hiding this comment.
| <default>0</default> | |
| <default>10</default> |
It should match the default set in the configuration.
There was a problem hiding this comment.
The channel default is "0", i.e. to use the thing-level timeout value.
There was a problem hiding this comment.
here i would remove the default tag to not set any value and leave it null
| <default>1</default> | ||
| <advanced>true</advanced> | ||
| </parameter> | ||
| <parameter name="requestTimeout" type="integer" min="0"> |
There was a problem hiding this comment.
A zero time-out makes no sense, or in many cases it is used as a signal to not set any timeout.
| <parameter name="requestTimeout" type="integer" min="0"> | |
| <parameter name="requestTimeout" type="integer" min="1"> |
There was a problem hiding this comment.
That's exactly the case, as detailed in the <description>:
Set to 0 to use the thing default.
There was a problem hiding this comment.
Not setting a timeout leaves it to the library, here it is set to the default time out. This is not a common pattern in openHAB and i would opt to reconsider this behavior.
There was a problem hiding this comment.
This is a consequence of using "0" as the value to mean "use config from thing", the http library is always called with an explicit timeout. But as discussed above I will change this to be nullable and then the minimum value can be 1.
The previously hardcoded 10 second timeout is now configurable as a thing-level parameter (requestTimeout) with an optional per-channel override. A channel timeout of 0 falls back to the thing default. This allows longer timeouts for local LLMs (e.g. ollama) or reasoning models that require more processing time. Signed-off-by: Andreas Brenk <mail@andreasbrenk.com>
|
I've removed the translations and added the requestTimeout to |
The previously hardcoded 10 second timeout is now configurable as a thing-level parameter (requestTimeout) with an optional per-channel override. A channel timeout of 0 falls back to the thing default. This allows longer timeouts for local LLMs (e.g. ollama) or reasoning models that require more processing time.
An example would be a rule that generates a weather report based on sensor data.
Fixes #20203 (although already closed, but without any changes).
A cherry-pick to the 5.1.x branch would be nice.