Convert SentencePieceTokenizer and associated models to new assets paradigm #1323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattdangerw
requested changes
Nov 21, 2023
keras_nlp/models/t5/t5_tokenizer.py
Outdated
| def set_vocabulary(self, proto): | ||
| super().set_vocabulary(proto) | ||
| if proto is not None: | ||
| for token in [self.pad_token]: |
There was a problem hiding this comment.
is this a prexisting bug that self.end_token was not included here?
There was a problem hiding this comment.
You're right, this is likely a preexisting bug. Fixed!
| path = os.path.join(dir_path, VOCAB_FILENAME) | ||
| self.set_vocabulary(path) | ||
|
|
||
| def set_vocabulary(self, proto): |
There was a problem hiding this comment.
I think we should just call this set_proto. A little weird, but matches the init arg naming we have today. And this should not be set on the results of get_vocabulary, that would blow up.
word_piece_tokenizer.set_vocabulary
sentence_piece_tokenizer.set_proto
byte_pair_tokenizer.set_vocabulary_and_merges
There was a problem hiding this comment.
Got it, I've switched them to set_proto throughout. Thanks!
| def save_assets(self, dir_path): | ||
| path = os.path.join(dir_path, VOCAB_FILENAME) | ||
| with open(path, "w") as file: | ||
| for token in self.proto: |
There was a problem hiding this comment.
This won't work! We need to write the proto to a file. Not sure how best to do this but we might want to spy on https://github.com/google/sentencepiece
I should also figure out how to enable our large testing on this branch, which includes saving. That would catch this issue.
There was a problem hiding this comment.
Great catch!
Fixed, I could simply write self.proto to the file directly (since I have set this as a bytes string), and load_assets will simply pass the filepath to set_proto (which has pre-existing capability to handle proto loading from a filepath)
I think our next PR can focus on e2e testing for the save/load assets workflow.
mattdangerw
approved these changes
Nov 21, 2023
mattdangerw
pushed a commit
that referenced
this pull request
Nov 29, 2023
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
mattdangerw
pushed a commit
that referenced
this pull request
Dec 7, 2023
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
mattdangerw
pushed a commit
that referenced
this pull request
Dec 7, 2023
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
mattdangerw
pushed a commit
that referenced
this pull request
Dec 8, 2023
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
sampathweb
pushed a commit
that referenced
this pull request
Dec 12, 2023
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
mattdangerw
pushed a commit
that referenced
this pull request
Jan 4, 2024
…radigm (#1323) * Convert SentencePiece tokenizer to save_assets/load_assets * Convert albert to new assets paradigm * Convert DebertaV3 to new assets paradigm * Fix formatting issues * Convert FNet to new assets paradigm * Convert XLMRoberta to new assets paradigm * Convert T5 Tokenizer to new assets paradigm * Fix sentencepiece tokenizer config test * Change set_vocabulary to set_proto * Change proto to raw proto * Change to proto_bytes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR converts the SentencePieceTokenizer to use save_assets/load_assets logic and converts the following models to defer associated vocabulary logic: