Repository navigation
perf(api-server): stop keeping finished sequences, keep prompt rendering and tokenizing off the event loop #2912
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
Changes from 10 commits
77212d9
bd367f7
c4ce7e5
3036898
a7b1b7c
b7387b9
6ca4ec8
80aea67
991517c
bf3f38a
25cb433
beefb96
9786106
5cfc14e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ def add_cli_args(cls, parser): | |
| def from_cli_args(cls, args): | ||
| return cls() | ||
|
|
||
| def create_engine(self, tokenizer=None): | ||
| def create_engine(self, tokenizer=None, **kwargs): | ||
| return None | ||
|
|
||
| _try_import_else_stub( | ||
|
|
@@ -876,6 +876,85 @@ async def drive(): | |
| assert events == ["initialize", "serving", "shutdown"] | ||
|
|
||
|
|
||
| class _FakeSeq: | ||
| def __init__(self, seq_id, token_ids): | ||
| self.id = seq_id | ||
| self.prompt_token_ids = list(token_ids) | ||
| self.num_prompt_tokens = len(token_ids) | ||
|
|
||
|
|
||
| def test_prompt_is_checked_and_truncated_in_the_preprocess_call(monkeypatch): | ||
| # /v1/messages and /v1/responses: no encode on the event loop. The length | ||
| # check reads the engine's own token count from the one preprocess call, | ||
| # and only an over-long prompt is built again, from its first ids. | ||
| requests = {} | ||
| monkeypatch.setattr( | ||
| api_server, | ||
| "engine", | ||
| SimpleNamespace(io_processor=SimpleNamespace(requests=requests)), | ||
| ) | ||
| monkeypatch.setattr( | ||
| api_server, | ||
| "tokenizer", | ||
| SimpleNamespace(decode=lambda ids, **_kw: f"<{len(ids)} ids>"), | ||
| ) | ||
| built = [] | ||
|
|
||
| def preprocess(prompt): | ||
| tokens = list(range(10)) if isinstance(prompt, str) else prompt | ||
| seq = _FakeSeq(len(built), tokens) | ||
| built.append(prompt) | ||
| requests[seq.id] = seq | ||
| return seq | ||
|
|
||
| cap = api_server._PromptCap(16) | ||
| seq = api_server._preprocess_prompt(preprocess, "short", cap) | ||
| assert built == ["short"] and seq.num_prompt_tokens == 10 | ||
| assert cap.cut_text is None | ||
| seq = api_server._preprocess_prompt(preprocess, "short", None) | ||
| assert seq.num_prompt_tokens == 10 | ||
|
|
||
| built.clear() | ||
| requests.clear() | ||
| cap = api_server._PromptCap(4) | ||
| seq = api_server._preprocess_prompt(preprocess, "long prompt", cap) | ||
| assert built == ["long prompt", [0, 1, 2, 3]] | ||
| assert seq.prompt_token_ids == [0, 1, 2, 3] | ||
| # The text the engine runs, for the reasoning channel's seed. | ||
| assert cap.cut_text == "<4 ids>" | ||
| # The discarded first build is not left registered. | ||
| assert list(requests) == [seq.id] | ||
|
|
||
|
|
||
| def test_responses_channel_follows_the_text_the_engine_runs(monkeypatch): | ||
|
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 doesn't drive responses_create, so deleting the call stays green.. can we add a structured case over the cap?
Collaborator
Author
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. Added @valar-cop re-review please. |
||
| # Structured: cut before the engine call (off the loop), so the channel | ||
| # the sampling params are built from reads the kept text. Unstructured: | ||
| # re-seeded from `cut_text` after the preprocess call cut it. | ||
| monkeypatch.setattr( | ||
| api_server, | ||
| "tokenizer", | ||
| SimpleNamespace( | ||
| encode=lambda text, **_kw: list(range(len(text))), | ||
| decode=lambda ids, **_kw: "x" * len(ids), | ||
| ), | ||
| ) | ||
| cap = api_server._PromptCap(4) | ||
| assert api_server._cut_prompt("abc", cap) == ("abc", "abc") | ||
| assert api_server._cut_prompt("abcdefgh", cap) == ([0, 1, 2, 3], "xxxx") | ||
|
|
||
| seeds = [] | ||
| monkeypatch.setattr( | ||
| api_server, "prompt_starts_in_reasoning", lambda text: seeds.append(text) | ||
| ) | ||
| monkeypatch.setattr( | ||
| api_server, "reasoning_channel", lambda start, **_kw: ("reseeded", start) | ||
| ) | ||
| assert api_server._reseed_after_cut("orig", cap, {}) == "orig" | ||
| cap.cut_text = "kept" | ||
| assert api_server._reseed_after_cut("orig", cap, {})[0] == "reseeded" | ||
| assert seeds == ["kept"] | ||
|
|
||
|
|
||
| class TestDeploymentDefaultOutputCap: | ||
| @pytest.mark.parametrize("explicit", [False, True]) | ||
| def test_small_deployment_distinguishes_default_from_explicit( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,19 +49,35 @@ def server(monkeypatch): | |
| """Stub the engine and record what each endpoint hands to it.""" | ||
| calls = SimpleNamespace(generate=[], setup=[], template="<rendered prompt>") | ||
|
|
||
| def encode(text, **_kw): | ||
| return [0] * (LONG_PROMPT_TOKENS if text == "<long prompt>" else 2) | ||
|
|
||
| def engine_prompt(prompt, kwargs): | ||
| # What preprocess builds: the prompt, cut to `prompt_cap.tokens` | ||
| # (shown as `<n>` for an n-token cut) when it is longer. | ||
| cap = kwargs.get("prompt_cap") | ||
| limit = None if cap is None else cap.tokens | ||
| n = len(encode(prompt)) | ||
| return (f"<{limit}>", limit) if limit is not None and n > limit else (prompt, n) | ||
|
|
||
| async def fake_generate_async(prompt, _sampling, _request_id, **kwargs): | ||
| calls.generate.append((prompt, kwargs)) | ||
| output = {"text": "hello", "finish_reason": "eos", "num_cached_tokens": 0} | ||
| sent, n = engine_prompt(prompt, kwargs) | ||
| calls.generate.append((sent, kwargs)) | ||
| output = { | ||
| "text": "hello", | ||
| "finish_reason": "eos", | ||
| "num_cached_tokens": 0, | ||
| "num_tokens_input": n, | ||
| "num_tokens_output": 1, | ||
| } | ||
| if kwargs.get("kv_transfer_params") is not None: | ||
| output["kv_transfer_output_meta_info"] = PREFILL_OUTPUT | ||
| yield output | ||
|
|
||
| async def fake_setup_streaming_request(prompt, _sampling, _request_id, **kwargs): | ||
| calls.setup.append((prompt, kwargs)) | ||
| return 1, _FinishedCollector(), 2 | ||
|
|
||
| def encode(text, **_kw): | ||
| return [0] * (LONG_PROMPT_TOKENS if text == "<long prompt>" else 2) | ||
| sent, n = engine_prompt(prompt, kwargs) | ||
| calls.setup.append((sent, kwargs)) | ||
| return 1, _FinishedCollector(), n | ||
|
|
||
| monkeypatch.setattr(api_server, "model_name", "m") | ||
| monkeypatch.setattr(api_server, "_request_logger", None) | ||
|
|
@@ -208,3 +224,41 @@ def test_without_handoff_truncation_still_follows_max_tokens( | |
| self._sent_prompt(server, endpoint, 4096, None) | ||
| == f"<{MAX_CTX - MAX_CTX // 8}>" | ||
| ) | ||
|
|
||
|
|
||
| class TestStructuredResponsesCutFirst: | ||
| """A structured /v1/responses request builds its reasoning channel into the | ||
| sampling params before the engine call, so an over-long prompt is cut | ||
| first and the channel is seeded from the kept text.""" | ||
|
|
||
| def test_over_the_cap_the_engine_gets_the_kept_ids(self, server, monkeypatch): | ||
| server.template = "<long prompt>" | ||
| configured = [] | ||
| seeds = [] | ||
|
|
||
| async def configure(_sampling, schema, channel, **_kw): | ||
| configured.append(schema) | ||
|
|
||
| monkeypatch.setattr(api_server, "_configure_structured_output", configure) | ||
| monkeypatch.setattr( | ||
| api_server, | ||
| "prompt_starts_in_reasoning", | ||
| lambda text: seeds.append(text) or False, | ||
| ) | ||
| monkeypatch.setattr(api_server, "validate_output", lambda *a, **kw: None) | ||
|
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 patch is dead for the non-stream path, it calls validate_responses_output. patch that and assert status 200?
Collaborator
Author
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. Good catch. It now patches |
||
| _responses( | ||
| { | ||
| "model": "m", | ||
| "input": "hi", | ||
| "max_output_tokens": 4096, | ||
| "text": {"format": {"type": "json_object"}}, | ||
| } | ||
| ) | ||
|
|
||
| sent, kwargs = server.generate[0] | ||
| kept = kwargs["prompt_cap"].tokens | ||
| assert configured == ['{"type": "object"}'] | ||
| # The engine is handed the kept ids, already under the cap. | ||
| assert sent == [0] * kept | ||
| # The channel was seeded from the cut text, before the engine call. | ||
| assert seeds[0] == f"<{kept}>" | ||
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.
pr description still describes the old design, please update it to match the diff..
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.
Updated after this review: Summary/Changes now describe
Config.keep_finished_outputsviacreate_engine,asyncio.to_thread, and the check/truncation in the preprocess call. Validation lists the current unit test run.