feat: Add some tokenizers and maximum token length flag - #657
Conversation
|
|
8fa124b to
240c06e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
==========================================
+ Coverage 85.58% 85.62% +0.04%
==========================================
Files 258 261 +3
Lines 11378 11412 +34
==========================================
+ Hits 9738 9772 +34
Misses 1640 1640 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaxiBoether
left a comment
MaxiBoether
left a comment
There was a problem hiding this comment.
Thanks John. Here I had some more comments
| - evidently==0.4.27 | ||
| - alibi-detect==0.12.* | ||
| - tenacity | ||
| - sentencepiece |
There was a problem hiding this comment.
Is this used anywhere in this PR?
There was a problem hiding this comment.
The T5 tokenizer needs it to work, it is not used in the code but it is needed.
| Adapted from WildTime's initialize_distilbert_transform | ||
| Here you can find the original implementation: | ||
| https://github.com/huaxiuyao/Wild-Time/blob/main/wildtime/data/utils.py | ||
| """ |
There was a problem hiding this comment.
this comment should probably be moved to the new HFTokenizerTransform, since that one is based on this original impl
| """Tokenizer transform class for the T5 model. | ||
|
|
||
| Args: | ||
| max_token_length: Maximum length for tokenization (default: 300). |
There was a problem hiding this comment.
default 300 it says here but it is set to 128
| def __call__(self, sample: str) -> torch.Tensor: | ||
| """ | ||
| Tokenize the input sample and return a tensor with input_ids and attention_mask. | ||
| Args: | ||
| sample: Input string to tokenize. | ||
| Returns: | ||
| A torch.Tensor with shape (max_token_length, 2), where: | ||
| - dim 0 is the token sequence length. | ||
| - dim 1 contains input_ids and attention_mask. | ||
| """ | ||
| if not sample: # This is needed here since this tokenizer cannot handle an empty list | ||
| sample = self.tokenizer.eos_token | ||
| tokens = self.tokenizer( | ||
| sample, padding="max_length", truncation=True, max_length=self.max_token_length, return_tensors="pt" | ||
| ) | ||
| data = torch.stack((tokens["input_ids"], tokens["attention_mask"]), dim=2) | ||
| data = torch.squeeze(data, dim=0) | ||
| return data |
There was a problem hiding this comment.
I don't understand. GPT2 usses huggingface, so we should use the HFTokenizerTransform. The only difference I can see is the if not sample code path. Why is that needed? The tokenizer should never get an empty string. Even in that case, right here you set sample = token, and then pass this token to the tokenizer, which does not make sense. I don't understand why this extra case is needed, but if it's needed, it affects all HF tokenizers and should probably live in the parent class
There was a problem hiding this comment.
last, if it truly only affects this tokenizer and is needed, you can check sample and call the superclass's call function instead of re-implementing the tokenization call
There was a problem hiding this comment.
You are right it does not make sense to put the token in the tokenizer, I think you can tecnically recieve an empty string if you have a generative "target" that is missing, which could happen in some datasets. all other tokenizers have a token for an empty string but not gpt2, so it is the only one that needs this.
| bool enable_accurate_gpu_measurements = 25; | ||
| int64 record_loss_every = 26; | ||
| bool drop_last_batch = 27; | ||
| int64 tokenizer_sequence_length =28; |
There was a problem hiding this comment.
We use this as max_token_length and I guess we should name it that way. This flag does not guarantee padding, does it? I.e., if it is set to 2048 and I have a short string, are we guaranteed to have padding? It should probably also be an optional int64 since not every training uses a tokenizer
There was a problem hiding this comment.
I think it does guarantee padding up to the tokenizer sequence length.
| ) -> torch.utils.data.DataLoader: | ||
| # TODO(#289): Replace inefficient per class dataloader | ||
| dataset = PerClassOnlineDataset( | ||
| dataset = PerClassOnlineDataset( # type: ignore # pylint: disable=too-many-function-args |
There was a problem hiding this comment.
We should not use type: ignore unless necessary. If there is a typing issue we need to fix the typing issue
There was a problem hiding this comment.
Solved, not sure how though
| shuffle: bool, | ||
| tokenizer: str | None, | ||
| log_path: pathlib.Path | None, | ||
| sequence_length: int = 128, |
There was a problem hiding this comment.
rename to max_token_length if that is truly what this is doing
There was a problem hiding this comment.
also probably this is int | None since not every training uses a tokenizer
| self.tokenizer: str | None = request.tokenizer.value if request.HasField("tokenizer") else None | ||
|
|
||
| self.offline_dataset_path = offline_dataset_path | ||
| self.tokenizer_seq_length: int = request.tokenizer_sequence_length |
There was a problem hiding this comment.
int | None? (handling similar to e.g. how self.tokenizer is set)
| bool enable_accurate_gpu_measurements = 25; | ||
| int64 record_loss_every = 26; | ||
| bool drop_last_batch = 27; | ||
| int64 tokenizer_sequence_length =28; |
There was a problem hiding this comment.
another question, I actually don't see this being used anywhere in the supervisor, i.e., all requests we send out right now don't set this. we should also (similar how you did it for grad clipping) add this to the pipeline config and forward it, no?
There was a problem hiding this comment.
Hey yes it seems like I deleted that line somehow, I put it back :)
MaxiBoether
changed the title
2 participants
Added the necessary code for the tokenizers, appropriate tests and the tokenizer length flag to decide sequence length in the pipeline.