Repository navigation
fix(frontend): accept safety_identifier, prompt_cache_retention, verbosity in chat #17508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,15 @@ pub const PASSTHROUGH_EXTRA_FIELDS: &[&str] = &[ | |
| "vllm_xargs", | ||
| ]; | ||
|
|
||
| /// OpenAI Chat Completions fields that are accepted and dropped. They configure | ||
| /// OpenAI's moderation, prompt cache retention, and response verbosity, which | ||
| /// Dynamo does not implement. | ||
| pub const CHAT_IGNORED_FIELDS: &[&str] = | ||
| &["prompt_cache_retention", "safety_identifier", "verbosity"]; | ||
|
|
||
| /// Maximum `safety_identifier` length in the OpenAI request contract. | ||
| const MAX_SAFETY_IDENTIFIER_LENGTH: usize = 128; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Chat Completions contract limits 🤖 AI FixChange the Chat Completions safety identifier limit to 64 characters and add boundary coverage for lengths 64 and 65.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 128 is right for chat. The current OpenAI spec puts Follow-up (later PR) |
||
|
|
||
| /// `vllm_xargs` keys vLLM itself fills from dedicated request fields. Dynamo only | ||
| /// forwards router-generated `kv_transfer_params`, so clients may not set them. | ||
| const RESERVED_VLLM_XARGS_KEYS: &[&str] = &["kv_transfer_params", "ec_transfer_params"]; | ||
|
|
@@ -158,17 +167,35 @@ pub fn validate_no_unsupported_fields( | |
| ) -> Result<(), anyhow::Error> { | ||
| validate_no_unsupported_fields_with_ignore( | ||
| unsupported_fields, | ||
| &[], | ||
| *IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS, | ||
| ) | ||
| } | ||
|
|
||
| /// Chat Completions version of [`validate_no_unsupported_fields`] that also | ||
| /// accepts [`CHAT_IGNORED_FIELDS`] after checking their values. | ||
| pub fn validate_no_unsupported_chat_fields( | ||
| unsupported_fields: &std::collections::HashMap<String, serde_json::Value>, | ||
| ) -> Result<(), anyhow::Error> { | ||
| validate_no_unsupported_fields_with_ignore( | ||
| unsupported_fields, | ||
| CHAT_IGNORED_FIELDS, | ||
| *IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS, | ||
| )?; | ||
| validate_chat_ignored_fields(unsupported_fields) | ||
| } | ||
|
|
||
| fn validate_no_unsupported_fields_with_ignore( | ||
| unsupported_fields: &std::collections::HashMap<String, serde_json::Value>, | ||
| accepted_fields: &[&str], | ||
| ignore_unsupported_fields: bool, | ||
| ) -> Result<(), anyhow::Error> { | ||
| let unknown: Vec<_> = unsupported_fields | ||
| .keys() | ||
| .filter(|k| !PASSTHROUGH_EXTRA_FIELDS.contains(&k.as_str())) | ||
| .filter(|k| { | ||
| !PASSTHROUGH_EXTRA_FIELDS.contains(&k.as_str()) | ||
| && !accepted_fields.contains(&k.as_str()) | ||
| }) | ||
| .map(|s| format!("`{}`", s)) | ||
| .collect(); | ||
| if !unknown.is_empty() && !ignore_unsupported_fields { | ||
|
|
@@ -214,6 +241,34 @@ fn validate_no_unsupported_fields_with_ignore( | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Checks [`CHAT_IGNORED_FIELDS`] values against the OpenAI request contract. | ||
| fn validate_chat_ignored_fields( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up idea: |
||
| fields: &std::collections::HashMap<String, serde_json::Value>, | ||
| ) -> Result<(), anyhow::Error> { | ||
| if let Some(value) = fields.get("safety_identifier") | ||
| && !value.is_null() | ||
| && value | ||
| .as_str() | ||
| .is_none_or(|id| id.chars().count() > MAX_SAFETY_IDENTIFIER_LENGTH) | ||
| { | ||
| anyhow::bail!( | ||
| "`safety_identifier` must be a string of at most {MAX_SAFETY_IDENTIFIER_LENGTH} characters" | ||
| ); | ||
| } | ||
| for (field, allowed) in [ | ||
| ("prompt_cache_retention", &["in_memory", "24h"][..]), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. openai-python 2.8 to 2.32 types this field as |
||
| ("verbosity", &["low", "medium", "high"][..]), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ] { | ||
| if let Some(value) = fields.get(field) | ||
| && !value.is_null() | ||
| && !value.as_str().is_some_and(|v| allowed.contains(&v)) | ||
| { | ||
| anyhow::bail!("`{field}` must be one of: {}", allowed.join(", ")); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Validates `vllm_xargs` against vLLM's OpenAI schema: an object whose values are | ||
| /// strings, numbers or booleans, or arrays of those. | ||
| fn validate_vllm_xargs(value: &serde_json::Value) -> Result<(), anyhow::Error> { | ||
|
|
@@ -1379,7 +1434,7 @@ mod tests { | |
| "vllm_xargs": {"flag": true}, | ||
| })) | ||
| .unwrap(); | ||
| validate_no_unsupported_fields_with_ignore(&fields.fields, false).unwrap(); | ||
| validate_no_unsupported_fields_with_ignore(&fields.fields, &[], false).unwrap(); | ||
| assert_eq!(fields.fields.len(), PASSTHROUGH_EXTRA_FIELDS.len()); | ||
| assert_eq!(fields.fields["detokenize"], json!(false)); | ||
| assert_eq!(fields.fields["stop_token_ids"], json!([])); | ||
|
|
@@ -1397,22 +1452,22 @@ mod tests { | |
| let fields: ExtraFields = serde_json::from_value(json!({field: bad})).unwrap(); | ||
| assert_eq!(fields.fields[field], bad); | ||
| let err = | ||
| validate_no_unsupported_fields_with_ignore(&fields.fields, false).unwrap_err(); | ||
| validate_no_unsupported_fields_with_ignore(&fields.fields, &[], false).unwrap_err(); | ||
| assert!(err.to_string().contains(field), "{err}"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_no_unsupported_fields_accepts_logprob_token_ids() { | ||
| let fields = HashMap::from([("logprob_token_ids".to_string(), json!([14, 15]))]); | ||
| validate_no_unsupported_fields_with_ignore(&fields, false).unwrap(); | ||
| validate_no_unsupported_fields_with_ignore(&fields, &[], false).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_no_unsupported_fields_rejects_malformed_logprob_token_ids() { | ||
| for bad in [json!(["notanint"]), json!(7), json!([[1, 2]]), json!([-1])] { | ||
| let fields = HashMap::from([("logprob_token_ids".to_string(), bad)]); | ||
| let err = validate_no_unsupported_fields_with_ignore(&fields, false).unwrap_err(); | ||
| let err = validate_no_unsupported_fields_with_ignore(&fields, &[], false).unwrap_err(); | ||
| assert!(err.to_string().contains("must be an array of token IDs")); | ||
| } | ||
| } | ||
|
|
@@ -1436,7 +1491,7 @@ mod tests { | |
| "tag": "read", | ||
| }), | ||
| )]); | ||
| validate_no_unsupported_fields_with_ignore(&fields, false).unwrap(); | ||
| validate_no_unsupported_fields_with_ignore(&fields, &[], false).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
|
|
@@ -1457,20 +1512,21 @@ mod tests { | |
| ), | ||
| ] { | ||
| let fields = HashMap::from([("vllm_xargs".to_string(), bad)]); | ||
| let err = validate_no_unsupported_fields_with_ignore(&fields, false).unwrap_err(); | ||
| let err = validate_no_unsupported_fields_with_ignore(&fields, &[], false).unwrap_err(); | ||
| assert!(err.to_string().contains(message), "{err}"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_no_unsupported_fields_rejects_unknown_fields_by_default() { | ||
| let err = validate_no_unsupported_fields_with_ignore(&unknown_fields(), false).unwrap_err(); | ||
| let err = | ||
| validate_no_unsupported_fields_with_ignore(&unknown_fields(), &[], false).unwrap_err(); | ||
| assert!(err.to_string().contains("Unsupported parameter(s)")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_no_unsupported_fields_ignores_unknown_fields_when_configured() { | ||
| validate_no_unsupported_fields_with_ignore(&unknown_fields(), true).unwrap(); | ||
| validate_no_unsupported_fields_with_ignore(&unknown_fields(), &[], true).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
|
|
@@ -1481,10 +1537,53 @@ mod tests { | |
| ]); | ||
|
|
||
| let err = | ||
| validate_no_unsupported_fields_with_ignore(&unsupported_fields, true).unwrap_err(); | ||
| validate_no_unsupported_fields_with_ignore(&unsupported_fields, &[], true).unwrap_err(); | ||
| assert!(err.to_string().contains("stop_token_ids")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn chat_ignored_fields_are_accepted_only_by_chat() { | ||
| let fields = HashMap::from([ | ||
| ( | ||
| "safety_identifier".to_string(), | ||
| json!("é".repeat(MAX_SAFETY_IDENTIFIER_LENGTH)), | ||
| ), | ||
| ("prompt_cache_retention".to_string(), json!("24h")), | ||
| ("verbosity".to_string(), json!("low")), | ||
| ]); | ||
| validate_no_unsupported_fields_with_ignore(&fields, CHAT_IGNORED_FIELDS, false).unwrap(); | ||
| validate_chat_ignored_fields(&fields).unwrap(); | ||
| let err = validate_no_unsupported_fields_with_ignore(&fields, &[], false).unwrap_err(); | ||
| assert!( | ||
| err.to_string().contains("Unsupported parameter(s)"), | ||
| "{err}" | ||
| ); | ||
|
|
||
| let nulls = CHAT_IGNORED_FIELDS | ||
| .iter() | ||
| .map(|field| (field.to_string(), serde_json::Value::Null)) | ||
| .collect::<HashMap<_, _>>(); | ||
| validate_chat_ignored_fields(&nulls).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn chat_ignored_fields_reject_malformed_values() { | ||
| for (field, bad) in [ | ||
| ("safety_identifier", json!(7)), | ||
| ( | ||
| "safety_identifier", | ||
| json!("x".repeat(MAX_SAFETY_IDENTIFIER_LENGTH + 1)), | ||
| ), | ||
| ("prompt_cache_retention", json!("1h")), | ||
| ("verbosity", json!("loud")), | ||
| ("verbosity", json!(["low"])), | ||
| ] { | ||
| let fields = HashMap::from([(field.to_string(), bad)]); | ||
| let err = validate_chat_ignored_fields(&fields).unwrap_err(); | ||
| assert!(err.to_string().contains(field), "{err}"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_top_p_rejects_zero() { | ||
| let err = validate_top_p(Some(0.0)).unwrap_err(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1220,6 +1220,61 @@ async fn responses_auto_tool_choice_without_tools_reaches_backend() { | |
| .await; | ||
| } | ||
|
|
||
| /// OpenAI's `safety_identifier`, `prompt_cache_retention`, and `verbosity` reach the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment only narrates what the test name and assertions already show: chat requests succeed without forwarding these fields, while legacy completions reject them. It preserves no additional constraint and should be removed. 🤖 AI FixRemove this redundant test doc comment. |
||
| /// backend on chat completions without being forwarded. Legacy completions, whose | ||
| /// contract has no such fields, still rejects them. | ||
| #[tokio::test] | ||
| #[serial] | ||
| async fn chat_completions_accept_and_drop_openai_ignored_fields() { | ||
| const FIELDS: [(&str, &str); 3] = [ | ||
| ("safety_identifier", "user-1234"), | ||
| ("prompt_cache_retention", "24h"), | ||
| ("verbosity", "low"), | ||
| ]; | ||
| fn with_fields(mut body: Value) -> Value { | ||
| for (field, value) in FIELDS { | ||
| body[field] = json!(value); | ||
| } | ||
| body | ||
| } | ||
|
|
||
| temp_env::async_with_vars(BASE_ENV, async { | ||
| let script = load_agent_fixture("text.sse").await.unwrap(); | ||
| let svc = HarnessService::start(vec![script; 2]).await; | ||
|
|
||
| for stream in [false, true] { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 🤖 AI FixReplace the stream loop with one |
||
| let body = with_fields(json!({ | ||
| "model": MODEL, | ||
| "stream": stream, | ||
| "messages": [{"role": "user", "content": "ping"}], | ||
| })); | ||
| let response = post_json(&svc, "/v1/chat/completions", body).await; | ||
| assert_eq!(response.status(), reqwest::StatusCode::OK); | ||
| bounded(response.bytes()).await.unwrap(); | ||
|
|
||
| let requests = svc.engine.take_requests().await; | ||
| assert_eq!(requests.len(), 1); | ||
| let forwarded = serde_json::to_value(&requests[0]).unwrap(); | ||
| for (field, _) in FIELDS { | ||
| assert!(forwarded.get(field).is_none(), "{field} was forwarded"); | ||
| } | ||
| } | ||
|
|
||
| let body = with_fields(json!({"model": MODEL, "prompt": "ping"})); | ||
| let response = post_json(&svc, "/v1/completions", body).await; | ||
| assert_openai_error( | ||
| response, | ||
| ExpectedError::Validation, | ||
| "Unsupported parameter(s)", | ||
| ) | ||
| .await; | ||
|
|
||
| assert_eq!(svc.engine.remaining_scripts().await, 0); | ||
| svc.shutdown().await; | ||
| }) | ||
| .await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn responses_reject_empty_input_and_required_tool_choice_without_tools() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OpenAI spec now deprecates
prompt_cache_retentionin favor ofprompt_cache_optionson the chat request, and openai-python 3.24+ exposes it. After this PR chat accepts the old field but still returns 400 for the new one. Add it here too?