Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,8 @@ describe('DeepSeekOpenAICompatibleProvider', () => {
});

describe('getDefaultGenerationConfig', () => {
it('returns temperature 0', () => {
expect(provider.getDefaultGenerationConfig()).toEqual({
temperature: 0,
});
it('does not force a deterministic temperature by default', () => {
expect(provider.getDefaultGenerationConfig()).toEqual({});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,7 @@ export class DeepSeekOpenAICompatibleProvider extends DefaultOpenAICompatiblePro
}

override getDefaultGenerationConfig(): GenerateContentConfig {
return {
temperature: 0,
};
return {};
}
Comment on lines 138 to 140

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] This override now returns {} — byte-for-byte identical to what the base class DefaultOpenAICompatibleProvider.getDefaultGenerationConfig() already returns (provider/default.ts:219), so inheritance yields identical behavior and the override — along with its dedicated test, which now asserts the base behavior as if it were DeepSeek-specific — is redundant; five of the seven subclasses already inherit {} without overriding. The cost is maintenance, not runtime: the override and test read as if DeepSeek carried a distinct default generation config, and if the base default ever gains a real value, DeepSeek silently opts out here and diverges from every other subclass with no visible diff — re-introducing the exact bug class this PR fixes. Either delete the override, its test block, and the now-unused GenerateContentConfig import, or — if the override deliberately pins {} as a regression guard for #9765 — say that why in a one-line comment, which is currently invisible:

override getDefaultGenerationConfig(): GenerateContentConfig {
  // Deliberately empty: forcing temperature 0 caused repetitive thinking
  // loops on DeepSeek reasoning models (#9765).
  return {};
}
中文说明

[Suggestion] 这个 override 现在返回 {}——与基类 DefaultOpenAICompatibleProvider.getDefaultGenerationConfig() 已经返回的内容(provider/default.ts:219)逐字节完全相同,因此继承即可得到完全一致的行为;该 override 连同它的专属测试(现在把基类行为当作 DeepSeek 特有行为来断言)都是冗余的——七个子类中已有五个不做 override、直接继承 {}。代价在维护层面而非运行时:这个 override 和测试读起来像是 DeepSeek 拥有独立的默认 generation config;如果基类默认值将来加入了真实的值,DeepSeek 会通过这里悄悄退出继承、在没有任何可见 diff 的情况下与其他所有子类产生差异——重新引入本 PR 所修复的那类 bug。可以删除该 override、对应的测试代码块以及因此不再使用的 GenerateContentConfig import;或者——如果这个 override 是有意将 {} 固定下来、作为 #9765 的回归防护——请用一行注释说明这个原因,目前这一点完全看不出来。

— qwen3.8-max via Qwen Code /review (v0.22.0)

}

Expand Down
Loading