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

VLM dpo bug #1972

Closed
liuchaohu opened this issue Aug 26, 2024 · 3 comments · Fixed by #2209
Closed

VLM dpo bug #1972

liuchaohu opened this issue Aug 26, 2024 · 3 comments · Fixed by #2209
Labels
🏋 DPO Related to DPO 👁️ VLM Related to Visual Language Models

Comments

@liuchaohu
Copy link

trl/trainer/dpo_trainer.py line 542
The tokenizer for super().init () should be set to self.tokenizer instead of tokenizer, otherwise the previous is_vision_model will be invalid.

@qgallouedec
Copy link
Member

what do you mean by

otherwise the previous is_vision_model will be invalid.

?

When the model is a VLM, the transformers.Trainer expect a processor for the arg tokenizer, not a tokenizer.
in other words:

Trainer(
    ...,
    # tokenizer=processor.tokenizer, # NO
    tokenizer=processor, # YES
)

@liuchaohu
Copy link
Author

liuchaohu commented Aug 26, 2024

In line 341-343, if self.is_vision_model == True, then self.processor = tokenizer(i.e., processor) and self.tokenizer = tokenizer.tokenizer.
However, In line 536-548, super().init(...) will set self.tokenizer = tokenizer(i.e., processor).
In this case, both self.tokenizer & self.processor are tokenizer(i.e., processor).

@qgallouedec qgallouedec added the 👁️ VLM Related to Visual Language Models label Aug 26, 2024
@qgallouedec qgallouedec added the 🏋 DPO Related to DPO label Oct 7, 2024
@qgallouedec qgallouedec linked a pull request Oct 20, 2024 that will close this issue
5 tasks
@qgallouedec
Copy link
Member

We are refactoring DPO data processing. The new implementation should be more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏋 DPO Related to DPO 👁️ VLM Related to Visual Language Models
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants