-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bff/token optimisation #268
base: master
Are you sure you want to change the base?
Conversation
Bff training tweaks
fixed task name
This is very useful! One small findings: |
Ah yes, thanks for catching that - removed the file. I should have put some context in the original PR. This is to fix the issue I flagged here #266 |
@bugface – let me know if you're happy to merge :) |
I think it looks great. |
I think we're good to merge then, @bugface :) |
news about this? @bugface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the default value of replace_additional_special_tokens
is False
, why do you assign False
value when you explicit its usage?
Ah yeah, I guess bad style. At the time I wanted to be explicit because this bug was so opaque. Are you happy to merge if I fix that? |
I could be interested in patching it into my fork, if you fix and explain the changes. Thank you.
|
if len(set(list_of_tokens) - set(self.tokenizer.all_special_tokens)) > 0: | ||
newly_added_num = self.tokenizer.add_special_tokens({"additional_special_tokens": sorted(set(list_of_tokens))}, replace_additional_special_tokens=replace_additional_special_tokens) | ||
else: | ||
newly_added_num = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the real solution to the detected bug, isn't it?
PR to fix: #266