[WIP] Multi-Lingual Tokenization #147
Conversation
pyproject.toml
Outdated
| @@ -54,7 +54,15 @@ processing = [ | |||
| "tldextract", | |||
| "trafilatura>=1.8.0", | |||
| "tokenizers", | |||
| "ftfy" | |||
| "ftfy", | |||
| "stanza", | |||
There was a problem hiding this comment.
I suggest to keep only necessary dependency for Korean and also only code to support Korean besides English for now. It will make it easier to review the PR and we can easily add more Languages once the structure is agreed on.
| } | ||
|
|
||
|
|
||
| def get_word_tokenizer(language: str): |
There was a problem hiding this comment.
This code is currently dead code. Can we add a test that guarantees that the behavior for English does not change and then replace:
from nltk.tokenize import word_tokenize
words = word_tokenize(doc.text)
with automatic language specific tokenisation?
There was a problem hiding this comment.
Currently, the tests in tests/pipeline/test_filters.py don't include language metadata. Including automatic language specific tokenization in the filters makes the tests fail. Should we also modify the tests to support the change?
|
@vsabolcec @beme248 please let me know when you are ready for me to review |
| text = doc.text | ||
| words = word_tokenize(text) # TODO we should use language id filter | ||
| language = doc.metadata.get("language", "en") |
There was a problem hiding this comment.
To keep backward compatibility, English tokenizer is used if no language metadata is provided. We could also use multi-language tokenizer from Spacy or require that language metadata is present.
| def test_english_tokenizer(self): | ||
| nltk_words = word_tokenize(SAMPLE_TEXT, language="english") | ||
| tokenizer_words = default_tokenizer.tokenize(SAMPLE_TEXT, language=Languages.english) | ||
|
|
||
| self.assertEqual(nltk_words, tokenizer_words, "NLTK tokenizer and multilingual tokenizer differ") |
There was a problem hiding this comment.
This test ensures that the "old" default tokenizer and "new" default tokenizer produce the same output. Is this test enough to ensure the correctness of the switch or do we also want to add integration tests?
| multilingual = [ | ||
| "spacy", | ||
| ] |
There was a problem hiding this comment.
We separate tokenization dependencies into a new group. In future, as more tokenization dependencies are added, we want to reduce the number of dependencies that the user downloads if they don't need all language tokenizers. Alternatively, tokenization dependencies could be moved to the "processing" group.
|
@guipenedo the PR is ready for review |
There was a problem hiding this comment.
Great work! Some changes:
- move word_tokenizers.py to
utils(toolsis more for CLI tools) - at some point we were planning to load the language from the metadata but have moved a bit to a more explicit approach: users should pass a
languageoption to each block that uses tokenization of some sort. We believe this is less error prone than using the language in the metadata. The default value here should still been - given the above, not sure having the MultilingualTokenizer makes a lot of sense
- instead of a
tokenizeroption being passed to the blocks, users should instead pass thelanguageoption, and there could be aload_tokenizerfunction in word_tokenizers.py that would return the correct tokenizer (and possibly cache it on a dictionary or something so it can reused across blocks) - there are many other blocks using sent_tokenize and word_tokenize but we can change those later
@guipenedo just explained. Approved |
No description provided.