Skip RoFormer ONNX test if rjieba not installed

[lewtun]

What does this PR do?

This PR adds the @require_rjieba decorator to the slow ONNX tests to deal with the following error in our daily CI runs:

(line 164)  ImportError: You need to install rjieba to use RoFormerTokenizer. See https://pypi.org/project/rjieba/ for installation.

I wasn't sure if rjieba should actually be installed in the GitHub workflow, but it doesn't seem to be the case for the RoFormer tests and so I omitted that for now.

Edit: I've added rjieba to the "tests" extras and also tested that the slow ONNX test passes when this dep is installed:

RUN_SLOW=1 pytest tests/onnx/test_onnx_v2.py -s -k "roformer"

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[patrickvonplaten]

So this test is then currently just skipped on our ONNX tests?

Should we maybe not better add rjieba to the test package to the test RoFormer (I couldn't find it in the setup.py or in the Docker file) cc @LysandreJik @sgugger

[sgugger]

It looks like RoFormer tokenization is completely untested yes, so this package should be added in the "testing" extra.

[ydshieh]

I agree we should include rjieba for the tests, unless there are reasons for not adding specific packages.

Further remark

If we could not add rjieba, it is not a good idea to add @require_rjieba for test_pytorch_export, otherwise this test won't be run for other models neither. In this case, I think we might need a specific way to skip this test for RoFormer (and others that require rjieba).

[lewtun]

Thanks for the feedback - I'll add rjieba to our testing suite as well :)

[lewtun]

This test was failing with Exception: Custom PreTokenizer cannot be serialized:

    def test_save_slow_from_fast_and_reload_fast(self):
        if not self.test_slow_tokenizer or not self.test_rust_tokenizer:
            # we need both slow and fast versions
            return
    
        for tokenizer, pretrained_name, kwargs in self.tokenizers_list:
            with self.subTest(f"{tokenizer.__class__.__name__} ({pretrained_name})"):
                with tempfile.TemporaryDirectory() as tmp_dir_1:
                    # Here we check that even if we have initialized a fast tokenizer with a tokenizer_file we can
                    # still save only the slow version and use these saved files to rebuild a tokenizer
                    tokenizer_fast_old_1 = self.rust_tokenizer_class.from_pretrained(
                        pretrained_name, **kwargs, use_fast=True
                    )
                    tokenizer_file = os.path.join(tmp_dir_1, "tokenizer.json")
>                   tokenizer_fast_old_1.backend_tokenizer.save(tokenizer_file)
E                   Exception: Custom PreTokenizer cannot be serialized

Looking at this similar issue huggingface/tokenizers#613 it seems that RoFormer belong to the class of tokenizers that can't be saved with tokenizer.backend_tokenizer.save(). Here's an example to reproduce:

from transformers import AutoTokenizer

tokenizer = AutoTokenizer.from_pretrained("junnyu/roformer_chinese_small", use_fast=True)
tokenizer.backend_tokenizer.save(".")

I'm not very familiar with RoFormer, so happy to debug this further if we expect this test really should pass.

[patrickvonplaten]

This one I think we can definitely skip since this tokenizer cannot be saved in the "fast" format

[lewtun]

This test fails for a different reason - as far as I can tell, saving the fast tokenizer and loading it again fails because vocab_file is None on the init:

src/transformers/tokenization_utils_base.py:1783: in from_pretrained
    return cls._from_pretrained(
src/transformers/tokenization_utils_base.py:1809: in _from_pretrained
    slow_tokenizer = (cls.slow_tokenizer_class)._from_pretrained(
src/transformers/tokenization_utils_base.py:1918: in _from_pretrained
    tokenizer = cls(*init_inputs, **init_kwargs)
src/transformers/models/roformer/tokenization_roformer.py:145: in __init__
    if not os.path.isfile(vocab_file):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

path = None

    def isfile(path):
        """Test whether a path is a regular file"""
        try:
>           st = os.stat(path)
E           TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType

Here's an example to reproduce:

from transformers import AutoTokenizer

tokenizer = AutoTokenizer.from_pretrained("junnyu/roformer_chinese_small", use_fast=True)
tokenizer.save_pretrained("./tmp/roformer", legacy_format=False)
AutoTokenizer.from_pretrained("./tmp/roformer/")

[patrickvonplaten]

I think you discovered a bug here @lewtun !

I've done some digging and I think the bug is that the vocab file is not correctly defined for RoFormer I think.

Could you replace

transformers/src/transformers/models/roformer/tokenization_roformer_fast.py

Line 30 in e6f00a1

VOCAB_FILES_NAMES = {"vocab_file": "vocab.txt"}

by:

VOCAB_FILES_NAMES = {"vocab_file": "vocab.txt", "tokenizer_file": "tokenizer.json"}

This should make your codesnippet above work and hopefully also make the test pass

[lewtun]

Thanks for the advice! Fixed in 1abc58a

With this fix, the following slow tests now all pass:

RUN_SLOW=1 pytest tests/roformer/test_tokenization_roformer.py -s

[lewtun]

Hey @sgugger @patrickvonplaten I'm hitting some peculiar issues with 2 of the slow tests of the RoFormer tokenizer. Would you mind taking a look and seeing if my decision to skip them is valid?

[sgugger]

I'll let @patrickvonplaten decide as I know nothing on that model too :-)

[patrickvonplaten]

Think we can fix 1 test by correcting a bug as mentioned in a comment. Happy to skip the other test and merge afterward :-)

[LysandreJik]

There is a test dedicated to custom tokenizers with specific dependencies: https:/huggingface/transformers/blob/main/.circleci/config.yml#L538

It installs jieba but not rjieba. Would it make sense to add it there? If you're testing for ONNX, it's very likely that it does not make sense as it's limited to tokenizer tests right now.

[lewtun]

There is a test dedicated to custom tokenizers with specific dependencies: https:/huggingface/transformers/blob/main/.circleci/config.yml#L538

It installs jieba but not rjieba. Would it make sense to add it there? If you're testing for ONNX, it's very likely that it does not make sense as it's limited to tokenizer tests right now.

Thanks for the tip! Done in 3cafcb2

[lewtun]

This adds rjieba to the custom tokenizer tests on CircleCI

[lewtun]

Hey @patrickvonplaten @LysandreJik I think this PR is ready for a final pass :)

The failing test is unrelated to the PR itself (a failing Pegasus generate test)

[sgugger]

You'll need to rebase for the move of the test files. Otherwise LGTM!