Added support of 2qpcs for internvl and llava - #279
Conversation
8758392 to
5a2eae6
Compare
|
How do we test this? Can you add a test file specific to this file? |
5a2eae6 to
be34d9d
Compare
|
Fix DCO. |
be34d9d to
5af25d5
Compare
| from QEfficient.utils.logging_utils import logger | ||
|
|
||
|
|
||
| class QEffInternVisionEncoder2QPC(nn.Module): |
There was a problem hiding this comment.
rename to QEffInternVisionEncoderWrapper 2QPC is internal terminology.
| return vit_embeds | ||
|
|
||
|
|
||
| class QEffInternLanguageDecoder2QPC(nn.Module): |
|
|
||
| output_names = {} | ||
| if kv_offload: | ||
| lang_output_names.pop(1) |
There was a problem hiding this comment.
instead of popping. insert the value based on kv_offload flag.
This makes code more intuitive and readable
| output_names["vision"] = vision_output_names | ||
| output_names["lang"] = lang_output_names | ||
| else: | ||
| lang_output_names.pop(2) |
|
|
||
| output_names = {} | ||
| if kv_offload: | ||
| lang_output_names.pop(1) |
There was a problem hiding this comment.
same comment as for intern
| super().__init__(model) | ||
| self.model = model.get_qeff_language_decoder() |
There was a problem hiding this comment.
change to
super().__init__(model.get_qeff_language_decoder())
single line should handle assigning self.model as well
| ] | ||
| _onnx_transforms = [FP16ClipTransform, SplitTensorsTransform] | ||
|
|
||
| def __init__(self, model: nn.modules): |
There was a problem hiding this comment.
Change to
super().__init__(model.get_qeff_vision_encoder())
There was a problem hiding this comment.
Agree, but I think here we cannot do this, as here model is without transform, so model.get_qeff_vision_encoder() will through an error. There is one solution that we can inherit the QEFFBaseModel in _QEffAutoModelForImageTextToTextDualQPC and there init we can call super().__init__(model) which will apply the pytorch transform then there will be no need of calling super().__init__(model) later separately for the vision and language.
There was a problem hiding this comment.
Okay, let's keep it like this for now. in both vision and causal classes.
Later we will create Mixin classes to fix this by applying transform in the AutoImageText class
There was a problem hiding this comment.
For the vision I had kept the same but for the lang, I had done in one line
| self.config = self.model.language_model.config | ||
|
|
||
| def forward(self, input_ids, vit_embeds, position_ids, past_key_values): | ||
| # TODO: Check if Hardcoding this is okay, i.e. check if this value is common for all intern models |
There was a problem hiding this comment.
Why are we hardcoding and using magic numbers? How do we handle it in case of single QPC?
There was a problem hiding this comment.
This is the image token_id, and it is same for all the Internmodels available so we are directly keeping that here
| head_dim = self.config.text_config.hidden_size // self.config.text_config.num_attention_heads | ||
| if vis_cfg := getattr(self.config, "vision_config", None): | ||
| img_size = getattr(vis_cfg, "image_size", 336) | ||
| else: |
There was a problem hiding this comment.
why are we using magic numbers in the code?
There was a problem hiding this comment.
U mean to say img_size as 336 ?
| } | ||
| lang_inputs = { | ||
| "input_ids": torch.ones((BS, SEQ_LEN), dtype=torch.int64), | ||
| "image_features": torch.ones((BS, 576, self.language_model.config.hidden_size), dtype=torch.float32), |
There was a problem hiding this comment.
same as above for 576?
There was a problem hiding this comment.
So if we fix the img_size to 336, it will create a shape of (BS,576,hs), and if we don't fix 336 as img_size, we might have to run the vision part inside this function to get the shapes of image_features
There was a problem hiding this comment.
So let say user changes the img_size in config.json and then execute it. Then hardcoding shape of 576 won't work correct? Shouldn't we discuss and plan for a generic approach rather having support for a very specific config.
There was a problem hiding this comment.
Yes you are correct, we can try for a generic approach, and should discuss about it.
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Mohit Soni <quic_mohisoni@quicinc.com> Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
5af25d5 to
1d482a3
Compare
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com> Signed-off-by: Hem Agnihotri <quic_hemagnih@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com> Signed-off-by: Hem Agnihotri <quic_hemagnih@quicinc.com>
Signed-off-by: Mohit Soni <quic_mohisoni@quicinc.com> Signed-off-by: Dipankar Sarkar <quic_dipankar@quicinc.com>
No description provided.