[Bugfix] Fix missing post_layernorm in CLIP

[DarkLight1337]

CLIP encoder currently assumes that len(self.encoder.layers) < config.num_hidden_layers which may not always be the case.

This is a follow-up to #8106.

It additionally fixes a potential issue in SigLIP where the weights are not sharded like in CLIP. cc @Isotr0py Fixed in #8269

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can do one of these:

• Add ready label to the PR
• Enable auto-merge.

🚀

[Isotr0py]

The modification of siglip loading logic looks good to me.

BTW, are the disabled "TIGER-Lab/Mantis-8B-siglip-llama3" model test also related to this post_layernorm issue? I saw it was disabled before.

vllm/tests/models/test_llava.py

Lines 23 to 27 in d331156

	models = [
	"llava-hf/llava-1.5-7b-hf",
	# TODO: Get this model to produce meaningful output in vLLM
	# "TIGER-Lab/Mantis-8B-siglip-llama3",
	]

[DarkLight1337]

The modification of siglip loading logic looks good to me.

BTW, are the disabled "TIGER-Lab/Mantis-8B-siglip-llama3" model test also related to this post_layernorm issue? I saw it was disabled before.

vllm/tests/models/test_llava.py

Lines 23 to 27 in d331156

	models = [
	"llava-hf/llava-1.5-7b-hf",
	# TODO: Get this model to produce meaningful output in vLLM
	# "TIGER-Lab/Mantis-8B-siglip-llama3",
	]

Yes, that's how I discovered it. However even after fixing the weight loading, the model cannot be run because of the special preprocessing they use. It will be addressed in another PR.

[ywang96]

Overall LGTM but I left some comments! PTAL

[litianjian]

@DarkLight1337 "len(self.encoder.layers) < config.num_hidden_layers" may not always determine skip the "post_layer_norm" . If model uses the first-last hidden states and skip the "post_layer_norm", int this case "len(self.encoder.layers) == config.num_hidden_layers".

[DarkLight1337]

@DarkLight1337 "len(self.encoder.layers) < config.num_hidden_layers" may not always determine skip the "post_layer_norm" . If model uses the first-last hidden states and skip the "post_layer_norm", int this case "len(self.encoder.layers) == config.num_hidden_layers".

I see what you mean. Is there anything in the HF config that lets you toggle this behavior on or off though?

[litianjian]

@DarkLight1337 "len(self.encoder.layers) < config.num_hidden_layers" may not always determine skip the "post_layer_norm" . If model uses the first-last hidden states and skip the "post_layer_norm", int this case "len(self.encoder.layers) == config.num_hidden_layers".

I see what you mean. Is there anything in the HF config that lets you toggle this behavior on or off though?

https://huggingface.co/llava-hf/llava-onevision-qwen2-7b-ov-hf/blob/main/config.json
I am developing the llava-ov models on vLLM. This config triggers this issue.

[DarkLight1337]

Looks like SigLIP has vision_config.use_vision_head setting. Let me open a PR for this.

[DarkLight1337]

Nevermind, I see that the problem actually comes from how the features are extracted in the LLaVA-OneVision model. I think you may have to modify the existing encoders' forward method to decide whether to apply post layernorm when you develop Llava-OneVision in your PR.

[ywang96]

This seems to get unnecessarily complicated than what it should be. Perhaps we can keep an internal mapping to see which VLM's vision encoders actually use the post layernorm instead of relying on the number of hidden layers as a proxy?

[DarkLight1337]

This seems to get unnecessarily complicated than what it should be. Perhaps we can keep an internal mapping to see which VLM's vision encoders actually use the post layernorm instead of relying on the number of hidden layers as a proxy?

We have several cases:

1. The output state of an intermediate layer is used without layer normalization
2. The output state of the last layer is used without layer normalization
3. The output state of the last layer is used with layer normalization

Just from the config, we can only distinguish between case (1) and case (2, 3). We could add another parameter to the vision encoder model to specify whether to load post_layernorm, which further distinguishes between case (2) and case (3). Since the vision encoder could be initialized from the VLM, this lets each VLM control whether to use post_layernorm.

[ywang96]

This seems to get unnecessarily complicated than what it should be. Perhaps we can keep an internal mapping to see which VLM's vision encoders actually use the post layernorm instead of relying on the number of hidden layers as a proxy?

We have several cases:

1. The output state of an intermediate layer is used without layer normalization
2. The output state of the last layer is used without layer normalization
3. The output state of the last layer is used with layer normalization

Just from the config, we can only distinguish between case (1) and case (2, 3). We could add another parameter to the vision encoder model to specify whether to load post_layernorm, which further distinguishes between case (2) and case (3). Since the vision encoder could be initialized from the VLM, this lets each VLM control whether to use post_layernorm.

Yea, this is also what I meant by an internal mapping (to keep track which models need post layernorm, and which not), or as you suggested this can be just an extra parameter in each model's implementation (but the user won't be able to change it from the config). Either way, this is something vLLM "maintains" instead of relying on the config file.

[litianjian]

This seems to get unnecessarily complicated than what it should be. Perhaps we can keep an internal mapping to see which VLM's vision encoders actually use the post layernorm instead of relying on the number of hidden layers as a proxy?

We have several cases:

1. The output state of an intermediate layer is used without layer normalization
2. The output state of the last layer is used without layer normalization
3. The output state of the last layer is used with layer normalization

Just from the config, we can only distinguish between case (1) and case (2, 3). We could add another parameter to the vision encoder model to specify whether to load post_layernorm, which further distinguishes between case (2) and case (3). Since the vision encoder could be initialized from the VLM, this lets each VLM control whether to use post_layernorm.

Yea, this is also what I meant by an internal mapping (to keep track which models need post layernorm, and which not), or as you suggested this can be just an extra parameter in each model's implementation (but the user won't be able to change it from the config). Either way, this is something vLLM "maintains" instead of relying on the config file.

Could you please update me on the progress? This will trigger bugs in LLaVA models with multi videos/images input.

[DarkLight1337]

This seems to get unnecessarily complicated than what it should be. Perhaps we can keep an internal mapping to see which VLM's vision encoders actually use the post layernorm instead of relying on the number of hidden layers as a proxy?

We have several cases:

1. The output state of an intermediate layer is used without layer normalization
2. The output state of the last layer is used without layer normalization
3. The output state of the last layer is used with layer normalization

Just from the config, we can only distinguish between case (1) and case (2, 3). We could add another parameter to the vision encoder model to specify whether to load post_layernorm, which further distinguishes between case (2) and case (3). Since the vision encoder could be initialized from the VLM, this lets each VLM control whether to use post_layernorm.

Yea, this is also what I meant by an internal mapping (to keep track which models need post layernorm, and which not), or as you suggested this can be just an extra parameter in each model's implementation (but the user won't be able to change it from the config). Either way, this is something vLLM "maintains" instead of relying on the config file.

Could you please update me on the progress? This will trigger bugs in LLaVA models with multi videos/images input.

This should be fixed already Sorry, just checked and it's not solved yet.

[DarkLight1337]

Can you open a new issue so it can be easily tracked? Thanks and sorry for the delay, we have been quite swamped lately.

[litianjian]

Can you open a new issue so it can be easily tracked? Thanks and sorry for the delay, we have been quite swamped lately.

Sure. Thanks.

[litianjian]

Can you open a new issue so it can be easily tracked? Thanks and sorry for the delay, we have been quite swamped lately.

FYI. We can use #9186 to track it.