[Renderer] Move InputPreprocessor into Renderer (1/2)#34510
Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that moves the logic from InputPreprocessor into the Renderer class. This centralization of input processing logic is a good architectural improvement. The changes include defining default tokenization parameters in the Renderer and allowing multimodal models to provide their own defaults, which helps resolve inconsistencies. The movement of StreamingInput to avoid circular imports and the relocation of benchmark-specific utilities are also positive changes.
I've found one issue where a method is called redundantly, which could impact performance. Please see my specific comment for details.
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that moves the responsibility of tokenization parameter management and multi-modal processing from various components into the Renderer. This centralization simplifies the LLM and InputProcessor classes and improves the overall code structure. The changes are well-executed and consistent with the goal of the PR. I've identified a few places where a None check for the multi-modal processor is missing, which could lead to a crash if multimodal data is passed to a non-multimodal model. Please see my detailed comments.
adfd4fc to
caa68e0
Compare
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
…4510) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
…4510) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Purpose
Towards #22880
TokenizeParamsin the Renderer, and let each MM model define its own defaults viaBaseProcessingInfo.get_default_tok_params. This allows tokenization for MM models to be handled by Renderer instead of inside MM processor, even for offline APIs. It should also solve the minor accuracy discrepency between offline and online APIs because of different handling ofadd_special_tokens.InputPreprocessor._tokenize_promptandInputProcessor._truncate_inputsto use Renderer.contains_only_stringscheck fromInputPreprocessortoInputProcessor.Misc changes:
StreamingInputfrom inputs file to engine protocol to avoid circular import.get_timing_stats_from_engine_clientto benchmarks since it's only used there.InputProcessor.closetoInputProcessor.shutdown.MultiModalCacheStats.record.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.