[V1] [2/n] Logging and Metrics - OutputProcessor Abstraction
#11973
Conversation
Signed-off-by: [email protected] <[email protected]>
robertgshaw2-neuralmagic
requested review from
WoosukKwon,
njhill,
ywang96,
comaniac and
alexm-neuralmagic
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
robertgshaw2-neuralmagic
changed the title
OutputProcessor Abstraction
| queue=queue, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
NOTE: this was previously called Detokenizer
| @@ -59,9 +59,6 @@ def __init__( | |||
| lora_config=vllm_config.lora_config) | |||
| self.tokenizer.ping() | |||
|
|
|||
| # Request streams (map of request_id -> queue). | |||
There was a problem hiding this comment.
NOTE: these queues are held in OutputProcessor
| If you need to touch every element of the batch, implement a | ||
| method called XXXClass.update_from_output() to be called | ||
| within the loop below. For examples, see: | ||
| * IterationStats.update_from_output() |
There was a problem hiding this comment.
This is a great abstraction IMO.
nit: I wonder if we also want to make it more explicit by having something like a OutputHandler protocol that takes in the engine core output + maybe a current request state?
There was a problem hiding this comment.
I will add a comment suggesting that we do this once we add RequestStats. I want to keep flexibility while we are in the development stage
| return | ||
|
|
||
| self.num_generation_tokens += len(output.new_token_ids) | ||
| if is_prefilling: |
There was a problem hiding this comment.
I think one scenario that might potentially complicate things is when a request has a pretty long prompt, and its prefill actually will span multiple "iteration". With current architecture, the prompt throughput stats is actually not accurate.
I wonder if we should have a path where we propogate each scheduler iteration rather than engine core iteration to the front end process for more accurate stats.
There was a problem hiding this comment.
We don't send outputs from EngineCore until it generates a token.
So from the POV of the Engine len(new_token_ids) > 0. We should add an assert for this invariant.
There was a problem hiding this comment.
added comment and assert.
|
@mgoin. Tests are added. Could I get an approval? |
Signed-off-by: [email protected] <[email protected]>
github-actions
bot
added
the
ready
robertgshaw2-neuralmagic
merged commit 619ae26
into
vllm-project:main
SUMMARY:
Detokenizer>>OutputProcessor.XXXClass.update_from_output+ be called inOutputProcessor.process_outputsloop.self._process_request_outputsinto this loop (previously this was a separate loop inoutput_handler)IterationStats.update_from_output()to this loopabortNOTES:
Detokenizerin a separate process hurts performance ([V1] [7/N] API Server: Multiprocessing Detokenizer [ DO NOT MERGE ] #11636). So feel confident that adding all of this to a single loop is the right approach.