[Performance] `model_config.compute_hash` is computed every time and introduce overhead in each new multi-modal req

[imkero]

call stack: InputPreprocessor::_process_multimodal -> MultiModalRegistry::create_processor -> MultiModalRegistry::_get_model_cls -> get_model_architecture -> model_config.compute_hash

vllm/vllm/model_executor/model_loader/utils.py

Lines 216 to 224 in c60e613

	def get_model_architecture(
	model_config: ModelConfig) -> tuple[type[nn.Module], str]:
	key = model_config.compute_hash()
	if key in _MODEL_ARCH_BY_HASH:
	return _MODEL_ARCH_BY_HASH[key]
	model_arch = _get_model_architecture(model_config)
	_MODEL_ARCH_BY_HASH[key] = model_arch
	return model_arch

[DarkLight1337]

How much overhead does this introduce compared to getting the model architecture each time?

[DarkLight1337]

I suppose a straightforward way to solve this would be to cache the hash of the model config, and update it whenever the config is mutated.

[DarkLight1337]

I've opened #25682, can you try it out?

[ProExpertProg]

Why is this computed multiple times? Can we not just save the model class or at least model arch?

[DarkLight1337]

The model class is saved in get_model_architecture by associating it with the model config, but we use the hash of the config to determine whether it is already in the cache or not. That's why compute_hash is called each time.

[DarkLight1337]

Basically, compute_hash is the way we save the model class. How do you suggest otherwise?

[ProExpertProg]

In InputPreprocessor::_process_multimodal

    if not hasattr(self, 'mm_processor'):
        self.mm_processor = self.mm_registry.create_processor(
            self.model_config,
            tokenizer=tokenizer,
            cache=self.mm_processor_cache,
        )
    
    # now we just use self.mm_processor instead of mm_processor? 

I'm not sure that works but that''s roughly the idea. Basically I think ModelConfig shouldn't be caching its hash instead the user should cache stuff because the user is also more aware of changes

[ProExpertProg]

To clarify, the only reason I care about this is we're overhauling how hashing is done for compilation: #23134

[DarkLight1337]

I see. I also just realized that we have gotten rid of lora_request so we don't have to worry about tokenizer changing per-request anymore. I'll apply your suggestion then, thanks!

[DarkLight1337]

Actually, I realized that this can't be done until the Renderer refactor (#22880), because we also call self.mm_registry.create_processor inside BaseMultiModalItemTracker which is initialized per request. I'll edit get_model_architecture to use a smaller set of fields instead of using compute_hash instead then.

[ywang96]

Actually, I realized that this can't be done until the Renderer refactor (#22880), because we also call self.mm_registry.create_processor inside BaseMultiModalItemTracker which is initialized per request. I'll edit get_model_architecture to use a smaller set of fields instead of using compute_hash instead then.

@DarkLight1337 But at least the processor inside InputPreprocessor::_process_multimodal can be done, correct?

[DarkLight1337]

Yes, I mean that we can't solve the problem only by changing the call inside InputPreprocessor. So we still need to modify get_model_architecture regardless.