Add GitForCausalLM model in VQA pipeline
Author: jpizarrom
Pull request: yes

# What does this PR do?

Add GitForCausalLM model in VisualQuestionAnsweringPipeline.
Fixes part of #21110 and is based on #23348 #21227 .

## Who can review?

Hi @NielsRogge  what do you think of this??
Thanks!

## TODOs

- [x]  Add GIT model in VQA pipelines
- [x] Add tests
- [ ] Move custom preprocessing to the tokenizer/processor class (this shall be done in other PR, recommended in https://github.com/huggingface/transformers/pull/25509#issuecomment-1719288200)
- [ ] Update docs

Comments:
cc @Narsil 

Hi @Narsil, this PR is ready for review. Could you please take a look? Thanks :) 

> Can we have a single flag to switch behavior ?
do you mean to try use the same flag in preprocess as in _forward/postprocess?

the model_type was used in preprocess because there could other models that can generate but don't require the same custom GIT preprocessing e.g. Salesforce/blip2-opt-2.7b


This sounds like an issue with the tokenizer itself, no ?

+1 on @Narsil comment. Custom processing like this should be contained within our processing classes (tokenizers, image processors, processors) - the pipelines shouldn't need to know about the model type

Thanks for the feedback @amyeroberts and @Narsil

As you recommended, I will take a look on how to move the custom processing from the vqa pipeline to the processing classes (tokenizers, image processors, processors), and maybe it could also allow to remove it from the image_to_text pipeline as well.

https://github.com/huggingface/transformers/blob/3b39b906183ed08d9961908eb73104aeea345d11/src/transformers/pipelines/image_to_text.py#L125-L130

FWIW, the `image-to-text` pipeline also does preprocessing based on the model type, as each model has its own specifics: https://github.com/huggingface/transformers/blob/0f08cd205a440d23e6bf924cddd73ff48e09fe35/src/transformers/pipelines/image_to_text.py#L123-L142

> the pipelines shouldn't need to know about the model type

I fully agree with that statement. I just want to note that exceptions already exist within pipelines. But they are a maintenance burden, and we really need a solid argument as to why it cannot be done in lower level tools to justify.
Also when behavior is directly in the tokenizer/image_processor etc.. it makes like much easier to users who don't have to guess those snippets of code anymore.

Hi, I am very new, this is only my second contribution, but I have been trying to understand the code base, and possible options I have found are:
- for the current PR, just uses the model type, like is being done in other pipelines
- add custom parameters to the pipeline to populate preprocess_params, then the pipe could be called with `vqa_pipeline(image=image, question=question, add_special_tokens=False, add_cls_token=True)`, this could avoid the needs to mention a particular model in the pipeline, but the parameters shall be given on runtime
- for the specific case of this PR _microsoft/git-base-textvqa_ currently uses a `BertTokenizer`, maybe a custom tokenizer could be implemented for GitForCausalLM for VisualQuestionAnswering, i believe this could require to publish a new model with the new tokenizer, or it could be done in a different way?
- processors like [GitProcessor](https://github.com/huggingface/transformers/blob/main/src/transformers/models/git/processing_git.py#L23C7-L23C19) looks very promising, as they are already wrapping the _tokenizer_ and the _image processor_ , but it should be extended for the vqa case, and maybe a general strategy shall be found to add them to the pipelines, possible similar to the tokenizer/image processor/feature extraction instantiation in the [pipeline feactory](https://github.com/huggingface/transformers/blob/main/src/transformers/pipelines/__init__.py#L518C5-L518C13), maybe for this PR a custom processor could be instantiated directly in vqa-pipeline

I would be very grateful to know in which direction you recommend me to go

> Hi, I am very new, this is only my second contribution, but I have been trying to understand the code base, and possible options I have found are:
> 
> * for the current PR, just uses the model type, like is being done in other pipelines
> * add custom parameters to the pipeline to populate preprocess_params, then the pipe could be called with `vqa_pipeline(image=image, question=question, add_special_tokens=False, add_cls_token=True)`, this could avoid the needs to mention a particular model in the pipeline, but the parameters shall be given on runtime
> * for the specific case of this PR _microsoft/git-base-textvqa_ currently uses a `BertTokenizer`, maybe a custom tokenizer could be implemented for GitForCausalLM for VisualQuestionAnswering, i believe this could require to publish a new model with the new tokenizer, or it could be done in a different way?
> * processors like [GitProcessor](https://github.com/huggingface/transformers/blob/main/src/transformers/models/git/processing_git.py#L23C7-L23C19) looks very promising, as they are already wrapping the _tokenizer_ and the _image processor_ , but it should be extended for the vqa case, and maybe a general strategy shall be found to add them to the pipelines, possible similar to the tokenizer/image processor/feature extraction instantiation in the [pipeline feactory](https://github.com/huggingface/transformers/blob/main/src/transformers/pipelines/__init__.py#L518C5-L518C13), maybe for this PR a custom processor could be instantiated directly in vqa-pipeline
> 
> I would be very grateful to know in which direction you recommend me to go

Hi @amyeroberts @Narsil
What do you think should be the approach I should follow for this PR. Thanks

Hi @jpizarrom, what I would propose is the following: 

* We open a new, separate PR which adds `GitTokenizer`. This tokenizer will prepare the inputs as necessary for the model. We don't need to add a new model. We just need to add `tokenization_git.py` and update `tokenization_auto.py` and update the official checkpoints to use the git tokenizer. 
* We add a new `use_legacy` argument to `GitProcessor` and `GitTokenizer`
* If `use_legacy` is `True` then the tokenizer does the previous behaviour i.e. matching `BertTokenizer`. We also issue a warning that this will change in a future release (2 releases from the one this commit will be part of).
* If `use_legacy` is `False` then the tokenizer does the preparation logic for the model. We might need to add a conditional  `unsqueeze` option in the model's forward pass. 
* The default value for `use_legacy` is `True`. We change to `False` in a later version. 
* We still have a model-specific check in the pipeline. If model type is `git` we'll call: `self.tokenizer(inputs["question"], return_tensors=self.framework, padding=padding, truncation=truncation, use_legacy=False)`. This can be removed once `use_legacy` defaults to `False` 
* We'll need to be careful with any `add_special_tokens` logic

WDYT @ArthurZucker @Narsil  - is this is reasonable plan? 

Only having to add a tokenizer is very reasonable and optimal IMO. So yes 👍🏻  for this!

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.