-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[None][fix] Multimodal InputProcessor dummy builder fix #8916
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,9 @@ class InputProcessor(Protocol): | |||||||||||||||
| config: any | ||||||||||||||||
| tokenizer: any | ||||||||||||||||
|
|
||||||||||||||||
| def __init__(self, **kwargs): | ||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||
|
|
||||||||||||||||
|
Comment on lines
+45
to
+47
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. Fix Protocol init to avoid super() in MRO (TypeError risk). InputProcessor is in the MRO of concrete processors; calling super().init(**kwargs) here can bubble kwargs to ABC/object and crash. Swallow kwargs instead. Apply this diff: class InputProcessor(Protocol):
@@
- def __init__(self, **kwargs):
- super().__init__(**kwargs)
+ def __init__(self, **kwargs) -> None:
+ # Protocol initializer intentionally swallows kwargs to terminate
+ # cooperative __init__ chains safely.
+ pass📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsThere 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. Why we need Line:45-46? I think we can simply skip definition of 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. 🧩 Analysis chain🏁 Script executed: Length of output: 664 @jaedeok-nvidia You're absolutely right! The Here's why:
Recommendation: Remove lines 45-46 entirely. The Protocol should only define the structural interface ( class InputProcessor(Protocol):
"""..."""
model_path: any
config: any
tokenizer: any
# No __init__ needed here
def __call__(
self, inputs: TextPrompt, sampling_params: SamplingParams
) -> Tuple[List[int], Optional[ExtraProcessedInputs]]:
...🧬 Code Graph Analysis Results
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. @jaedeok-nvidia, the reason for 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.
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. @coderabbitai can you share your thoughts? 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. 🧩 Analysis chain🏁 Script executed: Length of output: 1905 @yechank-nvidia You are absolutely correct! Thank you for the clarification. My initial analysis was wrong. Here's what's actually happening: The Multiple Inheritance ChainClasses like class Qwen2VLInputProcessorBase(BaseMultimodalInputProcessor,
BaseMultimodalDummyInputsBuilder):The Method Resolution Order (MRO) for such classes is:
Why
|
||||||||||||||||
| def __call__( | ||||||||||||||||
| self, inputs: TextPrompt, sampling_params: SamplingParams | ||||||||||||||||
| ) -> Tuple[List[int], Optional[ExtraProcessedInputs]]: | ||||||||||||||||
|
|
||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.