[RoFormer] Fix some issues #12397

JunnYu · 2021-06-28T14:52:46Z

What does this PR do?

add RoFormerTokenizerFast into AutoTokenizer
fix typo in roformer docs
Fix Exporting the operator repeat_interleave to ONNX opset version (<=12) is not supported! #12000 and make onnx export happy
update RoFormerConfig embedding_size
use jieba not rjieba and then we can enjoy "Hosted inference API in huggingface.co"
fix RoFormerTokenizerFast has a wrong result when setting "return_offsets_mapping=True" #12244 and make test_alignement passed
update roformer ARCHIVE_MAP

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@patil-suraj

NielsRogge

Thanks for these fixes. 2 main comments:

your PR updates the utils_qa.py file of the Tensorflow examples. Any reason why this is the case?
the jieba dependency of the tokenizer should be implemented in file_utils.py and testing_utils.py

NielsRogge · 2021-07-01T15:53:17Z

examples/tensorflow/question-answering/utils_qa.py

-    is_world_process_zero: bool = True,
+    log_level: Optional[int] = logging.WARNING,


Is there a reason you updated this? Same question for the lines below

Don't think this RoFormer PR needs to update the utils_qa.py file of the Tensorflow examples?

I forked transformers before this pr 276bc14.
And this pr do not run command fix-copies.

I run command fix-copies and then this file examples/tensorflow/question-answering/utils_qa.py change.

NielsRogge · 2021-07-02T08:21:43Z

src/transformers/models/roformer/tokenization_roformer.py

-            import rjieba
+            import jieba
        except ImportError:
            raise ImportError(
-                "You need to install rjieba to use RoFormerTokenizer."
-                "See https://pypi.org/project/rjieba/ for installation."
+                "You need to install jieba to use RoFormerTokenizer."
+                "See https://pypi.org/project/jieba/ for installation."
            )
-        self.jieba = rjieba
+        self.jieba = jieba


Is this the correct way of handling the jieba dependency @LysandreJik?

We decided to handle it this way as it's the only model that requires it - and if other models arrive, then to upstream it like it is done for the other models.

NielsRogge · 2021-07-02T08:23:36Z

src/transformers/models/roformer/tokenization_roformer.py

        try:
-            import rjieba
+            import jieba
        except ImportError:
            raise ImportError(
-                "You need to install rjieba to use RoFormerTokenizer."
-                "See https://pypi.org/project/rjieba/ for installation."
+                "You need to install jieba to use RoFormerTokenizer."
+                "See https://pypi.org/project/jieba/ for installation."
            )
-        self.jieba = rjieba
+        self.jieba = jieba


If you already have the try: except block at the init of the tokenizer, is this required here?

NielsRogge · 2021-07-02T08:28:14Z

tests/test_tokenization_roformer.py

-def is_rjieba_available():
-    return importlib.util.find_spec("rjieba") is not None
+def is_jieba_available():
+    return importlib.util.find_spec("jieba") is not None


-def require_rjieba(test_case):
+def require_jieba(test_case):
    """
    Decorator marking a test that requires Jieba. These tests are skipped when Jieba isn't installed.
    """
-    if not is_rjieba_available():
-        return unittest.skip("test requires rjieba")(test_case)
+    if not is_jieba_available():
+        return unittest.skip("test requires jieba")(test_case)
    else:
        return test_case


is_jieba_available() should not be defined within the test file. Rather, it should be defined in the file_utils.py file, as can be seen here.

require_jieba should be defined in testing_utils.py instead of here. You can check how this was done for the timm library as an example here.

Thanks, I have changed this :) .

NielsRogge · 2021-07-02T08:30:33Z

examples/tensorflow/question-answering/utils_qa.py

-        is_world_process_zero (:obj:`bool`, `optional`, defaults to :obj:`True`):
-            Whether this process is the main process or not (used to determine if logging/saves should be done).
+        log_level (:obj:`int`, `optional`, defaults to ``logging.WARNING``):
+            ``logging`` log level (e.g., ``logging.WARNING``)


Same comment here.

NielsRogge · 2021-07-02T08:30:44Z

examples/tensorflow/question-answering/utils_qa.py

-    logger.setLevel(logging.INFO if is_world_process_zero else logging.WARN)
+    logger.setLevel(log_level)


Same comment here.

NielsRogge · 2021-07-02T08:30:56Z

examples/tensorflow/question-answering/utils_qa.py

-        print(f"Saving predictions to {prediction_file}.")
+        logger.info(f"Saving predictions to {prediction_file}.")
        with open(prediction_file, "w") as writer:
            writer.write(json.dumps(all_predictions, indent=4) + "\n")
-        print(f"Saving nbest_preds to {nbest_file}.")
+        logger.info(f"Saving nbest_preds to {nbest_file}.")
        with open(nbest_file, "w") as writer:
            writer.write(json.dumps(all_nbest_json, indent=4) + "\n")
        if version_2_with_negative:
-            print(f"Saving null_odds to {null_odds_file}.")
+            logger.info(f"Saving null_odds to {null_odds_file}.")


Same comment here.

NielsRogge · 2021-07-02T08:31:04Z

examples/tensorflow/question-answering/utils_qa.py

-    logger.setLevel(logging.INFO if is_world_process_zero else logging.WARN)
+    logger.setLevel(log_level)


Same comment here.

NielsRogge · 2021-07-02T08:31:12Z

examples/tensorflow/question-answering/utils_qa.py

-    is_world_process_zero: bool = True,
+    log_level: Optional[int] = logging.WARNING,


Same comment here.

NielsRogge · 2021-07-02T08:31:19Z

examples/tensorflow/question-answering/utils_qa.py

-        is_world_process_zero (:obj:`bool`, `optional`, defaults to :obj:`True`):
-            Whether this process is the main process or not (used to determine if logging/saves should be done).
+        log_level (:obj:`int`, `optional`, defaults to ``logging.WARNING``):
+            ``logging`` log level (e.g., ``logging.WARNING``)


Same comment here.

NielsRogge · 2021-07-02T11:50:51Z

src/transformers/models/roformer/tokenization_roformer.py

+        import jieba
+
+        self.jieba = jieba


Same question here for @LysandreJik.

Woul dbe nice to leave the try/except statement

NielsRogge · 2021-07-02T11:53:05Z

Ok thanks, I just will let Lysandre review the try:except block, that's the only thing remaining.

LysandreJik

This looks good, but why did you change rjieba to jieba? Is the latter better?

src/transformers/models/roformer/configuration_roformer.py

LysandreJik · 2021-07-05T08:55:11Z

src/transformers/models/roformer/tokenization_roformer.py

-            import rjieba
+            import jieba
        except ImportError:
            raise ImportError(
-                "You need to install rjieba to use RoFormerTokenizer."
-                "See https://pypi.org/project/rjieba/ for installation."
+                "You need to install jieba to use RoFormerTokenizer."
+                "See https://pypi.org/project/jieba/ for installation."
            )
-        self.jieba = rjieba
+        self.jieba = jieba


We decided to handle it this way as it's the only model that requires it - and if other models arrive, then to upstream it like it is done for the other models.

LysandreJik · 2021-07-05T08:55:32Z

src/transformers/models/roformer/tokenization_roformer.py

+        import jieba
+
+        self.jieba = jieba


Woul dbe nice to leave the try/except statement

Co-authored-by: Lysandre Debut <lysandre@huggingface.co>

JunnYu · 2021-07-05T10:01:40Z

This looks good, but why did you change rjieba to jieba? Is the latter better?

@LysandreJik
I want to use Hosted inference API in https://huggingface.co.

I found CpmTokenizer and XLMTokenizer use jieba.

transformers/src/transformers/models/cpm/tokenization_cpm.py

Line 31 in fb41f9f

class CpmTokenizer(XLNetTokenizer):

transformers/src/transformers/models/xlm/tokenization_xlm.py

Line 530 in b24ead8

class XLMTokenizer(PreTrainedTokenizer):

LysandreJik · 2021-07-05T13:52:13Z

cc @Narsil

Narsil · 2021-07-05T14:00:38Z

@JunnYu Should work now:

https://huggingface.co/junnyu/roformer_chinese_base?text=%E7%94%9F%E6%B4%BB%E7%9A%84%E7%9C%9F%E8%B0%9B%E6%98%AF+%5BMASK%5D%E3%80%82

We do update the API regularly with dependencies, rjiebawas added pretty recently.
Cheers !

JunnYu · 2021-07-05T16:14:20Z

@Narsil thank you!

JunnYu · 2021-07-05T16:27:16Z

@LysandreJik
(1) Now I use rust jieba .
(2) This pr #12361 add test_training_new_tokenizer and test_training_new_tokenizer_with_special_tokens_change.
I found test_tokenization_roformer.py can't pass these two tests. Because roformer tokenizer has a custom PreTokenizer.

transformers/src/transformers/convert_slow_tokenizer.py

Line 318 in e2c1dd0

    
           tokenizer.pre_tokenizer = pre_tokenizers.PreTokenizer.custom(JiebaPreTokenizer(vocab))

LysandreJik

Great, LGTM! Thanks @JunnYu!

JunnYu and others added 13 commits June 28, 2021 21:55

add RoFormerTokenizerFast into AutoTokenizer

4b6088a

fix typo in roformer docs

d2f2bea

make onnx export happy

d96a2a0

update RoFormerConfig embedding_size

bae3675

use jieba not rjieba

8175b1a

fix 12244 and make test_alignement passed

c0daacd

update ARCHIVE_MAP

72c3095

make style & quality & fixup

3c02106

update

aa14de9

make style & quality & fixup

23c723a

Merge branch 'huggingface:master' into fix-some-roformer-issues

9490c0a

make style quality fixup

bbf0e0e

Merge branch 'huggingface:master' into fix-some-roformer-issues

f986f17

NielsRogge reviewed Jul 2, 2021

View reviewed changes

JunnYu and others added 2 commits July 2, 2021 18:09

Merge branch 'huggingface:master' into fix-some-roformer-issues

ef2d46e

update

90102cb

JunnYu requested a review from NielsRogge July 2, 2021 10:24

NielsRogge reviewed Jul 2, 2021

View reviewed changes

LysandreJik approved these changes Jul 5, 2021

View reviewed changes

suggestion from LysandreJik

1c5dd6a

Co-authored-by: Lysandre Debut <lysandre@huggingface.co>

make style

1f08622

use rjieba

52eb0f5

LysandreJik approved these changes Jul 6, 2021

View reviewed changes

LysandreJik merged commit 626a0a0 into huggingface:master Jul 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RoFormer] Fix some issues #12397

[RoFormer] Fix some issues #12397

JunnYu commented Jun 28, 2021

NielsRogge left a comment •

edited

Loading

NielsRogge Jul 1, 2021 •

edited

Loading

JunnYu Jul 2, 2021

NielsRogge Jul 2, 2021 •

edited

Loading

LysandreJik Jul 5, 2021

NielsRogge Jul 2, 2021 •

edited

Loading

NielsRogge Jul 2, 2021

JunnYu Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

NielsRogge Jul 2, 2021

LysandreJik Jul 5, 2021

NielsRogge commented Jul 2, 2021

LysandreJik left a comment

LysandreJik Jul 5, 2021

LysandreJik Jul 5, 2021

JunnYu commented Jul 5, 2021 •

edited

Loading

LysandreJik commented Jul 5, 2021

Narsil commented Jul 5, 2021

JunnYu commented Jul 5, 2021

JunnYu commented Jul 5, 2021

LysandreJik left a comment

		is_world_process_zero: bool = True,
		log_level: Optional[int] = logging.WARNING,

		logger.setLevel(logging.INFO if is_world_process_zero else logging.WARN)
		logger.setLevel(log_level)

[RoFormer] Fix some issues #12397

[RoFormer] Fix some issues #12397

Conversation

JunnYu commented Jun 28, 2021

What does this PR do?

Before submitting

Who can review?

NielsRogge left a comment • edited Loading

Choose a reason for hiding this comment

NielsRogge Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NielsRogge commented Jul 2, 2021

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunnYu commented Jul 5, 2021 • edited Loading

LysandreJik commented Jul 5, 2021

Narsil commented Jul 5, 2021

JunnYu commented Jul 5, 2021

JunnYu commented Jul 5, 2021

LysandreJik left a comment

Choose a reason for hiding this comment

NielsRogge left a comment •

edited

Loading

NielsRogge Jul 1, 2021 •

edited

Loading

NielsRogge Jul 2, 2021 •

edited

Loading

NielsRogge Jul 2, 2021 •

edited

Loading

JunnYu commented Jul 5, 2021 •

edited

Loading