-
Notifications
You must be signed in to change notification settings - Fork 267
[video-comprehension] Fix failing example. #2313
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
Conversation
|
The code quality check failed, please run |
5572c8f to
f7cd918
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
LGTM |
| elif cache_position is None: | ||
| past_length = past_key_values[0][0].shape[2] if past_key_values is not None else 0 | ||
| cache_position = torch.arange(past_length, input_ids.shape[1], dtype=torch.long, device=input_ids.device) | ||
| model_inputs["cache_position"] = cache_position |
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.
Are we sure this doesn't break generation for other models?
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.
Good point.
This change is in line with transformers upgrade, but I'm running text generation slow tests. It looks fine for now, I'll update when they're done.
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.
Hi @ugolowic can you submit the results?
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 ran text generation slow tests on main branch and on this change rebased onto the main branch and the results are exactly the same.
This commit fixes errors in video-comprehension example involving VideLLava and CLIP models and caused by latest transformers upgrade. * Adapt modeling_video_llava to the transformers upgrade by adding GaudiVideoLlavaModel.forward * Fix mismatched matrix sizes in CLIP attention. * Fix access to non-existing attribute in GaudiGenerationMixin. Signed-off-by: Urszula <[email protected]>
f7cd918 to
316601d
Compare
| elif cache_position is None: | ||
| past_length = past_key_values[0][0].shape[2] if past_key_values is not None else 0 | ||
| cache_position = torch.arange(past_length, input_ids.shape[1], dtype=torch.long, device=input_ids.device) | ||
| model_inputs["cache_position"] = cache_position |
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.
Hi @ugolowic can you submit the results?
regisss
left a comment
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.
LGTM!
This commit fixes errors in video-comprehension example involving VideLLava and CLIP models and caused by latest transformers upgrade.
This is a fix for failing README example here: https://github.com/huggingface/optimum-habana/tree/main/examples/video-comprehension