Skip to content

Use protobuf 4 - #24599

Merged
ydshieh merged 8 commits into
mainfrom
bon_courage
Jun 30, 2023
Merged

ydshieh merged 8 commits into
mainfrom
bon_courage

Conversation

@ydshieh

@ydshieh ydshieh commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator

What does this PR do?

I move forward to generate the new src/transformers/utils/sentencepiece_model_pb2.py using the protocol buffer compiler. No test failure but have a quality check fails. I can probably removed the undefined part _TRAINERSPEC which doesn't seem used.

@ydshieh

ydshieh commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator Author

I have checked to make sure protobuf==4.23.3 is installed and used in the CI.

@ydshieh
ydshieh requested review from Narsil and sgugger June 30, 2023 15:10
@ydshieh

ydshieh commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator Author

@Narsil mentioned we should keep the old/new version files, and determine which one to use (by the protobuf version numbers?).

Let me know more details about this please.

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might need to keep the old one in case the user has an older version of protobuf, otherwise the conversion won't work. And then dynamically add either the old protobuf stuff or the new one depending on the version detected.

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Jun 30, 2023 •

Copy link
Copy Markdown

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

@ydshieh
ydshieh marked this pull request as draft June 30, 2023 17:01

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

@ydshieh
ydshieh marked this pull request as ready for review June 30, 2023 18:10
@ydshieh

ydshieh commented Jun 30, 2023

Copy link
Copy Markdown
Collaborator Author

Will merge once the CI for protobuf 4 a nd protobuf 3 both green.

Don't hesitate to leave comments later if any @Narsil 🙏

@ydshieh
ydshieh merged commit 299aafe into main Jun 30, 2023
@ydshieh
ydshieh deleted the bon_courage branch June 30, 2023 18:56
@Narsil

Narsil commented Jul 1, 2023

Copy link
Copy Markdown
Contributor

Great PR. I hope this doesn't have any unforeseen consequence (I don't know what are the breaking changes between protobuf 3 and 4)

@ydshieh

ydshieh commented Jul 1, 2023

Copy link
Copy Markdown
Collaborator Author

I don't know what are the breaking changes between protobuf 3 and 4

Yeah, me neither. I just rely on our CI 🤞

@fxmarty

fxmarty commented Jul 11, 2023 •

Copy link
Copy Markdown
Contributor

@ydshieh I'm now getting

  File "/home/fxmarty/hf_internship/transformers/src/transformers/utils/sentencepiece_model_pb2_new.py", line 16, in <module>
    DESCRIPTOR = _descriptor_pool.Default().AddSerializedFile(
TypeError: Couldn't build proto file into descriptor pool: Invalid default '0.9995' for field sentencepiece.TrainerSpec.character_coverage of type 2

with protobuf=4.23.4 & transformers main when doing from transformers.utils import sentencepiece_model_pb2_new. Any idea what's wrong? protobuf 3.20.3 works well for me.

@ydshieh

ydshieh commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator Author

Hi @fxmarty !

Hmm, I remember I got 4.23.3 when I made this PR. Not sure if it's the reason. Let me check.

@ydshieh

ydshieh commented Jul 11, 2023 •

Copy link
Copy Markdown
Collaborator Author

Hi again

@fxmarty 4.23.4 works for me.

(py39) λ pip show protobuf
Name: protobuf
Version: 4.23.4

Python 3.9.13 (main, Oct 13 2022, 21:23:06) [MSC v.1916 64 bit (AMD64)] :: Anaconda, Inc. on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from transformers.utils import sentencepiece_model_pb2
>>> from transformers.utils import sentencepiece_model_pb2_new
>>>

@ydshieh

ydshieh commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator Author

Hm, there is something wrong and I am looking deeper.

@ydshieh

ydshieh commented Jul 11, 2023 •

Copy link
Copy Markdown
Collaborator Author

OK, PR #24622 makes this failing.

cc @ArthurZucker

@ArthurZucker

ArthurZucker commented Jul 11, 2023 •

Copy link
Copy Markdown
Collaborator

Comes from #24690. More details:
This does not fix the version error, but fixes the issue with 3.20.3, when we cannot use seqio or anything importing protobuf:

! pip install protobuf=3.20.3
from transformers import AutoTokenizer
from seqio import SentencePieceVocabulary
TypeError: Couldn't build proto file into descriptor pool!
Invalid proto descriptor for file "sentencepiece_model.proto":
  sentencepiece_model.proto: A file with this name is already in the pool.

@ydshieh

ydshieh commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator Author

@fxmarty We are not able to reproduce the situation you have. Could you specify your transformers commit that is installed? Thanks.

(The above discussion is another story)

@fxmarty

fxmarty commented Jul 11, 2023 •

Copy link
Copy Markdown
Contributor

@ydshieh It appears I can't reproduce anymore the issue I had. Must have messed something up in my install that I fixed since then. Maybe the pip uninstall protobuf && pip install --no-binary protobuf protobuf==3.20.3 that I used in the meanwhile helped to fix things.

@fxmarty

fxmarty commented Jul 11, 2023 •

Copy link
Copy Markdown
Contributor

@ydshieh Oh, actually I have this issue only running in my scripting IDE pyzo, but not in a terminal. The same python env is used so quite weird.

@fxmarty

fxmarty commented Jul 11, 2023 •

Copy link
Copy Markdown
Contributor

@ArthurZucker @ydshieh How does sentencepiece_model_pb2_new.py define TrainerSpec.character_coverage?

Specifically, how is

b'\n\x19sentencepiece_model.proto\x12\rsentencepiece"\x80\x0c\n\x0bTrainerSpec\x12\r\n\x05input\x18\x01 \x03(\t\x12\x14\n\x0cinput_format\x18\x07 \x01(\t\x12\x14\n\x0cmodel_prefix\x18\x02 \x01(\t\x12\x41\n\nmodel_type\x18\x03 \x01(\x0e\x32$.sentencepiece.TrainerSpec.ModelType:\x07UNIGRAM\x12\x18\n\nvocab_size\x18\x04 \x01(\x05:\x04\x38\x30\x30\x30\x12\x17\n\x0f\x61\x63\x63\x65pt_language\x18\x05 \x03(\t\x12 \n\x15self_test_sample_size\x18\x06 \x01(\x05:\x01\x30\x12*\n\x1b\x65nable_differential_privacy\x18\x32 \x01(\x08:\x05\x66\x61lse\x12+\n differential_privacy_noise_level\x18\x33 \x01(\x02:\x01\x30\x12\x32\n\'differential_privacy_clipping_threshold\x18\x34 \x01(\x04:\x01\x30\x12"\n\x12\x63haracter_coverage\x18\n \x01(\x02:\x06\x30.9995\x12\x1e\n\x13input_sentence_size\x18\x0b \x01(\x04:\x01\x30\x12$\n\x16shuffle_input_sentence\x18\x13 \x01(\x08:\x04true\x12 \n\x14mining_sentence_size\x18\x0c \x01(\x05\x42\x02\x18\x01\x12"\n\x16training_sentence_size\x18\r \x01(\x05\x42\x02\x18\x01\x12(\n\x17seed_sentencepiece_size\x18\x0e \x01(\x05:\x07\x31\x30\x30\x30\x30\x30\x30\x12\x1e\n\x10shrinking_factor\x18\x0f \x01(\x02:\x04\x30.75\x12!\n\x13max_sentence_length\x18\x12 \x01(\x05:\x04\x34\x31\x39\x32\x12\x17\n\x0bnum_threads\x18\x10 \x01(\x05:\x02\x31\x36\x12\x1d\n\x12num_sub_iterations\x18\x11 \x01(\x05:\x01\x32\x12$\n\x18max_sentencepiece_length\x18\x14 \x01(\x05:\x02\x31\x36\x12%\n\x17split_by_unicode_script\x18\x15 \x01(\x08:\x04true\x12\x1d\n\x0fsplit_by_number\x18\x17 \x01(\x08:\x04true\x12!\n\x13split_by_whitespace\x18\x16 \x01(\x08:\x04true\x12)\n\x1atreat_whitespace_as_suffix\x18\x18 \x01(\x08:\x05\x66\x61lse\x12+\n\x1c\x61llow_whitespace_only_pieces\x18\x1a \x01(\x08:\x05\x66\x61lse\x12\x1b\n\x0csplit_digits\x18\x19 \x01(\x08:\x05\x66\x61lse\x12#\n\x19pretokenization_delimiter\x18\x35 \x01(\t:\x00\x12\x17\n\x0f\x63ontrol_symbols\x18\x1e \x03(\t\x12\x1c\n\x14user_defined_symbols\x18\x1f \x03(\t\x12\x16\n\x0erequired_chars\x18$ \x01(\t\x12\x1c\n\rbyte_fallback\x18# \x01(\x08:\x05\x66\x61lse\x12+\n\x1dvocabulary_output_piece_score\x18 \x01(\x08:\x04true\x12\x1e\n\x10hard_vocab_limit\x18! \x01(\x08:\x04true\x12\x1c\n\ruse_all_vocab\x18" \x01(\x08:\x05\x66\x61lse\x12\x11\n\x06unk_id\x18( \x01(\x05:\x01\x30\x12\x11\n\x06\x62os_id\x18) \x01(\x05:\x01\x31\x12\x11\n\x06\x65os_id\x18* \x01(\x05:\x01\x32\x12\x12\n\x06pad_id\x18+ \x01(\x05:\x02-1\x12\x18\n\tunk_piece\x18- \x01(\t:\x05<unk>\x12\x16\n\tbos_piece\x18. \x01(\t:\x03<s>\x12\x17\n\teos_piece\x18/ \x01(\t:\x04</s>\x12\x18\n\tpad_piece\x18\x30 \x01(\t:\x05<pad>\x12\x1a\n\x0bunk_surface\x18, \x01(\t:\x05 \xe2\x81\x87 \x12+\n\x1ctrain_extremely_large_corpus\x18\x31 \x01(\x08:\x05\x66\x61lse"5\n\tModelType\x12\x0b\n\x07UNIGRAM\x10\x01\x12\x07\n\x03\x42PE\x10\x02\x12\x08\n\x04WORD\x10\x03\x12\x08\n\x04\x43HAR\x10\x04*\t\x08\xc8\x01\x10\x80\x80\x80\x80\x02"\xd1\x01\n\x0eNormalizerSpec\x12\x0c\n\x04name\x18\x01 \x01(\t\x12\x1c\n\x14precompiled_charsmap\x18\x02 \x01(\x0c\x12\x1e\n\x10\x61\x64\x64_dummy_prefix\x18\x03 \x01(\x08:\x04true\x12&\n\x18remove_extra_whitespaces\x18\x04 \x01(\x08:\x04true\x12 \n\x12\x65scape_whitespaces\x18\x05 \x01(\x08:\x04true\x12\x1e\n\x16normalization_rule_tsv\x18\x06 \x01(\t*\t\x08\xc8\x01\x10\x80\x80\x80\x80\x02"y\n\x0cSelfTestData\x12\x33\n\x07samples\x18\x01 \x03(\x0b\x32".sentencepiece.SelfTestData.Sample\x1a)\n\x06Sample\x12\r\n\x05input\x18\x01 \x01(\t\x12\x10\n\x08\x65xpected\x18\x02 \x01(\t*\t\x08\xc8\x01\x10\x80\x80\x80\x80\x02"\xfe\x03\n\nModelProto\x12\x37\n\x06pieces\x18\x01 \x03(\x0b\x32\'.sentencepiece.ModelProto.SentencePiece\x12\x30\n\x0ctrainer_spec\x18\x02 \x01(\x0b\x32\x1a.sentencepiece.TrainerSpec\x12\x36\n\x0fnormalizer_spec\x18\x03 \x01(\x0b\x32\x1d.sentencepiece.NormalizerSpec\x12\x33\n\x0eself_test_data\x18\x04 \x01(\x0b\x32\x1b.sentencepiece.SelfTestData\x12\x38\n\x11\x64\x65normalizer_spec\x18\x05 \x01(\x0b\x32\x1d.sentencepiece.NormalizerSpec\x1a\xd2\x01\n\rSentencePiece\x12\r\n\x05piece\x18\x01 \x01(\t\x12\r\n\x05score\x18\x02 \x01(\x02\x12\x42\n\x04type\x18\x03 \x01(\x0e\x32,.sentencepiece.ModelProto.SentencePiece.Type:\x06NORMAL"T\n\x04Type\x12\n\n\x06NORMAL\x10\x01\x12\x0b\n\x07UNKNOWN\x10\x02\x12\x0b\n\x07\x43ONTROL\x10\x03\x12\x10\n\x0cUSER_DEFINED\x10\x04\x12\x08\n\x04\x42YTE\x10\x06\x12\n\n\x06UNUSED\x10\x05*\t\x08\xc8\x01\x10\x80\x80\x80\x80\x02*\t\x08\xc8\x01\x10\x80\x80\x80\x80\x02\x42\x02H\x03'
generated? If I use decode() on it python complains that 'utf-8' codec can't decode byte 0x80 in position 43.

@ydshieh

ydshieh commented Jul 11, 2023

Copy link
Copy Markdown
Collaborator Author

Hi @fxmarty

That file is being generated by protobuf compile. I don't have the courage to read it ...

When I enable the support to use protobuf v4, I ran the whole CI (non-slow), and no failed test.

Could you show us your usage (related to protobuf) that produce some failures - maybe I can help ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants