Repository navigation
[None][fix] Chat Completions: accept the OpenAI store field - #20864
Conversation
ChatCompletionRequest forbids unknown fields, so a request that sets the standard OpenAI `store` field failed validation with HTTP 400. The pi coding agent sends `store: false` on every Chat Completions request by default, so each of its turns was rejected before reaching the model. Accept the field and leave it unused, like `user` and `parallel_tool_calls`: the server keeps no copy of a chat completion, so there is nothing to store. Other unknown fields are still rejected. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Request field and API stability reference tensorrt_llm/serve/openai_protocol.py, tests/unittest/api_stability/references/trtllm_serve_api.yaml |
ChatCompletionRequest adds an optional store boolean field. The API stability reference lists it as stable, optional, and null by default. |
Request validation and CPU test registration tests/unittest/llmapi/apps/test_openai_protocol_chat_store.py, tests/integration/test_lists/test-db/l0_cpu.yml |
Tests cover accepted values, omission, JSON parsing, invalid inputs, and model-dump behavior. The CPU integration test list includes the new test module. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Merge Risk: 
Minimal · up to 611ee
-
No actionable merge-blocking risk is identified in the reviewed change.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passedFull details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
/bot run --disable-fail-fast |
|
PR_Github #80073 [ run ] triggered by Bot. Commit: |
|
PR_Github #80073 [ run ] completed with state
|
Picks up the waive of unittest/executor/test_forward_error_survival.py (NVIDIA#20895, nvbug 6981910), which fails on this branch's base in the CPU stages independently of this change. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #80156 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
This handles store the same way as the existing accepted-but-ignored fields (parallel_tool_calls, user), and extra="forbid" still rejects other unknown keys, which test_unknown_fields_remain_rejected checks.
Minor points:
- The title uses
[None]for a user-visible bug fix (requests from a common client returned HTTP 400). If an NVBug or JIRA ticket tracks the pi-agent compatibility work, please cite it. storeis a no-op here, but it has real meaning in the Responses API (ResponsesRequest.storedefaults toTrueand controls persistence whenenable_storeis set). The code comment already says chat completions are never stored. A short note in the serve docs listing which OpenAI chat fields are accepted but ignored would help users who expectstore: trueto make a completion retrievable. This is optional.
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
PR_Github #80156 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #80216 [ run ] triggered by Bot. Commit: |
|
PR_Github #80216 [ run ] completed with state |
… rejects The request schema is not strict, so Pydantic coerces boolean-like strings and integers for store. A test now records that, and the rejection test is renamed for what it checks: a value that is not boolean-like. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
@brnguyen2 Thanks for the review. The inline point about lax coercion is addressed in fd33783. On the two body points:
|
|
/bot run --disable-fail-fast |
|
PR_Github #80244 [ run ] triggered by Bot. Commit: |
|
PR_Github #80244 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #80290 [ run ] triggered by Bot. Commit: |
|
PR_Github #80290 [ run ] completed with state |
Resolve tests/integration/test_lists/test-db/l0_cpu.yml: keep both new entries (main's test_openai_chat_multimodal_prompt.py and this branch's test_openai_protocol_chat_store.py). Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #80314 [ run ] triggered by Bot. Commit: |
|
PR_Github #80314 [ run ] completed with state
|
Picks up the waive of unittest/_torch/modeling_v2/models/glm_moe_dsa/test_routing.py (NVIDIA#20935, nvbug 6986044), which the previous main merge brought in failing on both CPU stages independently of this change. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #80349 [ run ] triggered by Bot. Commit: |
|
PR_Github #80349 [ run ] completed with state
|
Picks up main's waives of the LoRA tests that NVIDIA#20774 broke (test_lora_ownership.py and the test_resource_manager.py PEFT reload tests, Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com> NVIDIA#20953), which fail independently of this change.
|
/bot run --disable-fail-fast |
|
PR_Github #80434 [ run ] triggered by Bot. Commit: |
|
PR_Github #80434 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #80458 [ run ] triggered by Bot. Commit: |
|
PR_Github #80458 [ run ] completed with state |
Dev Engineer Review
ChatCompletionRequestnow acceptsstore: Optional[bool] = Nonefor OpenAI compatibility. The field is ignored because the server does not retain chat completions.QA Engineer Review
tests/unittest/llmapi/apps/test_openai_protocol_chat_store.pyand registered it in the CPU CI list.Nonedefault, JSON parsing, boolean-like coercion, invalid values, unchanged other fields, and rejection of unknown fields.Per-File QA Perspective
tensorrt_llm/serve/openai_protocol.py: Verify that chat completion requests accept optionalstore, default it toNone, and do not change completion storage behavior.tests/unittest/llmapi/apps/test_openai_protocol_chat_store.py: Covers field validation, JSON parsing, coercion, and rejection behavior. The file is listed intest-db/l0_cpu.yml.tests/integration/test_lists/test-db/l0_cpu.yml: Registers the new unit test for CPU CI.tests/unittest/api_stability/references/trtllm_serve_api.yaml: Recordsstorein the API stability reference. Verify that the reference remains aligned with the request model.Description
ChatCompletionRequestforbids unknown fields (extra="forbid"), so a request that sets OpenAI's standardstorefield fails validation with HTTP 400. The pi coding agent sendsstore: falseon every Chat Completions request unless its model config setscompat.supportsStore: false, so with default settings every pi turn againsttrtllm-serveis rejected before it reaches the model.This PR accepts
storeasOptional[bool] = Noneand leaves it unused, likeuserandparallel_tool_calls: the server keeps no copy of a chat completion, so there is nothing to store. Other unknown fields are still rejected.tensorrt_llm/serve/openai_protocol.py: the field, where the OpenAI reference orders it.tests/unittest/api_stability/references/trtllm_serve_api.yaml: its reference row (kind: openai,status: stable).#20368 adds
prompt_cache_keythe same way; the two PRs do not touch the same lines.Test Coverage
tests/unittest/llmapi/apps/test_openai_protocol_chat_store.py(CPU only), registered inl0_cpu.yml:storeaccepted as a keyword and from JSON, defaultNone, non-boolean values rejected, other fields unchanged by it, unknown fields still rejected. On main, 6 of its 8 tests fail; the 2 that pass are the negative cases, which hold there too.tests/unittest/api_stability/test_serve_api.pycovers the reference row.store: 400 on main, 200 with the change.PR Checklist
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.🤖 Generated with Claude Code