Summary
I found two issues with the type annotations. Both are easy to understand and can be fixed easily.
The first type annotation issue
I make this mistake all the time, so I recognize it from my own experience. The intent of the PreTokenizedInputSequence type annotation is to describe the data type of a function/method parameter that must be a list or tuple of strings. The mistake is that the correct type annotation for an arbitrary length tuple containing only strings is actually Tuple[str, ...], not Tuple[str]. You can read about this in the official documentation here: https://typing.python.org/en/latest/spec/tuples.html.
Here is the code as it is today:
|
PreTokenizedInputSequence = Union[List[str], Tuple[str]] |
And below is the surgical fix. This will allow correct programs to pass type checking when they pass tuples of strings into tokenizers:
PreTokenizedInputSequence = Union[List[str], Tuple[str, ...]]
If you don't care about the difference between list and tuple, then you can often accept any sequence type like this:
# At the top of the file:
from collections.abc import Sequence
# ...
PreTokenizedInputSequence = Sequence[str]
How this problem affects users
Say a user wants to:
- Tokenize pre-tokenized text.
- Pass in the pre-tokenized text as a tuple of strings.
- Use only code that passes Mypy type checking.
Then they can only do this because the typing issue above is masked by the second typing issue in tokenizers/__init__.pyi. See below.
The second typing issue that masks the first
In this code, the sequence argument has the annotation Any, which is not correct and not consistent with the doc string below that on line 948 that indicates the type should be tokenizers.InputSequence. The value of Any is overly permissive and masks the issue above.
Here is the code as it is today:
|
self, /, sequence: Any, pair: Any | None = None, is_pretokenized: bool = False, add_special_tokens: bool = True |
And here is the fix:
self, /, sequence: InputSequence, pair: Any | None = None, is_pretokenized: bool = False, add_special_tokens: bool = True
How this second typing issue affects users
Here is an example script with an obvious type issue:
# fail.py
from math import pi
from tokenizers import Tokenizer
tokenizer = Tokenizer.from_pretrained("bert-base-uncased")
tokenizer.encode(pi)As expected, this fails with a TypeError:
❯ uvx --with tokenizers==0.23.1 python fail.py
Traceback (most recent call last):
File "/home/james/temp.py", line 4, in <module>
tokenizer.encode(pi)
~~~~~~~~~~~~~~~~^^^^
TypeError: TextInputSequence must be strThe second typing issue prevents Mypy from identifying this problem:
❯ uvx --with tokenizers==0.23.1 mypy temp.py
Success: no issues found in 1 source file
Summary
I found two issues with the type annotations. Both are easy to understand and can be fixed easily.
The first type annotation issue
I make this mistake all the time, so I recognize it from my own experience. The intent of the
PreTokenizedInputSequencetype annotation is to describe the data type of a function/method parameter that must be a list or tuple of strings. The mistake is that the correct type annotation for an arbitrary length tuple containing only strings is actuallyTuple[str, ...], notTuple[str]. You can read about this in the official documentation here: https://typing.python.org/en/latest/spec/tuples.html.Here is the code as it is today:
tokenizers/bindings/python/py_src/tokenizers/__init__.py
Line 21 in 2ab6430
And below is the surgical fix. This will allow correct programs to pass type checking when they pass tuples of strings into
tokenizers:If you don't care about the difference between list and tuple, then you can often accept any sequence type like this:
How this problem affects users
Say a user wants to:
Then they can only do this because the typing issue above is masked by the second typing issue in
tokenizers/__init__.pyi. See below.The second typing issue that masks the first
In this code, the
sequenceargument has the annotationAny, which is not correct and not consistent with the doc string below that on line 948 that indicates the type should betokenizers.InputSequence. The value ofAnyis overly permissive and masks the issue above.Here is the code as it is today:
tokenizers/bindings/python/py_src/tokenizers/__init__.pyi
Line 930 in 2ab6430
And here is the fix:
How this second typing issue affects users
Here is an example script with an obvious type issue:
As expected, this fails with a TypeError:
The second typing issue prevents Mypy from identifying this problem: