Revert PR #7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes - #7594
Conversation
dosubot
Bot
added
size:L
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thank you for this clean revert! I've reviewed the changes and this effectively addresses the TabbyApi/ExLlamaV2 crash issue reported in #7581. The approach of doing a pure revert rather than adding new functionality is the right choice for a critical bug fix.
| info: { maxTokens: max_tokens }, | ||
| } = this.getModel() | ||
|
|
||
| const temperature = this.options.modelTemperature ?? this.defaultTemperature |
There was a problem hiding this comment.
Good restoration of the temperature parameter. This ensures it's always included with a fallback to the default temperature (0) when not explicitly set, preventing the TypeError in TabbyApi/ExLlamaV2.
Consider adding a comment here explaining why temperature must always be included to prevent future developers from attempting similar optimizations:
|
|
||
| const requestOptions: OpenAI.Chat.Completions.ChatCompletionCreateParamsStreaming = { | ||
| model: modelId, | ||
| temperature: this.options.modelTemperature ?? (deepseekReasoner ? DEEP_SEEK_DEFAULT_TEMPERATURE : 0), |
There was a problem hiding this comment.
The restoration here correctly handles the special case for DeepSeek Reasoner models while ensuring temperature is always included. This maintains compatibility with all OpenAI-compatible endpoints.
| undefined, | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Good cleanup - removing the tests that validated omitting temperature. The remaining test correctly expects temperature to always be present.
| stream: true, | ||
| stream_options: { include_usage: true }, | ||
| // temperature should be omitted when not set | ||
| temperature: 0, |
There was a problem hiding this comment.
Is it intentional that the temperature here is 0 instead of a default like 0.5 or 0.7? Just want to confirm this aligns with the expected default behavior for OpenAI providers when temperature isn't explicitly set.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Review
and removed
Issue/PR - Triage
The requestOptions parameter was added after PR #7188, so tests need to be updated to not expect it after the revert
2e05f51 to
a521eaf
Compare
…evert After reverting PR #7188, the createStream method in base-openai-compatible-provider.ts calls client.chat.completions.create() with two parameters: params and requestOptions. The tests need to be updated to expect this second parameter (which is often undefined).
github-project-automation
Bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
3 participants
Summary
This is a clean revert of PR #7188 (commit 090737c) which introduced crashes in TabbyApi/ExLlamaV2 by omitting the temperature parameter when not explicitly set.
Why This Revert?
PR #7188 made the temperature parameter optional, omitting it when not explicitly set. This caused crashes in certain backends like TabbyApi/ExLlamaV2 that expect the temperature parameter to always be present.
What This Does
This PR performs a pure of the problematic commit, restoring the original behavior where:
Difference from PR #7582
PR #7582 attempted to fix the same issue but added new functionality (provider-specific default temperatures) that wasn't in the original code. This PR is a clean revert without any additional changes.
Testing
After this revert:
Fixes #7581
Important
Reverts commit 090737c to always include temperature in API calls, fixing crashes in TabbyApi/ExLlamaV2.
base-openai-compatible-provider.tsandopenai.tsto always include temperature in API calls.This description was created by
for cb25121. You can customize this summary. It will automatically update as commits are pushed.