fix(server): update /v1/chat/completions endpoint to be backwards com…#1734
fix(server): update /v1/chat/completions endpoint to be backwards com…#1734christinaexyou wants to merge 1 commit intoNVIDIA-NeMo:developfrom
Conversation
Greptile SummaryThis PR adds a Pydantic
|
| Filename | Overview |
|---|---|
| nemoguardrails/server/schemas/openai.py | Adds a field_validator on model (mode="before") to coerce dict inputs into a string by extracting the id key; logic is correct but passes empty strings through silently. |
| tests/server/test_openai_integration.py | Adds a happy-path integration test for the dict-model input; correctly uses raw TestClient to bypass SDK string coercion, but the error branch of the validator is not exercised. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["POST /v1/chat/completions\n(raw JSON body)"] --> B["Pydantic: OpenAIChatCompletionRequest\nfield_validator 'model' mode=before"]
B --> C{Is model a dict?}
C -- Yes --> D{"Has string 'id' field?"}
D -- No --> E["raise ValueError\n422 Unprocessable Entity"]
D -- Yes --> F["return v['id'] (string)"]
C -- No --> G["return v as-is\n(Pydantic validates str)"]
F --> H["model: str set on request"]
G --> H
H --> I["GuardrailsChatCompletionRequest\n→ LLM pipeline"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: nemoguardrails/server/schemas/openai.py
Line: 61-64
Comment:
**Empty string `id` silently accepted**
The guard `not isinstance(v.get("id"), str)` passes when `id` is `""` (an empty string). An empty model identifier would propagate downstream and likely produce a confusing runtime failure (e.g., an empty model name being sent to the LLM provider) rather than a clean 422 validation error at the boundary.
Consider tightening the check to require a non-empty string:
```suggestion
if not isinstance(v.get("id"), str) or not v["id"]:
raise ValueError("Model object must contain a non-empty string 'id' field")
return v["id"]
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 9d024bc
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| @field_validator("model", mode="before") | ||
| @classmethod | ||
| def normalize_model(cls, v: Any) -> Optional[str]: |
There was a problem hiding this comment.
Optional[str] return type annotation
The return type is annotated as Optional[str], but the model field is declared as a required str (not Optional[str]). If the validator ever returns None (e.g., when an input of null is passed), Pydantic will raise its own type error rather than treating it as a valid value. The Optional[str] annotation therefore misleads readers into thinking None is a valid return value from this validator, when in practice it is not.
| def normalize_model(cls, v: Any) -> Optional[str]: | |
| def normalize_model(cls, v: Any) -> str: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/server/schemas/openai.py
Line: 60
Comment:
**Misleading `Optional[str]` return type annotation**
The return type is annotated as `Optional[str]`, but the `model` field is declared as a required `str` (not `Optional[str]`). If the validator ever returns `None` (e.g., when an input of `null` is passed), Pydantic will raise its own type error rather than treating it as a valid value. The `Optional[str]` annotation therefore misleads readers into thinking `None` is a valid return value from this validator, when in practice it is not.
```suggestion
def normalize_model(cls, v: Any) -> str:
```
How can I resolve this? If you propose a fix, please make it concise.| def test_chat_completion_model_as_dict(): | ||
| test_client = TestClient(api.app) | ||
| response = test_client.post( | ||
| "/v1/chat/completions", | ||
| json={ | ||
| "model": {"id": "gpt-4o", "name": "gpt-4o", "maxLength": 10000}, | ||
| "messages": [{"role": "user", "content": "hi"}], | ||
| "guardrails": {"config_id": "with_custom_llm"}, | ||
| }, | ||
| ) | ||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert data["model"] == "gpt-4o" | ||
| assert data["choices"][0]["message"]["content"] == "Custom LLM response" | ||
| assert data["choices"][0]["finish_reason"] == "stop" |
There was a problem hiding this comment.
The new test only covers the happy path (a dict with a valid string id). The validator also has an explicit error branch — raise ValueError("Model object must contain a string 'id' field") — that is triggered when id is absent or is not a string. Consider adding tests for:
- A model dict where
idis missing entirely, e.g.{"model": {"name": "gpt-4o"}}. - A model dict where
idis a non-string type, e.g.{"model": {"id": 42}}.
Both cases should return a 422 Unprocessable Entity response. Without these tests the error path is untested and could silently regress.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/server/test_openai_integration.py
Line: 89-103
Comment:
**Missing error-case test coverage for the new validator**
The new test only covers the happy path (a dict with a valid string `id`). The validator also has an explicit error branch — `raise ValueError("Model object must contain a string 'id' field")` — that is triggered when `id` is absent or is not a string. Consider adding tests for:
- A model dict where `id` is missing entirely, e.g. `{"model": {"name": "gpt-4o"}}`.
- A model dict where `id` is a non-string type, e.g. `{"model": {"id": 42}}`.
Both cases should return a `422 Unprocessable Entity` response. Without these tests the error path is untested and could silently regress.
How can I resolve this? If you propose a fix, please make it concise.
📝 WalkthroughWalkthroughAdds a field validator to accept the model parameter in OpenAI chat completion requests as either a string or a dictionary with an id key, resolving compatibility with the web UI. Includes a corresponding test case validating dictionary-format model handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/server/test_openai_integration.py (1)
89-103: Add a malformed-model negative test to pin validator behavior.This new test covers the happy path well; adding one 422 case for invalid dict shape would make the compatibility contract much safer against regressions.
✅ Suggested test addition
+def test_chat_completion_model_as_dict_invalid_id(): + test_client = TestClient(api.app) + response = test_client.post( + "/v1/chat/completions", + json={ + "model": {"name": "gpt-4o"}, # missing string "id" + "messages": [{"role": "user", "content": "hi"}], + "guardrails": {"config_id": "with_custom_llm"}, + }, + ) + assert response.status_code == 422🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/test_openai_integration.py` around lines 89 - 103, Add a negative test alongside test_chat_completion_model_as_dict that posts to "/v1/chat/completions" with an invalid model dict shape (e.g., missing required keys or wrong types) and assert the server returns a 422 status; locate the test near test_chat_completion_model_as_dict in tests/server/test_openai_integration.py, call the same TestClient(api.app), send a malformed "model" payload, and assert response.status_code == 422 and that the response body contains validation error details to pin expected validator behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/server/test_openai_integration.py`:
- Around line 89-103: Add a negative test alongside
test_chat_completion_model_as_dict that posts to "/v1/chat/completions" with an
invalid model dict shape (e.g., missing required keys or wrong types) and assert
the server returns a 422 status; locate the test near
test_chat_completion_model_as_dict in tests/server/test_openai_integration.py,
call the same TestClient(api.app), send a malformed "model" payload, and assert
response.status_code == 422 and that the response body contains validation error
details to pin expected validator behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e340869-0bc5-4b40-85ef-31d0a6061e8b
📒 Files selected for processing (2)
nemoguardrails/server/schemas/openai.pytests/server/test_openai_integration.py
christinaexyou
force-pushed
the
fix-chat-completions
branch
from
c167f3a to
9d024bc
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
thank you @christinaexyou , the issue is deeper. If you run nemoguardrails server you can see the UI is not responsive. It does not crash but does not work. Let's see if we want to adapt to this existing UI or come up with a new one. |
|
ack thanks @Pouyanpi, I'll look into this and we can discuss further during our sync. |
2 participants
…patabile
Description
This PR adds a field validator to the OpenAIChatCompletionRequest schema to convert models from objects to strings, ensuring backward compatibility of the web UI.
Related Issue(s)
Fixes #1728
Checklist
Summary by CodeRabbit
New Features