[Manual Test] diffusion endpoints allow model mismatch (upstream #2805)#20
[Manual Test] diffusion endpoints allow model mismatch (upstream #2805)#20lishunyang12 wants to merge 1 commit into
Conversation
Signed-off-by: xiaohajiayou <923390377@qq.com>
|
@claude review |
|
LGTM |
| @@ -2136,10 +2136,12 @@ async def _parse_video_form( | |||
| app_model_name, app_stage_configs = _resolve_video_runtime_context(raw_request) | |||
| effective_model_name = handler.model_name or app_model_name or request.model or "unknown" | |||
There was a problem hiding this comment.
The or request.model fallback silently defeats the new hard-error guard. When both handler.model_name and app_model_name are None (which is possible if OmniOpenAIServingVideo was constructed via __init__ with the default model_name=None, e.g. not via for_diffusion), effective_model_name becomes request.model and the check request.model != effective_model_name is always False — no 400 is ever raised.
The image endpoints don't have this hole: _get_engine_and_model uses "unknown" as the fallback, not the client's model string.
If the intent is a hard reject on mismatch, pull the model name directly from the handler/app and error explicitly when it's unset, rather than falling back to what the client sent:
| effective_model_name = handler.model_name or app_model_name or request.model or "unknown" | |
| effective_model_name = handler.model_name or app_model_name | |
| if effective_model_name is None: | |
| raise HTTPException( | |
| status_code=HTTPStatus.SERVICE_UNAVAILABLE.value, | |
| detail="Server model name is not configured.", | |
| ) |
Then keep the existing mismatch check below unchanged.
| effective_model_name = handler.model_name or app_model_name or request.model or "unknown" | ||
| if request.model is not None and effective_model_name is not None and request.model != effective_model_name: |
There was a problem hiding this comment.
The or request.model or "unknown" fallback defeats the validation. When both handler.model_name and app_model_name are None, effective_model_name falls through to request.model, so request.model != effective_model_name is always False — the 400 below can never fire. The image endpoints don't have this hole because they compare directly against model_name from _get_engine_and_model.
| effective_model_name = handler.model_name or app_model_name or request.model or "unknown" | |
| if request.model is not None and effective_model_name is not None and request.model != effective_model_name: | |
| effective_model_name = handler.model_name or app_model_name | |
| if request.model is not None and effective_model_name is not None and request.model != effective_model_name: |
|
Closing — superseded by label-only config change (no @claude, 3 tiers). |
|
👍 |
Manual test of the 3-tier model routing.
How to use
Apply one of these labels to kick off a review:
claude-reviewclaude-quickThen in comments, talk to the reviewer:
@claude review— full review (Sonnet)@claude why did you flag X?— question (Sonnet)@claude I disagree — Y handles that— disagreement (Sonnet)@claude LGTM?— short verdict (Sonnet)@claude look again after my fix— re-review (Sonnet)Good comparison experiment
claude-quickfirst → Haiku does a fast scanclaude-review→ Opus does a deep passMirrors upstream vllm-project#2805 (
[Bugfix] diffusion endpoints allow model mismatch— 3 files / 61 lines). Do not merge.