Skip to content

Refactor openai capabilities#80

Merged
knjiang merged 2 commits intomainfrom
ken/add-capabilities-to-apply-temp
Feb 5, 2026
Merged

Refactor openai capabilities#80
knjiang merged 2 commits intomainfrom
ken/add-capabilities-to-apply-temp

Conversation

@knjiang
Copy link
Contributor

@knjiang knjiang commented Feb 5, 2026

Deleted a bunch of dead code and added support for stripping temperature.

@knjiang knjiang requested a review from remh February 5, 2026 02:35
// Force translation if model needs any transforms (temperature stripping, max_tokens conversion, etc.)
let request_model = payload.get("model").and_then(Value::as_str).or(model);
request_model
.map(|m| !model_supports_max_tokens(m))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically we force translation in order for "capabilities" to apply

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for context i added -> deb50d6

Comment on lines 235 to -240

// Pass temperature through for non-reasoning models
// Reasoning models (o1-*, o3-*, gpt-5-*) don't support temperature
if !is_reasoning_model(model) {
insert_opt_f64(&mut obj, "temperature", req.params.temperature);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we already supported this for responses api - im using the same abstraction instead apply_model_transforms for both responses/chat completions since they use same model family

ForceMaxCompletionTokens => {
// (Responses API) max_output_tokens is valid.
if obj.contains_key("max_output_tokens") {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be continue instead of return ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some unit tests to the functions here

@knjiang knjiang requested a review from remh February 5, 2026 03:37
@knjiang knjiang merged commit 0f18f3e into main Feb 5, 2026
5 checks passed
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