we may want to think about how this could tie in with other models. Seems not specific to Granite.

Yeah, this is not Granite-specific, but hasn't been needed so far with any other models.
So far only granite-vision via llava_next sends its inputs to granite in this form. So maybe good for now for Granite?
But you are right, pixtral would probably have something similar.

For this param, I think we can get away with checking on the tensor.dim and removing the param entirely. If the size is 3, it would skip embedding, if the size is 2 would run the embedding. If we do this, we would add documentation for how x_in is handled. In either case, whether including a param or not, we will need some error handling, so don't really see a downside to this.

Cool, will try it out!

Done

typically we call this tie_heads, might make sense to make the same assumption

will replace-- actually wasn't being used, so removed it.

can you mention how hf initializes its weights

HF doesn't have this particular initialization! Will update in comment

Should this match closer to granite with is_input_embedded?

Hmm, this is passed with this particular key from an image processor.
Also, you are right, is_input_embedded is set to True, when inputs_embeds are sent to granite.forward() in line 399

should this raise an exception?

Had an exception here originally, but the tests complained for some reason. I can try and bring it back.

Could we fix these as part of this PR?

Let's do it in a follow-up PR.
Once this original implementation is in, others from the team will address the graph breaks issue-- one of the original motivation to support granite-vision in fms.

I think part of landing the model to FMS is having this fixed, right? Why do we want to land a broken model in FMS in the first place to then fix it later? If there's a good reason to not wait on that we can make an exception

Hmm, no reason, I thought we let the HF impl equivalence variant in first, so that others from the team can focus on modifying it to remove the graph break and add alternate ops for AIU. But we can work off of the branch and merge it all together if you prefer.

I think if we're careful with tensor arithmetic, all of this can be rewritten to not have graph breaks. Let's set up a short discussion offline to figure it out.

in case we can't figure it out, and given you already have a custom function hook for generate(), the call to image_size_to_num_patches can be moved there and outside of the forward() pass, which will remove the graph break, and then we change the inputs to forward from image_sizes to image_num_patches

Yup, I believe so too. First pass was HF equivalence, followed by graph break removal, followed by alternative ops for AIU.

Good idea regarding the custom function hook fallback, will keep in mind!

I moved image feature generation outside forward() to bypass unsupported op, and now no graph breaks either, as you predicted :)

this can be rewritten without a single loop

can you rewrite this using modular arithmetic?

Done!

does this also cause a graph break?

Interesting, I know this does, but the if comparison inside unpad_image might also cause one. Will verify with the next pass targeting graph break removal.

should this be config.vision_config?

Nah, Siglip uses text + vision, and fms support is only for vision (since granite-vision only uses the vision encoder of Siglip, without it's text part, using granite for the latter). So sending in vision here is fine, processed appropriately in line 215

even if infer_common_params is True, some models lack src_vocab_size right?

That was only true for Siglip, but I took care of it in its section itself.

same comment as here: https://github.com/foundation-model-stack/foundation-model-stack/pull/420/files#r2211465597

Fixed

to match other fms models, can we make this the inputs, and remove inputs_embeds

Done

question: are pixel_values constant (same value) for all inferences ? if so, then the input_embeds can be precomputed. This will eliminate the "masked_scatter" in the forward method which is difficult to support in Spyre... we can talk offline, but I checked implementations and they are complicated (even if the op is decomposed).

Great point, thanks @mserranos! Will work on it in the subsequent PR alongwith addressing the graph breaks.

better if you put it in this current PR instead of a subsequent PR, could you elaborate about the graph break problem that you are seeing ?

There is one here, and potentially another one in this function.

There might be more. We decided to fix them in a follow-up PR.

@sahilsuneja1 if there are graph breaks in torch._dynamo then this PR is not useful for Spyre, I suggest you think about how to fix those breaks in this PR along with the "masked_scatter" replacement. We can talk offline, I would like to understand more this code. Have you tried torch 2.7.1 or the upcoming torch 2.8.0 to be released soon ?

Ok.
This code is a first pass mapping of HF transformers impl to make granite-vision run on fms + gpu.
Haven't tested with torch 2.7.1 or 2.8.0.
Haven't added support for AIU yet-- that was supposed to be a follow-up PR.

We can hold off if you want everything as part of this PR itself

yes, I looked at the code where the graph break happens, it is part of the "pack_image_features", If the input_embeds are precomputed as I suggested then that graph break will disappear. I have not yet looked at the second potential graph break.

I modified as per your and Antoni's suggestion and now no graph breaks and unsupported ops :) Thank you!

Do we need a separate param for this? Could we just have input_ids be input_ids_or_embeds

agree with @JRosenkranz here

👍

Can this and the vision config be inside config in line 88?