Skip to content

fix(v1): forward sampling parameters across dialects - #2425

Open
xeophon wants to merge 3 commits into
mainfrom
fix/forward-sampling-params
Open

fix(v1): forward sampling parameters across dialects#2425
xeophon wants to merge 3 commits into
mainfrom
fix/forward-sampling-params

Conversation

@xeophon

@xeophon xeophon commented Aug 23, 2026

Copy link
Copy Markdown
Member

Overview

Forward eval sampling parameters consistently across Chat Completions, Responses, Anthropic Messages, and training requests.

Details

  • Flatten extra_body before building provider wire arguments, with explicit sampling fields taking precedence.
  • Preserve provider-specific sampling fields while retaining protocol-specific max-token and reasoning mappings.
  • Record service_tier in effective sampling metadata.
  • Avoid conflicting Chat Completions max-token aliases when an eval supplies the override.

Note

Medium Risk
Changes how sampling overrides are merged into provider requests, which can alter generation (max tokens, reasoning, extra_body). Logic is localized to dialect override paths, not auth or data handling.

Overview
Eval sampling now reaches Chat Completions, Responses, Anthropic Messages, and training requests through a shared SamplingConfig.wire_args() helper. That flattens OpenAI-style extra_body into top-level wire args (typed knobs win), so extra provider fields are no longer dropped.

Dialect apply_overrides paths merge those flattened args instead of a small hardcoded subset, while still remapping protocol-specific keys (max_tokensmax_output_tokens, reasoning_effort into reasoning/output_config). Chat Completions drops conflicting max_tokens/max_completion_tokens aliases when the eval supplies either. service_tier is recorded in each dialect’s sampling metadata.

Reviewed by Cursor Bugbot for commit b4d847b. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Forward sampling parameters across v1 dialects via wire_args

  • Adds SamplingConfig.wire_args() to flatten non-None fields, lift extra_body values, and resolve max_tokens vs max_completion_tokens conflicts.
  • Updates TrainClient and all dialects (Anthropic, Chat, Responses) to use wire_args() instead of manual model_dump logic.
  • Adds service_tier to sampling_fields for all dialects.
  • Improves dialect-specific handling: merges output_config/reasoning_effort for Anthropic and Responses, and maps max_tokens to max_output_tokens for Responses.
  • Behavioral Change: AnthropicDialect now preserves body temperature and top_p unless explicitly overridden; ChatDialect drops both max_tokens and max_completion_tokens from the body when any max token override is provided.

Macroscope summarized b4d847b.

Comment thread verifiers/v1/dialects/chat.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02fd7a62d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread verifiers/v1/dialects/chat.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ad95f52. Configure here.

Comment thread verifiers/v1/types.py
@macroscopeapp

macroscopeapp Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes effective generation requests across three provider dialects and the training renderer by forwarding arbitrary sampling fields, flattening extra_body, and remapping token and reasoning settings. These cross-path runtime changes can materially alter model output and parameter precedence, warranting human review.

You can add or adjust custom eligibility rules. Learn more.

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.

1 participant