Add support for ICU tokenizer #61
Conversation
| super().__init__(probability) | ||
|
|
||
| self.template = template | ||
| self.custom_detok_src = custom_detok_src |
There was a problem hiding this comment.
This is kind of brittle as an unstructured string. I think it would be better to parse the string and validate it first in case bad values are passed in. I would suggest splitting on : and optionally handling backwards compatible values.
There was a problem hiding this comment.
it's validated in the next line inside make_detokenizer which does the splitting and checks that the key is in a dictionary:
self.src_retokenizer = Retokenizer(
detokenizer=make_detokenizer(custom_detok_src) ...
There was a problem hiding this comment.
There is some validation, yes, but it's not ergonomic to use it afterwards since you are dealing with raw string manipulation rather than working with a structured class.
| def tokenize(self, text:str) -> Tuple[TokenList, TokenSpanList]: | ||
| from icu import BreakIterator, Locale | ||
|
|
||
| bi = BreakIterator.createWordInstance(Locale(self.lang)) |
There was a problem hiding this comment.
This is an issue as it's slow to create the BreakIterator. It should be created once and cached, probably in the constructor.
There was a problem hiding this comment.
Based on the following code it's a stateful object and it should be created for every string, so it's correct to initialize it here. A user makes a tokenizer object once and then tokenizes many strings with it.
bi.setText(text)
tokens = []
start = bi.first()
for end in bi:
token = text[start:end]Also, there are no issues with performance. It's super fast, unlike Moses tokenizer.
There was a problem hiding this comment.
I still think it should be cached given how these things work, but if it's fast enough currently it's probably not a big deal. I think you setText each time you use it on a cached one is the design principle for ICU things.
eu9ene
force-pushed
the
icu_tokenizer
branch
from
76ce441 to
554b720
Compare
Add support for ICU-based detokenization for Tag inline noise augmentation modifier.
The tokenizer is:
Simple reconstruction of the original text makes maintenance of detokenization trivial compared to the rules in Sacremoses python tokenizer.
It's being used and tested in Firefox Translation (related PR, model training test run). Based on the test run it looks working properly.
See the related discussion for an explanation of why it's useful.