Skip to content

feat: add self-correcting OpenAI sampling-param handling#101

Merged
vanloctech merged 2 commits into
vanloctech:mainfrom
thuytx03:fix/openai-param-retry-clarity
Jul 7, 2026
Merged

feat: add self-correcting OpenAI sampling-param handling#101
vanloctech merged 2 commits into
vanloctech:mainfrom
thuytx03:fix/openai-param-retry-clarity

Conversation

@thuytx03

@thuytx03 thuytx03 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Vấn đề

OpenAI đã tách Chat Completions thành hai thế hệ tham số sampling khác nhau:

  • Model cũ (gpt-3.5, gpt-4, gpt-4o, gpt-4.1, …) dùng temperature + max_tokens.
  • Model reasoning mới (o1/o3/o4/gpt-5 trở lên) dùng max_completion_tokenskhông cho phép override temperature.

Nếu gửi sai bộ tham số cho model, OpenAI sẽ trả về lỗi 400 và request thất bại. Việc cố gắng duy trì một danh sách đầy đủ tất cả model để biết dùng tham số nào là bất khả thi và dễ lỗi thời.

Giải pháp

PR này thêm cơ chế tự đoán rồi tự sửa để tính năng tóm tắt hoạt động ổn định trên cả hai thế hệ model, mà không cần bảo trì danh sách model dài dòng:

  1. apply_openai_sampling_params chọn bộ tham số "đoán tốt nhất" dựa trên một allowlist ngắn các dòng model cũ.
  2. Nếu OpenAI từ chối một tham số, post_openai_chat sẽ phân tích lỗi trả về — ưu tiên dùng trường có cấu trúc error.param thay vì đọc message dạng text — rồi sửa đúng trường bị lỗi ngay tại chỗ và thử lại.
  3. Số lần thử lại được giới hạn bởi MAX_OPENAI_PARAM_ADJUSTMENTS. Hằng số này đếm số lần điều chỉnh (không phải số request); trường hợp xấu nhất là số lần điều chỉnh + 1 request. Mọi lần gửi đều đi qua cùng một nhánh trong vòng lặp để tổng số request luôn rõ ràng.

Lý do thiết kế

  • Không hardcode danh sách model đầy đủ: các dòng model cũ về cơ bản đã "đóng băng", còn model mới của OpenAI đều đã thống nhất chuyển sang kiểu tham số mới. Vì vậy chỉ cần một allowlist ngắn cho model cũ, còn lại mặc định coi là model mới.
  • Tự sửa dựa trên lỗi thực tế của OpenAI: nếu đoán sai, chi phí chỉ là 1 request thừa rồi hệ thống tự điều chỉnh — an toàn hơn là cố đoán đúng 100%.

Cách kiểm thử

  • cargo check chạy sạch
  • Các unit test hiện có cho parser tương thích OpenAI vẫn pass
  • Thủ công: tóm tắt bằng một model cũ (vd gpt-4o) và một model reasoning (vd o3) để xác nhận cả hai kiểu tham số đều được chấp nhận

🤖 Được tạo với sự hỗ trợ của Claude Code

OpenAI split Chat Completions into two parameter generations: legacy
models use `temperature` + `max_tokens`, while newer reasoning models
(o1/o3/o4/gpt-5+) use `max_completion_tokens` and reject `temperature`
overrides.

Pick the best-guess parameter set per model family via a short legacy
allowlist, then auto-correct on rejection: `post_openai_chat` inspects
OpenAI's error (preferring the structured `error.param` field over the
message text) and adjusts the offending field in place, retrying up to
MAX_OPENAI_PARAM_ADJUSTMENTS times.

The constant counts corrections rather than requests, and every attempt
flows through a single loop path so the true request count (adjustments
+ 1) is clear.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@thuytx03 thuytx03 force-pushed the fix/openai-param-retry-clarity branch from 6ccdb5b to 7144215 Compare July 6, 2026 15:23
@vanloctech

Copy link
Copy Markdown
Owner

Thanks for the focused fix. The overall direction looks right: centralizing OpenAI sampling-parameter handling and using structured OpenAI error details is a good approach, and I don’t see a security issue in the bounded retry loop.

I’d like to request two small changes before merge:

  1. The same parameter mismatch can still happen through the Proxy provider. The app’s Proxy provider is OpenAI-compatible, defaults to https://api.openai.com, and also defaults to a GPT-5 model, but generate_raw_with_proxy() still sends temperature + max_tokens. Since the new helper already accepts a URL, please either reuse the same OpenAI-compatible retry path for Proxy or explicitly narrow the PR so Proxy is handled in a follow-up.

  2. Please add unit tests for the new logic. The current test suite still only covers the existing OpenAI-compatible response parser, not apply_openai_sampling_params(), adjust_openai_request(), or the retry adjustment cases. At minimum, I’d like tests for legacy models using temperature + max_tokens, reasoning/new models using max_completion_tokens without temperature, removing rejected temperature, and converting rejected max_tokens to max_completion_tokens.

I verified locally that cargo check, cargo test --lib, and bun run tsc -b pass, so this looks close. After those coverage/scope fixes, I’d be comfortable merging.

Address review feedback:

- Route the Proxy provider (OpenAI-compatible, defaults to api.openai.com
  with a GPT-5 model) through apply_openai_sampling_params + post_openai_chat,
  so it gets the same param selection and self-correcting retry as OpenAI
  instead of always sending temperature + max_tokens.
- Make adjust_openai_request swap the token-limit field in both directions:
  max_tokens -> max_completion_tokens (reasoning models) and
  max_completion_tokens -> max_tokens (proxies that only accept the legacy
  field), so the retry loop recovers regardless of which the endpoint rejects.
- Add unit tests for apply_openai_sampling_params (legacy vs reasoning models,
  unset token limit, case-insensitive detection) and adjust_openai_request
  (remove rejected temperature, convert max_tokens both ways, message-only
  fallback, no-op when field absent, combined adjustments).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@thuytx03

thuytx03 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Cảm ơn phản hồi chi tiết! Mình đã xử lý cả hai điểm trong commit mới:

1. Proxy provider dùng chung retry path OpenAI-compatible

generate_with_proxygenerate_raw_with_proxy giờ đi qua apply_openai_sampling_params + post_openai_chat, thay vì luôn gửi temperature + max_tokens. Vì Proxy mặc định trỏ api.openai.com với model GPT-5, giờ nó cũng chọn đúng bộ tham số và tự sửa khi bị reject.

Ngoài ra mình mở rộng adjust_openai_request để đổi trường token limit theo cả hai chiều:

  • max_tokensmax_completion_tokens (model reasoning của OpenAI)
  • max_completion_tokensmax_tokens (một số proxy OpenAI-compatible chỉ hiểu trường cũ)

Nhờ vậy vòng retry tự phục hồi dù endpoint từ chối trường nào.

2. Unit tests cho logic mới

Đã thêm test cho:

  • apply_openai_sampling_params: model legacy dùng temperature + max_tokens; model reasoning/mới dùng max_completion_tokens không kèm temperature; bỏ token limit khi không set; nhận diện legacy không phân biệt hoa/thường.
  • adjust_openai_request: gỡ temperature bị reject; chuyển max_tokensmax_completion_tokens cả hai chiều; fallback theo message khi không có error.param; không đổi gì khi trường đã vắng; điều chỉnh kết hợp nhiều trường.

Đã xác nhận local: cargo check, cargo test --lib (199 pass), và tsc -b đều xanh.

@vanloctech vanloctech merged commit ee5ed3e into vanloctech:main Jul 7, 2026
3 checks passed
anhtahaylove added a commit to anhtahaylove/youwee that referenced this pull request Jul 7, 2026
OpenAI and proxy summary requests now choose legacy max_tokens or reasoning max_completion_tokens based on the selected model, then retry when the API rejects temperature or token-limit fields.

Constraint: Port only upstream PR vanloctech#101 runtime handling and tests; do not pull broader long-video AI Summary flow or unrelated provider changes.

Rejected: Apply the new sampling map to every OpenAI-compatible provider | DeepSeek, Qwen, and LM Studio currently use their existing stable request shape and were outside PR vanloctech#101.

Rejected: Add a new user setting for parameter mode | automatic correction covers the observed API mismatch with less UI surface.

Confidence: high

Scope-risk: narrow

Directive: Keep OpenAI request compatibility in the provider layer so Summary callers do not branch on model families.

Tested: bun run biome check --write .

Tested: bun run tsc -b

Tested: cargo check

Tested: cargo test services::ai::providers::tests --lib

Tested: cargo test --lib
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