fix(serve): reject empty/falsy prompt input in format_prompts and AsyncEngine.generate - #4803
Conversation
…ne.generate
Empty/falsy prompt input ('', [], {}, ()) is not None, so it slipped past the 'messages is not None' XOR guard in AsyncEngine.generate. The falsy messages then took the input_ids else-branch, leaving input_ids at its default None and crashing later in len(input_ids) with a confusing 'TypeError: object of type NoneType has no len()'.
Add a shared MultimodalProcessor.validate_prompt that rejects None and the empty str/list/tuple/dict shapes with a clear ValueError, and apply it at the two non-redundant layers: format_prompts (the single pipeline-layer chokepoint every prompt passes through) and generate (the direct-SDK backstop reachable without a model). The HTTP path already filters empty input, so this is an offline-SDK backstop only.
A multimodal (prompt, image) pair tuple has len == 2 and is intentionally not rejected.
ErenAta16
left a comment
There was a problem hiding this comment.
The premise checks out — the XOR guard tests identity, so any falsy-but-not-None value walks straight through it:
messages='x' input_ids=None -> raise=False (intended)
messages=None input_ids=[1] -> raise=False (intended)
messages='x' input_ids=[1] -> raise=True
messages=None input_ids=None -> raise=True
messages='' input_ids=None -> raise=False <-- empty string passes
messages=[] input_ids=None -> raise=False <-- empty list passes
so generate("") reaches the body with input_ids still None, which is what produces the len(None) TypeError rather than a message naming the actual problem. Worth stating in the PR body, because the guard reads correct at a glance and the reason it isn't is the is not None rather than the XOR.
Two things I'd want settled.
format_prompts now rejects an empty list, and that's a separate behaviour change from the generate guard. Grepping the tree, format_prompts has no caller outside its own definition, so nothing in-repo breaks today — but it's a public @staticmethod on MultimodalProcessor, so external callers passing [] to mean "no prompts in this batch" would start getting a ValueError where they previously got [] back. If that's intended, the PR body should say so; if the goal is only to fix generate, the format_prompts change could be dropped and the guard would still do its job.
validate_prompt enumerates types rather than testing falsiness, which is the right call but for a reason the code doesn't state: a bare if not prompt would also reject 0. input_ids=[0] is a legitimate single-token prompt and not [0] is False, so that's fine, but validate_prompt is also called with name='input_ids' where a future caller might pass a bare int. The explicit per-type checks avoid that class of surprise — worth a one-line comment saying so, otherwise someone will simplify it back to if not prompt later.
Smaller: the test module docstring references "P8-002" and "RED on main, GREEN on branch", which read like internal tracker notes rather than something for this repo's history. Also _make_engine stubs _determine_gen_config to reproduce the crash, so the test is pinning the guard rather than the real call path — that's a reasonable trade given the alternative needs a loaded model, but the docstring should say the stub is what makes it fail on main, not the engine itself.
|
Read this against 1. The nested empty case, which is the one the motivation describes, still gets through.
for prompt in prompts:
MultimodalProcessor.validate_prompt(prompt, name='prompt')A format_prompts([('', image)])The tuple has length 2, So the guard catches 2. A batch with one empty string now fails as a whole. format_prompts(['a', '', 'b'])returned 3.
Smaller notes:
None of this is an argument against the change; the underlying complaint is legitimate and the offline SDK path should not accept an empty prompt silently. It's the scope that I think needs pinning down, since as written the fix is narrower than the motivation claims. |
ErenAta16
left a comment
There was a problem hiding this comment.
The problem is real and turning that TypeError from len(input_ids) into a named ValueError at the boundary is the right shape of fix. I loaded validate_prompt off this branch and ran it rather than reading the diff, and the cases the PR targets all behave:
'' rejected [] rejected () rejected
{} rejected None rejected ' ' accepted (correctly, a space is a prompt)
I also checked format_prompts rather than assuming the message was wrong, since 'prompts cannot be an empty list' looked like it could fire on a bare string. It cannot: the wrap into a list happens above the check, so '' becomes [''], survives that guard, and is rejected by validate_prompt with prompt string cannot be empty. The message a caller sees is accurate in both paths.
The gap I would want closed before this lands is on the input_ids side. validate_prompt is applied to input_ids as well, and there the realistic zero-length value is not a list:
validate_prompt(np.array([], dtype=int), name='input_ids') # accepted
validate_prompt(torch.tensor([], dtype=torch.long), name='input_ids') # accepted
validate_prompt([], name='input_ids') # rejectedAn empty tensor or ndarray is exactly the zero-length input this guard exists to stop, and both walk straight past it into the same len()-then-index path the PR is fixing. Someone tokenising to a tensor and hitting an empty result gets the original confusing failure, and now with a guard in place that looks like it should have caught it.
The same shape applies to bytes and set, which are less likely to show up but fall through for the same reason: the checks enumerate four concrete types instead of asking whether the value is empty.
Two ways out. Either add len()-based handling for sized objects:
if not isinstance(prompt, (str, bytes)) and hasattr(prompt, '__len__') and len(prompt) == 0:
raise ValueError(f'{name} cannot be empty')or keep the type list and add np.ndarray / torch.Tensor explicitly. I would lean toward the first, since it also covers whatever container type shows up next without another edit, and it keeps the docstring honest. Right now the docstring says "reject None / empty / falsy prompt input" and the implementation rejects None plus four specific empty containers, which is a narrower contract than it advertises.
One thing to be deliberate about rather than a defect: '' is now a hard error on the offline path. For a chat model that is clearly right. For a base model, generating unconditionally from an empty prompt is a legitimate thing to ask for, and this closes that off. If lmdeploy does not support that today then there is nothing to do, but it is worth a sentence in the PR description so it is a decision on record rather than a side effect of tightening the guard.
Adding a dedicated tests/test_lmdeploy/serve/test_empty_prompt_guard.py with the RED/GREEN note in the header is good practice, and it makes the regression obvious to whoever touches this next. If you take the __len__ route above, an empty-tensor case in that file would pin the part that is currently uncovered.
…e caught Collapse the four isinstance branches into a single hasattr(prompt, '__len__') and len(prompt) == 0 check, so empty numpy/torch tensors, bytes and sets are rejected too, not just str/list/tuple/dict. The per-type messages are replaced by type(prompt).__name__ interpolation. Also clean internal tracker notes out of the test module header and add a regression case for an empty ndarray input_ids.
|
Thanks for the thorough read @ErenAta16 — really appreciate you actually running Empty tensors / generalise to Nested empty case ( Batch behaviour change. Empty string as a hard error for base models. Recorded as a deliberate decision rather than a side effect: unconditional generation from an empty prompt is legitimate for a base model, but lmdeploy does not expose that through Test module notes. Removed the internal Method name (
|
|
Hi, @ErenAta16 We are writing on behalf of the lmdeploy project maintainers. We have noticed that you have been posting a very large number of comments across many pull requests in our repository. While we appreciate your enthusiasm for the project, your activity raises a serious concern: you are not a member of the official lmdeploy team, yet your frequent comments may lead other users to mistakenly perceive your personal opinions as official guidance or recommendations from the project. This can create significant confusion within the community, as your views do not necessarily represent the project’s technical direction or policy. To avoid misleading others, we kindly ask you to limit your PR comments to only the ones you created. For broader questions or suggestions, please open a dedicated Issue instead. We also encourage you to clearly state in your comments that you are speaking on your own behalf, not as a representative of lmdeploy. We hope you understand that we value focused, constructive participation. If this pattern of excessive commenting continues, we may need to take further moderation actions to protect the project’s workflow. We trust that you will cooperate and adjust your approach accordingly. Thank you for your cooperation. |
|
Understood, and thank you for saying it directly rather than just muting me. Your point about readers mistaking an outside comment for project guidance is fair, and it is not something I had weighed properly. Volume was the mistake. This is my last comment on a PR I did not open here. I will keep to my own (#4811) and use an issue if I have something worth raising. Apologies to the authors whose threads I added noise to. For what it is worth, @SuperMarioYL, the |
|
Hi, @SuperMarioYL Thanks for your PR. I would like to ask you to move the validation logic out of
Please revise your PR accordingly. Let us know if you need any clarification. Thanks for your cooperation. |
| @@ -0,0 +1,103 @@ | |||
| # tests/test_lmdeploy/serve/test_empty_prompt_guard.py | |||
There was a problem hiding this comment.
These test cases add little value and may increase maintenance burden without improving real coverage. Could you please remove this file from the PR?
There was a problem hiding this comment.
Done — removed the file from the PR (63ec775). Thanks for the callout.
AsyncEngine.generate no longer calls validate_prompt; the core engine stays interface-agnostic. The guard now lives solely at the Pipeline layer — MultimodalProcessor.format_prompts (the chokepoint pipeline.py calls) rejects empty/None prompts, and validate_prompt stays in MultimodalProcessor for that layer. The HTTP path already filters empty input upstream, so only the direct-SDK offline path loses the backstop, which is the caller's responsibility per the review. Remove tests/test_lmdeploy/serve/test_empty_prompt_guard.py per review feedback (low value, maintenance burden).
Remove the engine-core validate_prompt calls so AsyncEngine stays interface-agnostic, leaving the Pipeline layer (MultimodalProcessor. format_prompts, the chokepoint pipeline.py calls) as the sole home of the empty-prompt guard, per review. The HTTP path already filters empty input upstream, so only the direct-SDK offline path loses the backstop.
|
Thanks @lvhan028. Revised in 63ec775 + b6729eb:
The HTTP/OpenAI path already filters empty input upstream, so only the direct-SDK offline path loses the engine-level backstop — which is the caller's responsibility per your point. Happy to add an explicit guard at the |
|
I've tested the following cases with main branch: So, I think a better way to deal with the "TypeError: object of type 'NoneType' has no len()" is changing one line code in async_engine.py. That is: |
Change `if messages:` to `if messages is not None:` in AsyncEngine.generate so empty/falsy-but-not-None prompts (e.g. '') enter the processing branch instead of skipping it and hitting a downstream len(None) TypeError. None still skips the branch as before. Per maintainer review (lvhan028), this one-line guard supersedes the earlier Pipeline-layer validation.
|
Thanks @lvhan028 for testing main and the proposed one-liner — adopted it in the latest push. Changes in this update:
Net diff vs base is now a single line. Could you re-run your four cases ( |
fix(serve): reject empty/falsy prompt input in format_prompts and AsyncEngine.generate
Motivation
Passing an empty/falsy prompt (
'',[],{},()) to the offline SDK path(
AsyncEngine.generate/pipe.stream_infer) does not raise a clear error. Theprompt is not
None, so it slips past themessages is not NoneXOR guard inAsyncEngine.generate(async_engine.py:501). The falsymessagesthen takes theinput_idselse-branch, leavinginput_idsat its defaultNone, and the requestlater crashes in
len(input_ids)with a confusing:at
async_engine.py:566(and the same shape at:424). The HTTP path alreadyfilters empty messages (
api_server.py:443-445), so this only affects users of theoffline SDK /
pipelineAPI who pass malformed input — they get an opaqueTypeErrordeep inside the engine instead of an actionable error at the boundary.A regression test (
tests/test_lmdeploy/serve/test_empty_prompt_guard.py) reproducesthe real
generate()path (no model/GPU required) and is red onmain(raisesTypeError) / green on this branch (raises a clearValueError).Modification
lmdeploy/serve/processors/multimodal.py: add a sharedMultimodalProcessor.validate_prompt@staticmethodthat rejectsNoneand theempty str/list/tuple/dict shapes with a clear
ValueError; call it at the top offormat_prompts(root-cause boundary). A(prompt, image)multimodal pair haslen == 2and is intentionally not rejected.lmdeploy/serve/core/async_engine.py: callvalidate_promptingenerate()immediately after the existing XOR guard, covering both the
messagesandinput_idsfalsy shapes (direct-SDK backstop).tests/test_lmdeploy/serve/test_empty_prompt_guard.py: new regression test(red-on-
main/ green-on-branch) exercising the realgenerate()path viaAsyncEngine.__new__+ minimal mocks (no model/GPU), plus pure-functionformat_prompts('' / [] / None)assertions.Scope is intentionally offline-SDK only: the HTTP (
serve/openai,serve/anthropic), TurboMind C++, PyTorch engine,pipeline.py, and the reward/pplpaths are untouched.
BC-breaking (Optional)
No. The change only converts a previously-crashing input (
TypeErrordeep insidethe engine) into an early, clear
ValueErrorat the input boundary. All valid(non-empty) input flows through unchanged. Downstream projects passing valid prompts
see no behavioral change.
Use cases (Optional)
Offline SDK users who accidentally pass an empty prompt (
pipe(''),engine.generate(messages=''), an empty batch) now get an actionableValueError: ...at the boundary instead of an opaqueTypeError: object of type 'NoneType' has no len()from inside the engine.Checklist
ruff checkon all changed files — all checks passed (line-length 120,E/F/I/W/UP, py310), per
.pre-commit-config.yaml.python -m pytest tests/test_lmdeploy/serve/test_empty_prompt_guard.py -q→ 7 passed (green on branch; 7 failed onmain);python -m pytest tests/test_lmdeploy/serve/test_session_cleanup.py -q→ 9 passed (regression forthe touched
generate()path, no regressions).error for previously-crashing invalid input).