-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Support Kosmos-2.5 #31711
base: main
Are you sure you want to change the base?
Support Kosmos-2.5 #31711
Conversation
cc @ydshieh |
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.
Thanks a lot! ❤️ Just a few more things that can be removed now but looks nice overall to me. Looking forward for changes to accommodate generate()
from mixin and should be okay
return_legacy_cache = False | ||
if ( | ||
use_cache and not isinstance(past_key_values, Cache) and not self.training | ||
): # kept for BC (non `Cache` `past_key_values` inputs) | ||
return_legacy_cache = True | ||
past_key_values = DynamicCache.from_legacy_cache(past_key_values) | ||
logger.warning_once( | ||
"We detected that you are passing `past_key_values` as a tuple and this is deprecated and will be removed in v4.43. " | ||
"Please use an appropriate `Cache` class (https://huggingface.co/docs/transformers/v4.41.3/en/internal/generation_utils#transformers.Cache)" | ||
) |
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.
hmm i've been removing deprecation for new models. Oke, if core maintainer agrees we can leave but maybe change the deprecation version to around v4.50 or smth?
self.boi = tokenizer.convert_tokens_to_ids("<image>") | ||
self.eoi = tokenizer.convert_tokens_to_ids("</image>") | ||
self.pad = tokenizer.convert_tokens_to_ids("<pad>") | ||
self.bos = tokenizer.convert_tokens_to_ids("<s>") | ||
self.eos = tokenizer.convert_tokens_to_ids("</s>") | ||
|
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.
nit: these can be saved as special tokens in tokenizer so you can do tokenizer.boi_token_id
instead of hardcoding the string of each token. Not super necessary, simply FYI :)
for that when converting you need to add
# bos/eos/pad tokens are already in tokenizer so no need to add them
extra_special_tokens = {"boi_token": "<image>", "eoi_token": </image>}
tokenizer = AutoTokenizer.from_pretrained(model_id, extra_special_tokens=extra_special_tokens)
Hi @zucchini-nlp Great reviews! Could you check the changes shown below? I think all comments addressed. (and also the removed |
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.
Thanks a lot for iterating and removing the overwritten generate()
!
run slow |
This comment contains run-slow, running the specified jobs: ['models/kosmos2_5'] ... |
What does this PR do?
#30877 Implementation of Kosmos-2.5 in transformers.
https://huggingface.co/kirp/kosmos2_5/blob/main/README.md
Usage