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

Could there be a bug in the FT implementation #173

Closed
drd13 opened this issue Feb 6, 2024 · 6 comments
Closed

Could there be a bug in the FT implementation #173

drd13 opened this issue Feb 6, 2024 · 6 comments
Labels
question Further information is requested

Comments

@drd13
Copy link

drd13 commented Feb 6, 2024

I've found what I think might be a bug in the implementation of the fine-tuning baseline. If this is indeed the case, this bug would yield incorrect results when the unlearning target is longer than one token.

Using the VSCode debugger, I found that the code in ft_main.py doesn't carry-out backpropagation properly. The current version of the code passes the prompts without the targets to the model by calling model(**inputs). It then gathers the logits of all tokens in the target from the last tokens logits. This will maximise the probability of all tokens in the target immediately succedding the prompt. This is not the correct behaviour which should maximise the probability of the first token in the target being a continuation to the input and then maximising the probability of the second token in the target being a continuation to the first token in the target...

I think this issue might be in the ROME repository, where the original code came from, where I've written an issue but they haven't responded. Thanks for any assistance you may offer.

@zxlzr zxlzr added the question Further information is requested label Feb 6, 2024
@pengzju
Copy link
Collaborator

pengzju commented Feb 12, 2024

Thanks for your advice, we will modify the training paradigm of FT-L as soon as possible

pengzju added a commit that referenced this issue Feb 12, 2024
@pengzju
Copy link
Collaborator

pengzju commented Feb 12, 2024

I have updated the optimization target of FT, you can refer to the latest version of the code

@drd13
Copy link
Author

drd13 commented Feb 12, 2024

Hello @pengzju. I'll mark the issue as closed (but I haven't double-checked the code). If you ever rerun the experiments in the survey paper with the fix, I would be interested in knowing how it changes relative performance of fine-tuning.

Thank you very much for your rapid response and bug fix.

@drd13 drd13 closed this as completed Feb 12, 2024
@pengzju
Copy link
Collaborator

pengzju commented Feb 13, 2024

Thank you for your suggestion.
In my actual testing. Even if the optimization objective is changed, FT-L still cannot take into account both Reliability and Locality, which means that high reliability means that the weights of the model are completely damaged. High locality cannot guarantee a high editing success rate, which is still consistent with the results in our paper
😊

@drd13 drd13 mentioned this issue Feb 23, 2024
@zxlzr zxlzr reopened this Feb 23, 2024
@pengzju
Copy link
Collaborator

pengzju commented Feb 25, 2024

Thank you for your suggestion, we have provided two implementations (objective_optimization in FT-L):

    1. prompt_last: the method of ROME's (https://arxiv.org/abs/2202.05262) original paper, which calculates nll loss through the last token of the input.
    1. target_new: the standard autoregressive method, using the cross-entropy loss function

You can choose the appropriate optimization goal based on your experiment settings. Welcome you to try. 😊

@pengzju pengzju closed this as completed Feb 25, 2024
@zxlzr
Copy link
Contributor

zxlzr commented Mar 2, 2024

I've found what I think might be a bug in the implementation of the fine-tuning baseline. If this is indeed the case, this bug would yield incorrect results when the unlearning target is longer than one token.

Using the VSCode debugger, I found that the code in ft_main.py doesn't carry-out backpropagation properly. The current version of the code passes the prompts without the targets to the model by calling model(**inputs). It then gathers the logits of all tokens in the target from the last tokens logits. This will maximise the probability of all tokens in the target immediately succedding the prompt. This is not the correct behaviour which should maximise the probability of the first token in the target being a continuation to the input and then maximising the probability of the second token in the target being a continuation to the first token in the target...

I think this issue might be in the ROME repository, where the original code came from, where I've written an issue but they haven't responded. Thanks for any assistance you may offer.

Thank you very much for raising the issue. We actually encountered this problem in the early experiments last year, but to maintain consistency with previous work ROME, we didn't address it at the time. As @pengzju mentioned, our current approach involves splitting FT into two strategies.

  • prompt_last: the method of ROME's (https://arxiv.org/abs/2202.05262) original paper, which calculates nll loss through the last token of the input.

  • target_new: the standard autoregressive method, using the cross-entropy loss function. To differentiate and make it comparable, we're referring to it as FT-M which can achieve much better performance than the FT-L.

We'll be planning to update the survey paper on Arxiv with new experimental results soon, and we've already noted this issue in the readme. Feeling like in the future, everyone can use this FT-M technique as a strong knowledge editing baseline.

Best,

EasyEdit Team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants