Skip to content

fix temperature error.#27

Merged
sayakpaul merged 4 commits into
mainfrom
fix-deprecated-temperature-anthropic
Jul 1, 2026
Merged

fix temperature error.#27
sayakpaul merged 4 commits into
mainfrom
fix-deprecated-temperature-anthropic

Conversation

@sayakpaul

Copy link
Copy Markdown
Member

Fix:

› Preparing PR checkout…
› Preparing PR checkout…
› Checkout ready in 1.1s (/tmp/reviewbot-clones/worktrees/huggingface__diffusers__13986__f89611e581e049f1bef4377c616da2ae)
› Starting review of huggingface/diffusers#13986
› Fetched PR with 18 changed file(s)
› Calling LLM…
› LLM turn (blind=0/30)
✗ LLM endpoint returned 400 Bad Request: {"error":{"code":"invalid_request_error","message":"`temperature` is deprecated for this model.","type":"invalid_request_error","param":null}}

@sayakpaul sayakpaul requested a review from tarekziade June 24, 2026 04:56
@tarekziade

Copy link
Copy Markdown
Collaborator

@askserge review this

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤗 Serge says:

This PR correctly adds graceful fallback when an LLM endpoint rejects the temperature parameter (e.g., newer Claude models that deprecate it). The implementation introduces a new _TemperatureUnsupported exception, a provider-neutral heuristic _is_temperature_rejection to detect such400s, and refactors the retry loop in complete() to strip the offending field and retry once. The tests cover both the happy-path fallback and the important negative case where a value-range error does not trigger the fallback. No issues found.

serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 3 LLM turns · 2 tool calls · 37.8s · 22080 in / 7375 out tokens

@tarekziade

Copy link
Copy Markdown
Collaborator

@askserge the fix is sound, but can we make the fix more generic. e.g. capture and fallback for any unsupported argument

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤗 Serge says:

The fix correctly detects the temperature deprecation 400 and retries without the parameter, which resolves the reported failure. The new tests cover both the fallback path and the case where a value-range error must not be silently swallowed.

Suggestion (non-blocking): As the human reviewer noted, the current implementation is hardcoded to temperature. Consider generalizing the exception and the checker so any unsupported payload argument can be caught and removed without adding new exception types and except blocks for each field. A single _ParameterUnsupported(param) exception and a parameterized _is_parameter_rejection(param, body_preview) check would make the while True loop in complete() naturally extensible for future deprecations (e.g. max_tokens, top_p, etc.).

serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 5 LLM turns · 4 tool calls · 87.9s · 42433 in / 24891 out tokens

Comment thread reviewbot/llm_client.py Outdated
response body preview for logging."""


class _TemperatureUnsupported(Exception):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fix is sound for the immediate issue, but the exception is hardcoded to temperature. To make this generic for any unsupported argument as requested, consider a single _ParameterUnsupported(param: str) exception (and a matching _is_parameter_rejection(param, body_preview) helper) so the while True loop in complete() can catch the generic exception and pop whatever parameter it carries, avoiding new exception types and handler blocks for each new deprecated field.

@tarekziade tarekziade left a comment

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.

Let's try a more generic approach, as explained by Serge

@sayakpaul

Copy link
Copy Markdown
Member Author

@tarekziade could you check again?

@sayakpaul sayakpaul requested a review from tarekziade July 1, 2026 10:45
@tarekziade

Copy link
Copy Markdown
Collaborator

@askserge check this PR, it adds graceful fallback when an LLM endpoint rejects a parameter that is not supported. The first use case is the temperature parameter.

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤗 Serge says:

This PR adds graceful fallback when an LLM endpoint rejects specific sampling parameters (starting with temperature) with a 400 error. The implementation is clean and well-tested.

What changed:

  • New _ParameterUnsupported exception and _is_parameter_rejection() heuristic to detect parameter-level400s (as opposed to value-range errors).
  • _REMOVABLE_PARAMS tuple defines sampling parameters that are safe to drop and retry without.
  • complete() now uses a while True loop to strip rejected parameters one at a time and retry.
  • _post_with_retries() checks for parameter rejections before falling back to the existing tools-unsupported path.

Assessment:

  • The retry logic correctly avoids infinite loops: each iteration pops the offending parameter, and guards prevent re-raising for parameters already removed.
  • Tests cover the happy path (temperature rejected → retry succeeds), value-error discrimination (should not retry), and generic parameter fallback (top_p).
  • The heuristic matching in _is_parameter_rejection is pragmatically provider-neutral; worst-case false positives waste one retry but still surface the real error.

No blocking issues found.

serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 3 LLM turns · 4 tool calls · 32.4s · 30808 in / 6534 out tokens

@sayakpaul sayakpaul merged commit 0df2cb1 into main Jul 1, 2026
2 checks passed
@sayakpaul sayakpaul deleted the fix-deprecated-temperature-anthropic branch July 1, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants