to support Blip2 in mobile clip notebook

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2024-09-04T05:08:54Z ----------------------------------------------------------------

Line #4.    class blip2model(torch.nn.Module):

general code style in python to use CamelCase for classes nameing and lowercase underscore for class fields , variables and arguments, so

blip2model -> Blip2Model

Qformer -> q_former

review-notebook-app[bot] commented 1 week ago

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2024-09-04T05:08:55Z ----------------------------------------------------------------

Line #12.    if model_type.value == "Blip2":

I see a lot of if-else statemet with the same actions, maybe it is better to move into reusable functions?

review-notebook-app[bot] commented 1 week ago

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2024-09-04T05:08:55Z ----------------------------------------------------------------

I think it is not only Blip2 issue, CLIP generally mostly trained on English data, I think translation will be useful for other models as well. Can we make this description is more generic? Can we also high-light that we use optimum-intel for that and provide link on model card?

review-notebook-app[bot] commented 1 week ago

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2024-09-04T05:08:56Z ----------------------------------------------------------------

Line #3.    trans_model_path = "ov_models/trans_model"

maybe it is better to be more specific and add languages in dir naming (potentially, if it will be requested in future, we can extend it on other languages)?

review-notebook-app[bot] commented 1 week ago

View / edit / reply to this conversation on ReviewNB

eaidova commented on 2024-09-04T05:08:57Z ----------------------------------------------------------------

maybe it will be also good to mention about translation in this description?

openvinotoolkit / openvino_notebooks

to support Blip2 in mobile clip notebook #2347