mistral-common really shouldn't be a mandatory dependency (discuss?)

[LostRuins]

Hello @CISC @MaggotHATE @slaren and @juliendenize (tagged due to prior related discussion).

I recognize that Mistral has been... enthusiastically... promoting the usage of mistral-common, and I am sure many people indeed find that library helpful.

However in my opinion, I don't think it's correct to mandate its installation for all users. For example, there might be users who wish to simply convert their Qwen, Gemma or GLM-Air model, who are now forced to install mistral-common ever since #14737 was merged.

For a practical example, @TheLocalDrummer encounters the same issue here: #15420 (comment)

Additionally, I also point you to the concerns raised by u/dobomex761604 and various redditors in this comment chain: https://www.reddit.com/r/LocalLLaMA/comments/1njgovj/magistral_small_2509_has_been_released/neq8w8p/

Ideally this dependency will not be mandated, and a llama.cpp community solution preferred, but failing that, perhaps we can consider placing it behind a try/except block so others who do not wish to use Mistral models are not burdened by this limitation.

try:
    from mistral_common.tokens.tokenizers.base import TokenizerVersion
    from mistral_common.tokens.tokenizers.multimodal import DATASET_MEAN, DATASET_STD
    from mistral_common.tokens.tokenizers.tekken import Tekkenizer
    from mistral_common.tokens.tokenizers.sentencepiece import (
        SentencePieceTokenizer,
    )
except Exception:
    print("Warning: No Mistral Common Installed. You cannot convert Mistral models.")

Alternative suggestions are welcome. I do like and use Mistral models, but I don't think they should be given special treatment in this project.

[CISC]

While I fully agree it should not be a required dependency it's not as simple as a try/except as that will completely break all Mistral codepaths. It will also anger the typing gods, and break our CI. :)

For the sake of simplicity the PR was accepted as-is today, and the burden of another dependency deemed low enough.

A PR that can reasonably extract the requirement without too much fuss will be accepted though.

[TheLocalDrummer]

IIRC, everything was fine prior to the Mistral patch ('patch' used loosely here). I suggest we revert the PR and call it a day.

[MaggotHATE]

Hi! I also want to point out that name of that PR clearly shows the intent of Mistral to simply use llama.cpp for their causes, including popularizing and forcing the use of mistral-common, but maybe even more in future - like gating future models behind their own library. However, it's important to remember that llama.cpp is first of all a C++ project.

@CISC I remember that you were most active in reviewing both those PRs, can you please state if you have any affiliation with Mistral?

[CISC]

I remember that you were most active in reviewing both those PRs, can you please state if you have any affiliation with Mistral?

I can conclusively say that I do not have any affiliation with Mistral, as you might have guessed from my comments in those PRs...

[MaggotHATE]

I remember that you were most active in reviewing both those PRs, can you please state if you have any affiliation with Mistral?

I can conclusively say that I do not have any affiliation with Mistral, as you might have guessed from my comments in those PRs...

And yet it was merged.

#14737 is the most important in this case and should've never been accepted in that state. I really don't understand why it was left in such state.

[MaggotHATE]

This is where the mistake was made, it seems.

[CISC]

This is where the mistake was made, it seems.

@ngxson points are completely valid, it was not a mistake from a development perspective.

I understand your grief about the added requirement, and as already stated if you can come up with an adequate solution it will be accepted.

[MaggotHATE]

This is where the mistake was made, it seems.

@ngxson points are completely valid, it was not a mistake from a development perspective.

I understand your grief about the added requirement, and as already stated if you can come up with an adequate solution it will be accepted.

Even if those points are valid, it doesn't mean that the dependency issue wasn't overlooked - it should've been at least noted at that time, but was ignored instead.

Now we have a mess because of that.

[MaggotHATE]

it's not as simple as a try/except

Hold on, it was exactly though try/except back before that in Voxtral PR, why not do it again if it was ok back then?

[CISC]

it's not as simple as a try/except

Hold on, it was exactly though try/except back before that in Voxtral PR, why not do it again if it was ok back then?

If you look closely you will see that it took quite a bit of extra code to make that work, and that code is still there! The main difference was that this was touching gguf, where we absolutely do not want the added dependency, while current code only touches the conversion script.

[MaggotHATE]

it's not as simple as a try/except

Hold on, it was exactly though try/except back before that in Voxtral PR, why not do it again if it was ok back then?

If you look closely you will see that it took quite a bit of extra code to make that work, and that code is still there! The main difference was that this was touching gguf, where we absolutely do not want the added dependency, while current code only touches the conversion script.

Then why we want that dependency to be mandatory for the conversion script, again? Yes, it's much less critical, but it's still shouldn't be there that way.

[CISC]

Then why we want that dependency to be mandatory for the conversion script, again? Yes, it's much less critical, but it's still shouldn't be there that way.

Simply cost/benefit, adding all the extra safeguards was not worth it compared to just adding it to requirements-convert_hf_to_gguf.txt as everyone using the conversion script must install them.

[ngxson]

I didn't have time to look at the import, but we can just try the simple solution: move all the import statements to MistralModel class

It's the same idea where we have from transformers import AutoTokenizer added in some places in the code

mistral-common is can also be removed from requirements, there is an example code for checking its presence:

llama.cpp/gguf-py/gguf/vocab.py

Lines 663 to 667 in 28baac9

	if not _mistral_common_installed:
	raise ImportError(
	"To use MistralVocab, please install the `mistral-common` package. "
	"You can install it with `pip install mistral-common`."
	)

I think I assumed them to copy this code over covert_hf_to_gguf in the first place, but turns out it wasn't

[ngxson]

Beside, I think none of the collaborator of llama.cpp has any affiliations with Mistral. New models from Mistral don't usually have day-0 support for this reason (but strangely, ollama does)

[LostRuins]

move all the import statements to MistralModel class

yes that is a good start. ideally all other architectures do not need that class to function anyway