bug(openai): support structured chat content metadata for OpenAI backend endpoint#947
bug(openai): support structured chat content metadata for OpenAI backend endpoint#947Prasannajaga wants to merge 1 commit into
Conversation
…tions (vllm-project#940) Models like `google/translategemma-12b-it` require structured text content objects with extra metadata fields (e.g., `source_lang_code` and `target_lang_code`) in `/v1/chat/completions` requests. Without these fields, server-side Jinja chat templates fail with HTTP 400 errors. This change adds support for appending custom metadata fields into structured chat content objects: - Added `content_extra_fields` to `OpenAIHTTPBackendArgs` and forwarded it to `ChatCompletionsRequestHandler._format_prompts()`. - Added the generic `--append-payloads` CLI option to pass key-value metadata. - Enabled preservation of dictionary prompt objects from custom dataset rows in `text_column`. - Updated `GenerativeRequestFinalizer.finalize_turn()` to safely extract string content from dictionary prompts when computing character and word usage metrics. - Added comprehensive unit test coverage under `tests/unit/backends/openai/` and `tests/unit/data/`. Resolves vllm-project#940 Signed-off-by: prasanna <prasannajaga9@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fabc22c84
ℹ️ 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".
| continue | ||
|
|
||
| input_metrics.add_text_metrics(text) | ||
| input_metrics.add_text_metrics(self._extract_prompt_text(prompt)) |
There was a problem hiding this comment.
Keep structured prompt objects out of non-chat backends
When the dataset prompt is a structured dict and the selected backend is not the OpenAI chat handler (for example vllm_python), this only extracts the nested text for metrics while the original dict remains in GenerationRequest.columns. The VLLM backend formats text columns with str(item) at src/guidellm/backends/vllm_python/vllm.py:388, so those runs silently benchmark the dict literal instead of the prompt text; the structured-content handling needs to be scoped to the chat handler or the column value normalized before other backends see it.
AGENTS.md reference: AGENTS.md:L84-L85
Useful? React with 👍 / 👎.
dbutenhof
left a comment
There was a problem hiding this comment.
I'm still evaluating some of the changes, but I'm going to put out a few high level comments first.
| @click.option( | ||
| "--append-payloads", | ||
| callback=_parse_append_payloads, | ||
| help=( | ||
| "Append key-value fields to structured content objects in chat completion " | ||
| "requests. Values override matching metadata from custom datasets. " | ||
| 'Example: `--append-payloads \'{"key":"value"}\'`' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
You've already defined append_payloads as a backend args parameter, so it should be passed as part of `--backend '{"kind":"openai_http","request_format":"/v1/chat/completions","append_payloads":{"source_lang_id":"en","target_lang_id":"es"}}'
We don't need another global CLI option for this: so this option and the helper function should be removed.
There was a problem hiding this comment.
curious to know though we have this inside , isnt it better if we give the global CLI option here for the better UX ?
There was a problem hiding this comment.
For one thing, it's not a "global CLI option" because it only applies to the chat completions and responses request APIs on the openai_http backend. One of the motives behind the "kind" discriminated models was to better collect related parameters (e.g., the HTTP backends have api_key and target, but the in-process backends don't).
The main advantage of breaking it out here is that with multi-level options it can be tricky to specify the discriminated option without going to quoted serialized JSON, which is awkward and error-prone to type, whereas the broken-out version can be flatter and easier to use in the simplified k=v syntax. But we'd rather not add global CLI syntax for options that aren't global ... it's messier, harder to document, and harder to support.
And this actually works, without serialized JSON ... 😁
$ guidellm run --backend kind=openai_http,extras.content.source_lang_code=en,extras.content.target_lang_code=es| if self.request_format != "/v1/chat/completions": | ||
| raise ValueError( | ||
| "append_payloads is only supported with the " | ||
| "'/v1/chat/completions' request format" |
There was a problem hiding this comment.
Only chat completions? Not, for example, responses?
There was a problem hiding this comment.
sorry I missed out! added /responses as well
| append_payloads = kwargs.pop("append_payloads", None) | ||
| if append_payloads is not None: | ||
| spec = kwargs.setdefault("spec", {}) | ||
| backend = spec.setdefault("backend", {}) | ||
| backend.setdefault("kind", "openai_http") | ||
| backend["append_payloads"] = append_payloads |
There was a problem hiding this comment.
Remove this, too -- no new handling is required at the CLI level.
| return {"type": tool.get("type", "function"), "function": fn} | ||
| return tool | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
Ah, I see what I was missing in my first pass.
This is a simple solution for a constructed dataset like the example in your documentation -- but it requires a single dataset field with a JSON structure containing the exact fields needed for the translategemma model: text, source_lang_code, target_lang_code. Maybe that's a first step, although I'm not thrilled about the special-case interpretation code here in the request handler. (We have some work underway to handle JSON columns in datasets more generally, at the data/mapper layers.)
My bigger concern here is that you're counting on a fairly specialized JSON format to supply the content type/text/language-keys. There are, for example, several huggingface datasets with names that seem targeted to google/translategemma models and none of them have this format. We'll need to be able to map from separate columns to these content keys.
The reason GuideLLM supports column mapping (--data-column-mapper) is that dataset keys vary. Again, in the few translategemma datasets on huggingface, one use "src" and "tgt" for the from-/to- language keys, while another uses "language" and "target_language"; we need to allow for such variations.
I think it's fine to have a "first level" PR that just adds a backend parameter like your "append_payloads" to provide static data for those fields, and we (or you) can add full dataset support with column mapping later -- but this is a special case "halfway hack" that I'd rather avoid.
There was a problem hiding this comment.
Thanks for explaining this! I want to make sure I'm following the right approach.
I was a bit confused about how you'd prefer to structure this
- should I go ahead and address the full dataset column mapping (--data-column-mapper) as part of this PR ? or
- would you prefer keeping this PR strictly focused on static backend payload overrides (extras.content) and leaving --append-fields & dataset column mapping for a separate PR ?
Let me know how you'd like me to proceed!
There was a problem hiding this comment.
I was a bit confused about how you'd prefer to structure this
Yeah, I have mixed thoughts on this one.
- should I go ahead and address the full dataset column mapping (--data-column-mapper) as part of this PR ? or
I'd rather keep this relatively simple and skip a "full solution" that involves new dataset column mapping. So I'd say that's a "no".
- would you prefer keeping this PR strictly focused on static backend payload overrides (extras.content) and leaving --append-fields & dataset column mapping for a separate PR ?
- The minimal change to support the TranslateGemma model is introducing the source/target language codes, and the
extras.contentmodel does that relatively straightforwardly. - Your JSON dataset prompt field decoding works for a custom-made JSONL data source, but isn't compatible with the TranslateGemma datasets I found on Huggingface; and while nested JSON fields seem to be increasingly common in some other areas (our team is dealing with this in a more general way for trace replay use cases), I worry that the way it's inserted here is perhaps too special case to be supportable.
Let me know how you'd like me to proceed!
I think my preference at this point would be to stick with the extras.content static injection for this PR, and then expand it later with real dataset column mapping support.
| default=None, | ||
| description="Additional parameters to include in generation requests.", | ||
| ) | ||
| append_payloads: dict[str, Any] | None = Field( |
There was a problem hiding this comment.
"Append payloads" sounds far too generic. Maybe something like "extra_content" would be better. In fact a much cleaner option would be to put this under the existing "extras" parameter. Right now that uses the GenerationRequestArguments object, which allows setting headers, params, body, and adding files ... perhaps a content field with your new semantics would fit nicely in that model, with k/v pairs that would be added to the 'content' in each applicable request.
guidellm run --backend '{
"kind":"openai_http",
"target":"<url>",
"extras": {
"content": {
"metadata": {"category": "support"},
"priority": 1
}
}(I already have some concerns with the fact that a lot of OpenAI HTTP-specific classes & fields are defined globally in schemas as if they were universal, but we haven't gotten around to worrying about that and this isn't the right context. For now, nobody else would use this anyway.)
There was a problem hiding this comment.
makes complete sense! ill update the implementation to use extras.content!
Summary
GuideLLM right now doesn't support structured chat content objects for models like
google/translategemma-12b-itthat require additional metadata fields (such assource_lang_codeandtarget_lang_code) in/v1/chat/completionsrequests. Without these fields, Jinja chat templates on inference servers fail with HTTP 400 errors ('dict object' has no attribute ...).This PR details the design and implementation for supporting extra metadata fields directly in chat content objects as mentioned in issue #940. It provides:
--append-payloadsoption (and backend argumentcontent_extra_fields) to specify extra fields to inject into chat completion requests for any model or endpoint.text_columnso dynamic, per-prompt metadata can be passed.Applied Changes
[MODIFY] http.py
content_extra_fields: dict[str, Any] | NonetoOpenAIHTTPBackendArgs.content_extra_fieldsfrom backend configuration torequest_handler.format(...)in_prepare_resolve_request.[MODIFY] request_handlers.py
ChatCompletionsRequestHandler.format()and_format_prompts()to injectcontent_extra_fieldsinto text content objects.text_column.[MODIFY] run.py
--append-payloadsCLI option to append key-value fields to structured content objects in chat completion requests.[MODIFY] generative.py
GenerativeRequestFinalizer.finalize_turn()to ensure word and character metric calculation works for dictionary inputs.[NEW] Unit Tests
tests/unit/backends/openai/test_request_handlers.py: Added tests verifyingcontent_extra_fieldsinjection and dictionary item preservation in chat completions format.tests/unit/backends/openai/test_http.py: Added tests verifying backend argument handling and request resolution.tests/unit/data/test_finalizers.py: Added tests forGenerativeRequestFinalizerwith dictionary prompt items.Alternatives Considered:
(Empty)
Test Verified
tox -e test-unit,tox -e lint-check, andtox -e type-check. All tests passed.Related Issues
Use of AI
git log
commit 7fabc22
Author: prasanna prasannajaga9@gmail.com
Date: Thu Jul 23 00:44:32 2026 +0530
Signed-off-by: prasanna prasannajaga9@gmail.com