-
Notifications
You must be signed in to change notification settings - Fork 1.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
🌋 Add support for LLaVA-Next in DPOTrainer
#2413
🌋 Add support for LLaVA-Next in DPOTrainer
#2413
Conversation
Great! Thanks @chenweize1998! Can you try to uncomment Line 1142 in ac26778
|
Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com>
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. |
DPOTrainer
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 @chenweize1998!
As expected, "Tests / Tests with dev dependencies" will fail until huggingface/transformers#34953 is merged. We can safely ignore this failing test. |
What does this PR do?
This PR provides a quick fix for issue #2403. Previously, DPOTrainer did not support LLaVA-Next because the required
image_sizes
parameter from the LLaVA-Next forward function was being removed during data processing within DPOTrainer. This update modifies DPOTrainer to retain the image_sizes parameter if it is returned by the image processor and passes it to the model when present.While this fix resolves the issue on my end - I successfully ran DPOTrainer with LLaVA-Next - but it has not been extensively tested. I would appreciate assistance or guidance on the next steps to ensure broader compatibility and robustness.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines.
Who can review?
Maybe @qgallouedec