Add video modality for InstrucBLIP
Author: zucchini-nlp
Pull request: yes
Labels: name: run-slow

# What does this PR do?

I made these changes a month ago and forgot contributing. This PR adds video processing capabilities for InstructBLIP models. The paper states InstructBLIP was trained and evaluated on video, along with images and the original repo has some code on how video inference works. 

Seems like this feature has some interest from the community (see [here](https://huggingface.co/Salesforce/instructblip-vicuna-7b/discussions/8)), so I believe we can add it.

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_30182). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@NielsRogge 

> 1. making sure that we have a robust API for multimodal processors that is consistent
yeah, that needs to be reworked. Right now we have only Blip as first model that supports videos and there willl be VideoLlava. 

Unfortunately video llava processing is going a different way to easily be able to interleave modalities. I guess that is part of what is being discussed internally in slack

> 2. the current InstructBLIP models on the hub all use [BlipImageProcessor](https://huggingface.co/Salesforce/instructblip-vicuna-7b/blob/main/preprocessor_config.json#L11). This PR would introduce a new image processor, I guess we would then need to update the [auto mapping (https://github.com/huggingface/transformers/blob/e516d1b19d035469b4852e34ba0356587e6f8ade/src/transformers/models/auto/image_processing_auto.py#L75) to make sure AutoImageProcessor still works as expected.

Yep, seems like now it works only if calling specifically "InstructBlipImageProcessor"

@amyeroberts 
I see, but I am not sure how to make a new model and keep it easy for users to load and for us to maintain. For example: we can make a separate processing for video modality, which will be further enhanced with new features. Call it `InstructBlipVideoProcessing` and another class for video-based generation, `InstructBlipForConditionalVideoGeneration`? For the case of blip, which will never work with interleaving two vision modalities at the same time it might work. But I also want to make sure there will be consistency in how video+image modality LLMs are handled in `transformers`.

What if:
1. Image processor returns one of two possible tensors "pixel_values_image" (4-dim tensor) or "pixel_values_video" (5-dim tensor). This is the way I made it for VideoLlava, which actually can generate from interleaving both at the same time.
    1.1 Or maybe make Image and Video processor as separate classes, and call the appropriate while processing.
2. Modeling stays the same, but we add one line which expands image dimensionality so that it is `(batch_size, 1, num_channels, height, width)` and keep the rest of code as if it were a 1-frame video passed. All the conditions then can be removed, if we treat vision inputs as video-like but 1-frame for image and 4-frame for video clip.



@amyeroberts ping

@zucchini-nlp one thing related that I'll merge this week, regarding processors: https://github.com/huggingface/transformers/pull/30511 I added a `VideosKwargs`but no `VideosInput` yet. Personal opinion from having spent too much time around `Processors`, I think a separate model would be actually easier to maintain rather than patching a previous one, because _what the model does_ to _which modality_ is easier to understand rather than mixed modalities. I don't have the final say on this, so just a comment :) 

@zucchini-nlp Sorry for the late reply here. 

> Modeling stays the same, but we add one line which expands image dimensionality so that it is (batch_size, 1, num_channels, height, width) and keep the rest of code as if it were a 1-frame video passed. All the conditions then can be removed, if we treat vision inputs as video-like but 1-frame for image and 4-frame for video clip.

This is a neat solution. My main question would be, what does this do to the shapes of the returned tensors e.g. `hidden_states`? If they remain the same, then this is a nice easy way to enable this within the modeling file. 

If they're not the same, then we'll need to add a new class in the modeling file e.g. `InstructBlipForVideoConditionalGeneration` or perhaps, have a new modeling file `src/transformers/models/instructblip/modeling_instructblip_video.py`. The latter would add a new model type e.f. `VideoInstructBlip` / `InstructBlipVideo` which would enable the correct auto mapping in `image_processing_auto.py`. It's not unheard of for us to add new models which can load existing checkpoints under a new architecture name. 

In terms of the image processor: 
* If we can use the same class and just expand within the forward pass, then the trick is to correctly batch the inputs such that for a batch of images, the shape of the output pixel values remains `(batch_size, num_channels, height, width)` and for videos it's `(batch_size, num_frames, num_channels, height, width)`. Both should be output as `pixel_values`. That is, the output of the image processor for images should remain unchanged from the previous behaviour: same shape, and same key names in the `BatchFeature` output
* If we can't use the same class, but add a single module in `modeling_instructblip` you can do as above
* If we can't use the same class, but add a new modeling file, then we can add a new separate image processor. 

My vote would be for a new model. Having to handle video and images within the same image processor is a good indication they should be separated 

Thanks for detailed explanations! At this point it should be possible to go with the first option, I will have to go back and check. If not, making a separate model sounds good since BLIP will never work with both modalities at the same time.

@molbap i see, probably having separate VideoProcessor files can be a better solutions for mutli-modal models, instead of packing it all in ImageProcessor

I went for the "let's keep one processing and modeling files" way. Currently the following changes are applied:

1. Processor can accept as arg either `images` or `videos`
2. Image processor returns pixel values of shape (b, c, h, w) for images by squeezing the extra dim and adds and extra frame dimension for videos.
3. Modeling file unsqueezed back the frame dimension for images, and continues running as if the inputs are all videos. Finally the frame dimension is merged back to embeddings' sequence length.

Slow tests are passing locally + added a few tests for the video generation. 

Thanks for the continued work on this and apologies for the delay in my review. Skimming over the PR, and thinking about this more I think we should split this up such that we have a video image processor and a video model. We want to avoid conditional outputs from our base processing objects as well as conditional logic within our model's forward passes. As we won't interleave images and videos i.e. we don't expect a user to both be using video and images at the same time then we don't need to be able to handle these all at once 

@amyeroberts oke, I made Video InstructBlip its own model with its separate modeling and image processing files. Added some tests and indicated in the model-doc that it's the same model as InstructBlip except for the video processing capability. Ready for review

@ArthurZucker I made more changes into diff converter in this PR, as it didn't work in some cases. Specifically:

- Some models apply CamelCase without an underscore that splits subwords, like InstructBlip, Diff converter cannot infer correct model name in this case, so I added a possibility to indicate model-names by passing it as an arg for converter
- In case if we have a config that inherits from another config in the library, the auto-generated config doesn't get all the imports from its parent class causing errors. I added a special visited_module_config to store config-specific classes. I could use the same `visited_module` that is already defined but it will result in configs being imported into the config file
- Sometimes configs need globally defined vars, e.g. in case of the logger. So there's a new line that ports "SimpleStatements" to configs. If they are not used, everything will be cleaned up by ruff anyway 
- Properties with their setters weren't being ported because they have identical naming, so the fix is to iterate "node.body" directly w/o saving it in a dict

Plus these are the changes in [LLaVa-NeXT-Video PR](https://github.com/huggingface/transformers/pull/31252), just duplicating to have all written down somewhere :)
- Sometimes we want to add new methods in a class, and still retain all methods from the parent. This case wasn't covered, I added a few lines to fix it
- Inferring model-name from file-name didn't consider long model names with an underscore, fixed by modifying the regex

Still have to fix cases when only "init" docstring is changed, I couldn't make it work yet 

@amyeroberts this one is ready for the final review I guess

Got the CI green, including slow tests. Will merge the PR