-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Why are instructions not masked when performing VSFT for LLaVa? #1880
Comments
@qgallouedec When performing SFT on a VLM, it may be a better choice only to calculate the loss of the response part. Does trl provide a direct implementation for this? Can you give an example? Thanks! |
@qgallouedec Sorry to bother you, I would like to ask if SFTTrainer can directly calculate the loss of only the response part, and whether you have plans to implement a vsft script that only calculates the response loss. Thank you! |
Hi, sorry for the delay, I'm addressing the issues in order, and there are a lot these days.
Can you justify this?
I'm not sure about this. Have you tried it? It would be good to have some results to confirm or refute this statement.
A small modification of the data collator should be enough. Just set labels to |
In the implementation of the llava repository, the padding token and instruction token are all set to -100. For reference, see the preprocess_v1 function in https://github.com/haotian-liu/LLaVA/blob/main/llava/train/train.py
|
I roughly implemented this idea as follows:
|
Thanks for the reference and for the piece of code which can certainly be useful. |
I have some questions about the
LLavaDataCollator
in the vsft_llava.py:https://github.com/huggingface/trl/blob/main/examples/scripts/vsft_llava.py
I noticed that you copied the input_id (image, question concatenated with answer) to the label, and then only set the label of the pad token to -100 (no loss will be calculated). However, as far as I understand SFT, only the loss of the answer part should be calculated, which means that we should also set the labels of all question parts to -100?
Looking forward to your reply!
The text was updated successfully, but these errors were encountered: