Skip to content
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 LLaVA-NeXT in Vision SFT #1959

Merged
merged 5 commits into from
Aug 23, 2024
Merged

Support LLaVA-NeXT in Vision SFT #1959

merged 5 commits into from
Aug 23, 2024

Conversation

qgallouedec
Copy link
Member

Closes #1785

@qgallouedec qgallouedec requested review from lewtun and kashif August 22, 2024 15:00
@kashif
Copy link
Collaborator

kashif commented Aug 22, 2024

do we need to pin the transformer version to the latest?

@HuggingFaceDocBuilderDev

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.

@qgallouedec
Copy link
Member Author

do we need to pin the transformer version to the latest?

Good point! let me check

@kashif
Copy link
Collaborator

kashif commented Aug 22, 2024

it would be good to pin the transformer to latest due to another PR as well...

@lewtun
Copy link
Member

lewtun commented Aug 22, 2024

do we need to pin the transformer version to the latest?

Where do you want to pin the version? I don't think we want to do this in requirements.txt, but an alternative would be to specify a min version for this specific model that is checked like we do in transformers: https://github.com/huggingface/transformers/blob/273c0afc8f0289e0cb5d18dc76ccc251504298e0/src/transformers/utils/__init__.py#L260

@kashif
Copy link
Collaborator

kashif commented Aug 22, 2024

@lewtun sorry i meant >= <latest> in here: https://github.com/huggingface/trl/blob/main/setup.py#L65

@lewtun
Copy link
Member

lewtun commented Aug 22, 2024

@lewtun sorry i meant >= <latest> in here: https://github.com/huggingface/trl/blob/main/setup.py#L65

Ah no we don't want to do that because it will lead to a lot of pain for users with conflicting transformers dependencies. In my opinion we should use the checker helper (or something similar) for architectures which are version-specific

@qgallouedec
Copy link
Member Author

LLaVA-NeXT has been added in v4.40.0~235 but the example is failing before huggingface/transformers@e84012737 (not released yet)

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the tests pass!

examples/scripts/vsft_llava.py Show resolved Hide resolved
@qgallouedec qgallouedec merged commit 4788e5c into main Aug 23, 2024
6 of 10 checks passed
@qgallouedec qgallouedec deleted the support-llava-next-vsft branch August 23, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with SFT of LLaVA-Next
4 participants