@ggerganov Anything you hate about these changes so far?

Also:https://github.com/ggerganov/llama.cpp/blob/9cc112929c5a798c93d6a34abe53b751a478aa52/gguf-py/gguf/gguf.py#L165-L169

I don't fully understand what this means, but if you explain a bit more I can probably do it. Do you just mean you wantget_tensor_name_map to returnDict[str, Tuple[str, MODEL_TENSOR]] instead ofDict[str, str]?

Think I figured it out and this approach is an improvement. Reduces duplicated code a fair bit.

I guess this could be used in the simpler conversion scripts (the import be moved to top?)
https://github.com/ggerganov/llama.cpp/blob/f55538c3ccba9b926846ef862fa830cea08c433e/convert.py#L338-L339
instead of
https://github.com/ggerganov/llama.cpp/blob/dd0dc366dab10e8df28d3924e7f313b5c695e908/convert-falcon-hf-to-gguf.py#L16-L36

tokens are used by both spm and bpe tokenizers

toktypes are used only by spm tokenizer
scores are used only by spm tokenizer

merges are used only by bpe tokenizer

We should have one class for extracting the spm parts and another for the bpe parts?

The special token mapping (bos eos..) are used by both spm and bpe.
Theclass SpecialVocab should not include the bpemerges only the special token mapping?

The classSpecialVocab should not include the bpe merges only the special token mapping?

How about passing it a flag or something to tell it to leave that part out? Maybe it should be renamed to something more general thanSpecialVocab also (any ideas?)

I was kind of thinking of making it a general class for the non-main vocab stuff. I also wanted to make it so you could access the loaded JSON stuff liketokenizer_config.json because there are some places where that gets loaded twice.

I've implemented some of the suggested changes. I was pretty aggressive about resolving conversations just to try to keep track of the stuff I dealt with. If anyone doesn't like my approach to implementing the suggestions, please don't hesitate to re-open those resolved conversations.

I was kind of thinking of making it a general class for the non-main vocab stuff.

The merges are as important for the function of the bpe tokenizer as the scores are for the spm tokenizer. About 33% higher perplexity without them in both tokenizers.

IMO the script should determinate the tokenizer type and call the toeknizer specific class that will extract only what is needed for that tokenizer to function properly. The special token mapping could be separate.

With --check-untyped-defs, mypy doesn't like that lazy_rebuild_tensor_v2 and rebuild_from_type_v2 are not marked as static methods. We should revert the change made by#1327 and uselazy_rebuild_tensor_v2.__func__ andrebuild_from_type_v2.__func__ in CLASSES to make sure we aren't using unbound methods.

There are still some missing type parameters according to mypy --disallow-any-generics:

convert.py:450: error: Missing type parameters for generic type "ndarray"  [type-arg]    def bf16_to_fp32(bf16_arr: np.ndarray) -> NDArray:    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~convert.py:755: error: Missing type parameters for generic type "Callable"  [type-arg]    ...n], Out], iterable: Iterable[In], concurrency: int, max_workers: Optional[int] = None, factory: Callable = ThreadPool...convert-lora-to-ggml.py:63: error: Missing type parameters for generic type "dtype"  [type-arg]        self, name: str, shape: Sequence[int], data_type: np.dtype        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Aquila model uses the LLaMA architecture and the BPE tokenizer. It should be a good test target for BPE in theconvert.pyscript:
https://huggingface.co/BAAI/Aquila-7B

@cebtenzzre

We should revert the change made by#1327 and uselazy_rebuild_tensor_v2.__func__ andrebuild_from_type_v2.__func__ inCLASSES to make sure we aren't using unbound methods.

edit2: Usinggettattr seems okay as a workaround.

        ('torch._tensor','_rebuild_from_type_v2'):rebuild_from_type_v2.__func__,        ('torch._utils','_rebuild_tensor_v2'):lazy_rebuild_tensor_v2.__func__,

This runs but mypy hates it:

convert.py:678: error: "Callable[[Any, Any, Any, Any], Any]" has no attribute "__func__"  [attr-defined]convert.py:679: error: "Callable[[Any, Any, Any, Any, Any, Any, Any], LazyTensor]" has no attribute "__func__"  [attr-defined]

(I didn't forget to uncomment the decorators.)

edit:python/mypy#11211 andpython/mypy#14123

Open since 2021, so I wouldn't hold my breath. What do you suggest as a workaround?

edit:

convert.py:755: error: Missing type parameters for generic type "Callable"  [type-arg]

I'm not really sure what this one should be. I guess the simplest way to deal with it is just make it a boolean for using threadpool executor or not instead of being able to pass in the executor.

I'm not really sure what this one should be.

It would be nice ifType[Executor] worked here, but the base class doesn't supportmax_workers. Strictly speaking, it's aType[Union[ThreadPoolExecutor, ProcessPoolExecutor]], but that's a bit verbose.

I agree that using a boolean would be the simplest solution.

ce00528 to fix#2865 (comment)

Some naming issues for future model support: All references to BPE should be changed togpt2 since both the SentencePiece tokenizer and the GPT2 tokenizer is in fact BPE aka Byte-Pair-Encoder tokenizers.The tokenizer the original LLaMA model uses is a special version of the original SentencePiece tokenizer, so it should be referenced asllama orllama-spm The Replit models uses another version of the SentencePiece tokenizer and when support for the replit model is added it should be referenced asreplit orreplit-spm.

mypy with--disallow-any-generics and--check-untyped-defs still has one problem withgguf.py:

I'm not sure what's up with that, the first argument seems to matchSupportsRead[bytes] and the second argument seems to matchSupportsWrite[bytes]. You can tack # type: ignore[misc] onto that line and I'll look into filing a mypy issue.
edit: mypy seems to be happy if you replacefout: BinaryIO withfout: BufferedWriter (io.BufferedWriter).
This issue is currently tracked atpython/mypy#15031

It looks like mypy wants two more type annotations:

convert-llama-ggmlv3-to-gguf.py:90: error: Incompatible types in assignment (expression has type "tuple[Any, ...]", variablehas type "tuple[()]")  [assignment]            self.dims = struct.unpack(f'<{n_dims}I', data[offset:offset + (4 * n_dims)])                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~convert-llama-ggmlv3-to-gguf.py:122: error: Need type annotation for "tensors" (hint: "tensors: List[<type>] = ...") [var-annotated]            tensors = []            ^~~~~~~

@klosax

Some naming issues for future model support: All references to BPE should be changed to gpt2 since both the SentencePiece tokenizer and the GPT2 tokenizer is in fact BPE aka Byte-Pair-Encoder tokenizers.

I think I'll leave that to a different pull unless it's unanimous that this should happen and clear what to do. RenamingBpeVocab toGpt2Vocab (I assume) and changing the commandline argument togpt2 wouldn't be an issue. I don't know enough about this to make the decision myself though.

(I also kind of just want to get this merged since it touches so much stuff. Pretty much any change to other scripts is going to create conflicts that I have to mess around with.)

@cebtenzzre

edit: mypy seems to be happy if you replace fout: BinaryIO with fout: BufferedWriter (io.BufferedWriter).

Are you referring to theshutil.copyfileobj thing? If so, the issue I saw was referring to argument 1, not argument 2 (fout)

convert-llama-ggmlv3-to-gguf.py:90: error: Incompatible types in assignment

convert-llama-ggmlv3-to-gguf.py just doesn't have type annotations at all. I saw those warnings but I don't think it really makes sense to add just one or two annotations in a script that has none. (Also might be opening a can of worms by doing that.)

If I get a chance, I'll look at that in a different pull.

Are you referring to theshutil.copyfileobj thing? If so, the issue I saw was referring to argument 1, not argument 2 (fout)

It's talking abouttype argument one - the single TypeVar that it's trying to resolve between the two file arguments. ForSupportsRead[AnyStr] andSupportsWrite[AnyStr], it needs to resolveAnyStr tobytes.

It's talking abouttype argument one - the single TypeVar that it's trying to resolve between the two file arguments.

This is why they call you Massive Brain Cebtenzzre.

@KerfuffleV2 I'm wondering how we properly stage these changes vs rolling out a new gguf pip package release?

As it is right now, e.g theconvert-falcon-hf-to-gguf.py expectedly fails with:

Traceback (most recent call last):  File ".\convert-falcon-hf-to-gguf.py", line 169, in <module>    special_vocab = gguf.SpecialVocab(dir_model, load_merges = True)AttributeError: module 'gguf' has no attribute 'SpecialVocab'

(because the conversion script doesn't usegguf from this repo, rather it imports the package)

Can we publish that update ASAP? I saw@monatis added a workflow for this in#2896

because the conversion script doesn't usegguf from this repo

FWIW, there is nothing stopping you from cd'ing to gguf-py and runningpip install -e .. That would use the gguf from this repo, and-e will make sure it automatically updates when yougit pull.

@akawrykow I already made a manual release, and you can upgrade your gguf package withpip install --upgrade gguf. But you are advised to install it in editable mode as@cebtenzzre pointed out. With that said, any future change should be released with the workflow I described ingguf-py/README.md from now on.

@KerfuffleV2@monatis after upgrading, I get:

Traceback (most recent call last):  File ".\convert-falcon-hf-to-gguf.py", line 4, in <module>    import gguf  File "C:\Python38\lib\site-packages\gguf\__init__.py", line 1, in <module>    from .gguf import *  File "C:\Python38\lib\site-packages\gguf\gguf.py", line 425, in <module>    class GGUFWriter:  File "C:\Python38\lib\site-packages\gguf\gguf.py", line 435, in GGUFWriter    temp_file: Optional[tempfile.SpooledTemporaryFile[bytes]] = NoneTypeError: 'type' object is not subscriptable

any ideas?

Seems like my python version is outdated (currently on 3.8) and I guess this requires 3.9 and up?

edit: working now. Thanks

Python 3.8 isn't EOL yet. Should we addfrom __future__ import annotations to files with annotations so python doesn't try to interpret them at runtime?

Does this fix the error?

temp_file:Optional[tempfile.SpooledTemporaryFile]=None

We should support python 3.8 definitely. I'll check type hints tomorrow and make sure that it works with 3.8.

Does this fix the error?

mypy won't like that because SpooledTemporaryFile is generic in the type stubs. As an alternative to the __future__ import we could do this:

temp_file:'Optional[tempfile.SpooledTemporaryFile[bytes]]'=None

edit: another problem is that typing.TypeAlias does not exist in Python 3.8. We should make the import conditional on TYPE_CHECKING.

I'm wondering how we properly stage these changes vs rolling out a new gguf pip package release?

The current behavior is pretty unintuitive in my behavior. I really feel like if I'm running these scripts from the repo directory it should use thegguf module that's from the repo without having to mess around with virtual environments and stuff. Probably a lot of people also don't know about thepip install -e trick Cebtenzzre mentioned (I didn't).

What if we did something like

importsys,osifos.environ.get('NO_LOCAL_GGUF')isNoneandPath('gguf-py').is_dir():sys.path.insert(1,str(Path('gguf-py')/'gguf'))importgguf

in the scripts that usegguf? That way it'll find the local one if running from the repo root and can also be disabled if necessary.

edit:#2927

@KerfuffleV2 What do you think about the comments I've made above?