bugfix: fix open-ai temperature and reasoning effort. - #87
Conversation
chenchuw886
left a comment
There was a problem hiding this comment.
Thanks for adding the OpenAI temperature default and forwarding top-level reasoning_effort into chat_template_kwargs. Two PD-disaggregated response-path gaps are still uncovered:
Scheduler::record_new_request()still resolves both parsers from the client-providedrequest->model, while mixed deployment resolves from the actual model type inconfig.json. Model aliases therefore diverge between mixed and PD deployments.infer_model_type_from_model_id()still has no DeepSeek V4 mapping. With--tool_call_parser=autoor--reasoning_parser=auto, the bridge returns an empty parser, so PD can still return raw DSML and omit parsedreasoning_content.
Please resolve parser formats from the tokenizer/model config, add the DeepSeek V4 mappings (deepseekv4 for tool calls and deepseek-v4 for reasoning), and add streaming/non-streaming regression tests for PD tool calls and reasoning. The observed explicit tool-parser case should also be covered because this PR currently does not touch that path.
Also, the third_party/CMakeLists.txt and Anthropic test changes appear unrelated to the OpenAI fix; please split them or explain why they are required.
chenchuw886
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review. The DeepSeek V4 OpenAI/PD paths now look covered: parser formats come from the deployed model config, the V4/V4-MTP mappings are present, and the new tests exercise streaming/non-streaming tool calls and reasoning with a client-side model alias.
I found one remaining cross-model regression that should be fixed before merge:
- GLM5 with
--reasoning_parser=autonow aborts during Scheduler construction. The new startup-time resolution passes the exact config model type (glm_moe_dsaorglm_moe_dsa_mtp) toReasoningParser::get_parser_auto(). In the pinned xLLM registry, those model types are mapped for the tool parser, but the reasoning auto map only containsglm4_moe -> glm45; although aglm5reasoning detector exists, there is no auto mapping for the GLM5 model types.get_parser_name_by_model_type()therefore reachesLOG(FATAL). Previously, the request-model inference collapsed GLM aliases toglm4_moe, so reasoning auto did not fail this way. Please mapglm_moe_dsa/glm_moe_dsa_mtpto theglm5reasoning parser (in the bridge or upstream registry) and add regression tests for both model types.
There is also a compatibility edge worth preserving: when config.json cannot be read or has no model_type, model_type.value_or("") makes both auto parsers silently resolve to empty strings. The previous request-model inference could still select a parser in that case. Please retain the request-model fallback when config model type is unavailable, or at minimum emit a clear warning instead of silently returning raw tool/reasoning output. A test with an unavailable model type would make this behavior explicit.
No description provided.