Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/nightly-benchmark.yml
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ jobs:
model:
- { id: meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8, slug: meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8, test_class: TestNightlyLlama4MaverickSingle }
- { id: minimaxai/minimax-m2, slug: minimaxai-minimax-m2, test_class: TestNightlyMinimaxM2Single }
- { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix YAMLlint's braces violation.

This new inline mapping is already being flagged by static analysis, so the workflow will stay red until the spacing matches the repo's YAMLlint rule.

🧹 Minimal fix
-          - { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single }
+          - {id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single }
- {id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single}
🧰 Tools
🪛 YAMLlint (1.38.0)

[error] 349-349: too many spaces inside braces

(braces)


[error] 349-349: too many spaces inside braces

(braces)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/nightly-benchmark.yml at line 349, Replace the inline
mapping "- { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class:
TestNightlyGlm46Single }" with a block-style mapping to satisfy YAMLlint braces
rules: convert the list item to use nested keys (id, slug, test_class) on
separate indented lines under the list dash, preserving the same values and
indentation so the entry becomes a normal YAML mapping rather than an inline
brace mapping.

variant:
- { id: sglang, runtime: sglang, grpc_only: "false", setup_vllm: false, setup_trtllm: false, extra_deps: "genai-bench" }
- { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" }
Expand Down
148 changes: 145 additions & 3 deletions crates/protocols/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::validated::Normalizable;
/// This is the main request type for `/v1/messages` endpoint.
#[serde_with::skip_serializing_none]
#[derive(Debug, Clone, Serialize, Deserialize, Validate, schemars::JsonSchema)]
#[validate(schema(function = "validate_mcp_config"))]
#[validate(schema(function = "validate_message_request"))]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

These validator changes appear unrelated to GLM-4.6 benchmark coverage.

Per CatherineSue's review comment, this PR's scope is adding GLM-4.6 to nightly benchmarks. The validate_message_request changes (replacing validate_mcp_config and adding tool_choice/tools cross-field validation) are unrelated protocol-level changes that should be in a separate PR.

Also applies to: 108-155

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/protocols/src/messages.rs` at line 23, The PR includes unrelated
protocol validator changes — the attribute #[validate(schema(function =
"validate_message_request"))] and the new cross-field validations (replacing
validate_mcp_config and adding tool_choice/tools checks) — which must be removed
from this benchmark-focused PR; revert messages.rs to use the original validator
configuration (restore use of validate_mcp_config where it was before) and
remove the new validate_message_request/schema reference and any added
cross-field logic (tool_choice/tools validation and related helper functions) so
these protocol-level changes can be submitted in a separate follow-up PR.

pub struct CreateMessageRequest {
/// The model that will complete your prompt.
#[validate(length(min = 1, message = "model field is required and cannot be empty"))]
Expand Down Expand Up @@ -105,13 +105,52 @@ impl CreateMessageRequest {
}
}

/// Validate that `mcp_servers` is non-empty when `mcp_toolset` tools are present.
fn validate_mcp_config(req: &CreateMessageRequest) -> Result<(), validator::ValidationError> {
/// Validate cross-field constraints for Messages API requests.
fn validate_message_request(req: &CreateMessageRequest) -> Result<(), validator::ValidationError> {
if req.has_mcp_toolset() && req.mcp_server_configs().is_none() {
let mut e = validator::ValidationError::new("mcp_servers_required");
e.message = Some("mcp_servers is required when mcp_toolset tools are present".into());
return Err(e);
}

let Some(tool_choice) = &req.tool_choice else {
return Ok(());
};

let has_tools = req.tools.as_ref().is_some_and(|tools| !tools.is_empty());
let requires_tools = !matches!(tool_choice, ToolChoice::None);

if requires_tools && !has_tools {
let mut e = validator::ValidationError::new("tool_choice_requires_tools");
e.message = Some(
"Invalid value for 'tool_choice': 'tool_choice' is only allowed when 'tools' are specified."
.into(),
);
return Err(e);
}

if let ToolChoice::Tool { name, .. } = tool_choice {
let tool_exists = req.tools.as_ref().is_some_and(|tools| {
tools.iter().any(|tool| match tool {
Tool::Custom(tool) => tool.name == *name,
Tool::ToolSearch(tool) => tool.name == *name,
Tool::Bash(tool) => tool.name == *name,
Tool::TextEditor(tool) => tool.name == *name,
Tool::WebSearch(tool) => tool.name == *name,
Tool::McpToolset(_) => false,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Permit mcp_toolset when validating specific tool_choice

In validate_message_request, a request with tool_choice: {"type":"tool","name":...} and tools coming from mcp_toolset is now always rejected because the McpToolset branch hard-codes false. This introduces a 400 regression for MCP-specific tool selection (including passthrough requests and SMG MCP flows where tools are materialized later via inject_mcp_tools_into_request), even when the named MCP tool would be valid at runtime. The existence check should not fail solely because the tool is sourced from mcp_toolset.

Useful? React with 👍 / 👎.

})
});

if !tool_exists {
let mut e = validator::ValidationError::new("tool_choice_tool_not_found");
e.message = Some(
format!("Invalid value for 'tool_choice': tool '{name}' not found in 'tools'.")
.into(),
);
return Err(e);
}
}

Ok(())
}

Expand Down Expand Up @@ -1764,6 +1803,46 @@ mod tests {

use super::*;

fn base_request() -> CreateMessageRequest {
CreateMessageRequest {
model: "claude-test".to_string(),
messages: vec![InputMessage {
role: Role::User,
content: InputContent::String("hello".to_string()),
}],
max_tokens: 16,
metadata: None,
service_tier: None,
stop_sequences: None,
stream: None,
system: None,
temperature: None,
thinking: None,
tool_choice: None,
tools: None,
top_k: None,
top_p: None,
container: None,
mcp_servers: None,
}
}

fn custom_tool(name: &str) -> Tool {
Tool::Custom(CustomTool {
name: name.to_string(),
tool_type: None,
description: Some("test tool".to_string()),
input_schema: InputSchema {
schema_type: "object".to_string(),
properties: None,
required: None,
additional: HashMap::new(),
},
defer_loading: None,
cache_control: None,
})
}

#[test]
fn test_tool_mcp_toolset_defer_loading_deserialization() {
let json = r#"{
Expand Down Expand Up @@ -1876,6 +1955,69 @@ mod tests {
}
}

#[test]
fn test_tool_choice_auto_requires_tools() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::Auto {
disable_parallel_tool_use: None,
});

assert!(request.validate().is_err());
}

#[test]
fn test_tool_choice_any_requires_tools() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::Any {
disable_parallel_tool_use: None,
});

assert!(request.validate().is_err());
}

#[test]
fn test_tool_choice_specific_tool_requires_tools() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::Tool {
name: "get_weather".to_string(),
disable_parallel_tool_use: None,
});

assert!(request.validate().is_err());
}

#[test]
fn test_tool_choice_specific_tool_must_exist() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::Tool {
name: "get_weather".to_string(),
disable_parallel_tool_use: None,
});
request.tools = Some(vec![custom_tool("search_web")]);

assert!(request.validate().is_err());
}

#[test]
fn test_tool_choice_none_without_tools_is_valid() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::None);

assert!(request.validate().is_ok());
}

#[test]
fn test_tool_choice_specific_tool_is_valid_when_declared() {
let mut request = base_request();
request.tool_choice = Some(ToolChoice::Tool {
name: "get_weather".to_string(),
disable_parallel_tool_use: None,
});
request.tools = Some(vec![custom_tool("get_weather")]);

assert!(request.validate().is_ok());
}

#[test]
fn test_full_message_with_tool_search_flow_deserialization() {
// Simulates the full response from Anthropic API with tool search flow
Expand Down
1 change: 1 addition & 0 deletions e2e_test/benchmarks/test_nightly_perf.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def _run_nightly(setup_backend, genai_bench_runner, model_id, worker_count=1, **
("Qwen/Qwen3-30B-A3B", "Qwen30b", 4, ["http", "grpc"], {}),
("openai/gpt-oss-20b", "GptOss20b", 1, ["http", "grpc"], {}),
("minimaxai/minimax-m2", "MinimaxM2", 1, ["http", "grpc"], {}),
("zai-org/GLM-4.6", "Glm46", 1, ["http", "grpc"], {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

multi_workers=1 still generates a duplicate single-worker variant.

The generator at Lines 145-151 will create TestNightlyGlm46Multi for this entry, but _run_nightly() treats worker_count == 1 as worker_type == "single". That leaves you with a misleading extra class and colliding experiment_folder names if the whole module is collected, even though the PR objective is single-worker coverage only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e_test/benchmarks/test_nightly_perf.py` at line 105, The generator
currently emits a "*Multi" test class even when the entry sets multi_workers
(worker_count) == 1, which collides with _run_nightly() logic that treats
worker_count==1 as single; update the generator that creates TestNightly...Multi
to only emit the multi-worker variant when multi_workers (worker_count) > 1 (or
alternatively derive worker_type from worker_count), and ensure the generated
experiment_folder includes the worker_type or worker_count to avoid name
collisions between single and multi variants; reference the generator that
creates TestNightlyGlm46Multi, the multi_workers/worker_count field in the
tuple, the experiment_folder naming, and the _run_nightly() function when making
the change.

(
"meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8",
"Llama4Maverick",
Expand Down
6 changes: 6 additions & 0 deletions e2e_test/infra/model_specs.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ def _resolve_model_path(hf_path: str) -> str:
"worker_args": ["--trust-remote-code"],
"vllm_args": ["--trust-remote-code"],
},
# GLM-4.6 - nightly benchmarks
"zai-org/GLM-4.6": {
"model": _resolve_model_path("zai-org/GLM-4.6"),
"tp": 8,
"features": ["chat", "streaming", "function_calling", "reasoning"],
},
# Llama-4-Maverick (17B with 128 experts, FP8) - Nightly benchmarks
"meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8": {
"model": _resolve_model_path("meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8"),
Expand Down