Add Phi-3.5-vision
Author: Dahlbomii
Pull request: yes
Labels: name: New model

Draft PR for now, still need to add tests and convert to modular

cc @Rocketknight1

Fixes #36071 

Comments:
Looks good so far! The TODO list from here is:

- [x] Make the `transformers` imports relative imports so that we stop getting circular import errors in the CI
- [x] Add the `phi3_5.md` doc (you can copy the layout from a similar model and copy the text from the model card)
- [x] `pip install transformers[quality]` and `make fixup` or `make style` to get the repo consistency checks green

Once everything is green, then:
- [ ] Add tests (you can copy them from another similar VLM and just change model names etc.)

Once the new tests are green as well, then:
- [ ] Convert the modeling and configuration files to `modular`, then regenerating the modeling/config files and confirm everything passes

At that point, the PR should be finished!


Fixes #36166! 

I've run into an issue when trying to run the model.  From Modeling_phi3_v.py I'm getting "AttributeError: 'DynamicCache' object has no attribute 'get_max_length'"  This might be due to a change to the DynamicCache class, but I really can't tell!

@Dahlbomii hey! Yes, we deprecated `get_max_length` from cache a few versions ago, now it has to be `Cache.get_max_cache_shape(layer_idx)`

Woah - this means the remote code version of Phi-3.5-vision-instruct is broken on `main` right now and requires a fixed older version of `transformers`. Definitely makes this PR more urgent/important!

cc @zucchini-nlp @gante I can't figure this one out either! The `test_generate_continue_from_inputs_embeds` test is failing for Phi-3V but I don't really understand how `generate()` handles `inputs_embeds` - when I added some breakpoints it seems to generate an `input_ids` tensor with shape `(batch_size, 0)`, which then causes errors in Phi.

Seems like Phi-3.5V was not updated for the latest changes we had in generation, probably it has its own custom generation loop. For this test, we can call `super().prepare_input_for_generation(kwargs)` to get all the inputs processed correctly. Then image related kwargs can be set to `None` if needed. You can take a look on how we did it in other VLMs, hope it solves the issue


@zucchini-nlp yes, that was it, sorry! They don't override `generate()` but they do override `prepare_input_for_generation()` and discard `inputs_embeds` after the first step.

@Rocketknight1 for reasons that escape me, when the test processor tries to call self.get_component for the tokenizer, it breaks.  Any insight as to why?

@Rocketknight1 ALRIGHT I got most of the tests into the green, but I have no idea what I'm doing with the chat template stuff!

`test_apply_chat_template()` isn't really necessary - I think it just copied over from smolvlm! You can remove it.

@Rocketknight1 With that I think it might be ready for review

cc @Dahlbomii removing my review until @zucchini-nlp's comments are adressed! 

Any news on this? Phi-3.5-vision has been out for a while, just wanted to know an ETA if there is one

I will try to finish this PR next week

I'm sorry, I left this on the back burner for far longer than I thought! @zucchini-nlp I don't know if you've already started working on this, but I could pick it back up now if it's still up in the air! Whatever works best, really.

@Dahlbomii sure, would be perfect if you can pick it up. I haven't yet started working on it so feel free to start from addressing the above comments and lmk if you anything isn't clear 🤗 

**[For maintainers]** Suggested jobs to run (before merge)

run-slow: auto, phi3_v

@zucchini-nlp I could use a little help; I've been trying to get the model running again for testing, but another PR removed get_usable_length from cache_utils and I'm unsure how to adjust the model for this change.  If it helps I tracked down the removal, #39106 

Hey @Dahlbomii I can see that a lot of modules need standardizing and refactoring to transformers API. I can help with the PR to push changes quickly with my previous experience so that  the model can be merged faster. LMK if you’re open to that and if so please add me as a collaborator to this PR.

@Dahlbomii any progress on the PR? We would love to get it merged soon as it will help us to support the new Phi-Grounding model. If not, I'm happy to take over or ask @yaswanth19 for help if they have bandwidth

@zucchini-nlp Since there isn't much activity here for some time. I will start working on this and raise a new PR :saluting_face: 

I've been working on getting things back in line with codebase changes, but I still hadn't finished enough to push any commits!  I'm sure @yaswanth19 could handle things faster though, I can make you a collaborator still if that helps!

I can also share some of what I've been looking through: phi3_v's SuScaledRotaryEmbedding class is perfectly in line with the rope update's 'longrope' type, so a change to the config and using phi3's RotaryEmbedding class works out fine there.  However I couldn't figure out how to preserve phi3_v's YarnScaledRotaryEmbedding, as it still uses the long and short rope factors. Maybe your experience can help with that class, I've been at a loss.

Thanks @Dahlbomii for the confirmation,  AFAIK the the text backbone is phi3 language model itself so you don't have to worry about rope - simply utilize phi3 model or utilize modular. 

>I can make you a collaborator still if that helps!

Thanks, although I will open a new PR as this one is a bit clumsy and is in middle ground b/w custom code and ideal code.

@Rocketknight1, should close this as duplicated of #41977

Closing!