Skip to content

Revert PR #7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes #7594

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 8, 2025
Merged

Revert PR #7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes #7594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dfb30a
Revert "fix: omit temperature parameter when not explicitly set for O…
daniel-lxs Sep 2, 2025
a521eaf
fix: update tests to match reverted code signature
daniel-lxs Sep 2, 2025
7e6c8eb
fix: update provider tests to expect requestOptions parameter after r…
daniel-lxs Sep 6, 2025
cb25121
fix: update Chutes DeepSeek R1 test to match actual implementation
daniel-lxs Sep 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/api/providers/__tests__/chutes.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ describe("ChutesHandler", () => {
content: `${systemPrompt}\n${messages[0].content}`,
},
],
max_tokens: 32768,
temperature: 0.6,
stream: true,
stream_options: { include_usage: true },
}),
)
})
Expand Down Expand Up @@ -438,6 +442,7 @@ describe("ChutesHandler", () => {
expect.objectContaining({
model: modelId,
max_tokens: modelInfo.maxTokens,
temperature: 0.5,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
stream_options: { include_usage: true },
Expand Down
1 change: 1 addition & 0 deletions src/api/providers/__tests__/fireworks.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ describe("FireworksHandler", () => {
expect.objectContaining({
model: modelId,
max_tokens: modelInfo.maxTokens,
temperature: 0.5,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
stream_options: { include_usage: true },
Expand Down
79 changes: 1 addition & 78 deletions src/api/providers/__tests__/groq.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,7 @@ describe("GroqHandler", () => {
it("createMessage should pass correct parameters to Groq client", async () => {
const modelId: GroqModelId = "llama-3.1-8b-instant"
const modelInfo = groqModels[modelId]
const handlerWithModel = new GroqHandler({
apiModelId: modelId,
groqApiKey: "test-groq-api-key",
modelTemperature: 0.5, // Explicitly set temperature for this test
})
const handlerWithModel = new GroqHandler({ apiModelId: modelId, groqApiKey: "test-groq-api-key" })

mockCreate.mockImplementationOnce(() => {
return {
Expand Down Expand Up @@ -194,77 +190,4 @@ describe("GroqHandler", () => {
undefined,
)
})

it("should omit temperature when modelTemperature is undefined", async () => {
const modelId: GroqModelId = "llama-3.1-8b-instant"
const handlerWithoutTemp = new GroqHandler({
apiModelId: modelId,
groqApiKey: "test-groq-api-key",
// modelTemperature is not set
})

mockCreate.mockImplementationOnce(() => {
return {
[Symbol.asyncIterator]: () => ({
async next() {
return { done: true }
},
}),
}
})

const systemPrompt = "Test system prompt"
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Test message" }]

const messageGenerator = handlerWithoutTemp.createMessage(systemPrompt, messages)
await messageGenerator.next()

expect(mockCreate).toHaveBeenCalledWith(
expect.objectContaining({
model: modelId,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
}),
undefined,
)

// Verify temperature is NOT included
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs).not.toHaveProperty("temperature")
})

it("should include temperature when modelTemperature is explicitly set", async () => {
const modelId: GroqModelId = "llama-3.1-8b-instant"
const handlerWithTemp = new GroqHandler({
apiModelId: modelId,
groqApiKey: "test-groq-api-key",
modelTemperature: 0.7,
})

mockCreate.mockImplementationOnce(() => {
return {
[Symbol.asyncIterator]: () => ({
async next() {
return { done: true }
},
}),
}
})

const systemPrompt = "Test system prompt"
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Test message" }]

const messageGenerator = handlerWithTemp.createMessage(systemPrompt, messages)
await messageGenerator.next()

expect(mockCreate).toHaveBeenCalledWith(
expect.objectContaining({
model: modelId,
temperature: 0.7,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
}),
undefined,
)
})
})
Copy link

@roomote roomote bot Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup - removing the tests that validated omitting temperature. The remaining test correctly expects temperature to always be present.

67 changes: 1 addition & 66 deletions src/api/providers/__tests__/openai.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,71 +315,6 @@ describe("OpenAiHandler", () => {
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs.max_completion_tokens).toBe(4096)
})

it("should omit temperature when modelTemperature is undefined", async () => {
const optionsWithoutTemperature: ApiHandlerOptions = {
...mockOptions,
// modelTemperature is not set, should not include temperature
}
const handlerWithoutTemperature = new OpenAiHandler(optionsWithoutTemperature)
const stream = handlerWithoutTemperature.createMessage(systemPrompt, messages)
// Consume the stream to trigger the API call
for await (const _chunk of stream) {
}
// Assert the mockCreate was called without temperature
expect(mockCreate).toHaveBeenCalled()
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs).not.toHaveProperty("temperature")
})

it("should include temperature when modelTemperature is explicitly set to 0", async () => {
const optionsWithZeroTemperature: ApiHandlerOptions = {
...mockOptions,
modelTemperature: 0,
}
const handlerWithZeroTemperature = new OpenAiHandler(optionsWithZeroTemperature)
const stream = handlerWithZeroTemperature.createMessage(systemPrompt, messages)
// Consume the stream to trigger the API call
for await (const _chunk of stream) {
}
// Assert the mockCreate was called with temperature: 0
expect(mockCreate).toHaveBeenCalled()
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs.temperature).toBe(0)
})

it("should include temperature when modelTemperature is set to a non-zero value", async () => {
const optionsWithCustomTemperature: ApiHandlerOptions = {
...mockOptions,
modelTemperature: 0.7,
}
const handlerWithCustomTemperature = new OpenAiHandler(optionsWithCustomTemperature)
const stream = handlerWithCustomTemperature.createMessage(systemPrompt, messages)
// Consume the stream to trigger the API call
for await (const _chunk of stream) {
}
// Assert the mockCreate was called with temperature: 0.7
expect(mockCreate).toHaveBeenCalled()
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs.temperature).toBe(0.7)
})

it("should include DEEP_SEEK_DEFAULT_TEMPERATURE for deepseek-reasoner models when temperature is not set", async () => {
const deepseekOptions: ApiHandlerOptions = {
...mockOptions,
openAiModelId: "deepseek-reasoner",
// modelTemperature is not set
}
const deepseekHandler = new OpenAiHandler(deepseekOptions)
const stream = deepseekHandler.createMessage(systemPrompt, messages)
// Consume the stream to trigger the API call
for await (const _chunk of stream) {
}
// Assert the mockCreate was called with DEEP_SEEK_DEFAULT_TEMPERATURE (0.6)
expect(mockCreate).toHaveBeenCalled()
const callArgs = mockCreate.mock.calls[0][0]
expect(callArgs.temperature).toBe(0.6)
})
})

describe("error handling", () => {
Expand Down Expand Up @@ -515,7 +450,7 @@ describe("OpenAiHandler", () => {
],
stream: true,
stream_options: { include_usage: true },
// temperature should be omitted when not set
temperature: 0,
Copy link

@roomote roomote bot Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
{ path: "/models/chat/completions" },
)
Expand Down
6 changes: 3 additions & 3 deletions src/api/providers/__tests__/roo.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,16 +354,16 @@ describe("RooHandler", () => {
})

describe("temperature and model configuration", () => {
it("should omit temperature when not explicitly set", async () => {
it("should use default temperature of 0.7", async () => {
handler = new RooHandler(mockOptions)
const stream = handler.createMessage(systemPrompt, messages)
for await (const _chunk of stream) {
// Consume stream
}

expect(mockCreate).toHaveBeenCalledWith(
expect.not.objectContaining({
temperature: expect.anything(),
expect.objectContaining({
temperature: 0.7,
}),
undefined,
)
Expand Down
1 change: 1 addition & 0 deletions src/api/providers/__tests__/sambanova.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ describe("SambaNovaHandler", () => {
expect.objectContaining({
model: modelId,
max_tokens: modelInfo.maxTokens,
temperature: 0.7,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
stream_options: { include_usage: true },
Expand Down
1 change: 1 addition & 0 deletions src/api/providers/__tests__/zai.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ describe("ZAiHandler", () => {
expect.objectContaining({
model: modelId,
max_tokens: modelInfo.maxTokens,
temperature: ZAI_DEFAULT_TEMPERATURE,
messages: expect.arrayContaining([{ role: "system", content: systemPrompt }]),
stream: true,
stream_options: { include_usage: true },
Expand Down
8 changes: 3 additions & 5 deletions src/api/providers/base-openai-compatible-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,17 @@ export abstract class BaseOpenAiCompatibleProvider<ModelName extends string>
info: { maxTokens: max_tokens },
} = this.getModel()

const temperature = this.options.modelTemperature ?? this.defaultTemperature
Copy link

@roomote roomote bot Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 params: OpenAI.Chat.Completions.ChatCompletionCreateParamsStreaming = {
model,
max_tokens,
temperature,
messages: [{ role: "system", content: systemPrompt }, ...convertToOpenAiMessages(messages)],
stream: true,
stream_options: { include_usage: true },
}

// Only include temperature if explicitly set
if (this.options.modelTemperature !== undefined) {
params.temperature = this.options.modelTemperature
}

try {
return this.client.chat.completions.create(params, requestOptions)
} catch (error) {
Expand Down
9 changes: 1 addition & 8 deletions src/api/providers/openai.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,20 +159,13 @@ export class OpenAiHandler extends BaseProvider implements SingleCompletionHandl

const requestOptions: OpenAI.Chat.Completions.ChatCompletionCreateParamsStreaming = {
model: modelId,
temperature: this.options.modelTemperature ?? (deepseekReasoner ? DEEP_SEEK_DEFAULT_TEMPERATURE : 0),
Copy link

@roomote roomote bot Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

messages: convertedMessages,
stream: true as const,
...(isGrokXAI ? {} : { stream_options: { include_usage: true } }),
...(reasoning && reasoning),
}

// Only include temperature if explicitly set
if (this.options.modelTemperature !== undefined) {
requestOptions.temperature = this.options.modelTemperature
} else if (deepseekReasoner) {
// DeepSeek Reasoner has a specific default temperature
requestOptions.temperature = DEEP_SEEK_DEFAULT_TEMPERATURE
}

// Add max_tokens if needed
this.addMaxTokensIfNeeded(requestOptions, modelInfo)

Expand Down
Loading