-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 Multitask Prompt Tuning #400
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
@pacman100 |
Also, a working notebook has been added @pacman100 🤗 |
Fix dict index in embedding retrieval
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.
Thank you so much @mayank31398 for adding MultiTask Prompt Tuning 🔥! Could you please resolve the conflicts and run make style
and make quality
to resolve the quality issues. Post that we can go ahead and merge this PR 🤗.
Done @younesbelkada :) |
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 so much for this PR and all the work you put in.
I haven't read the paper in detail, so I cannot comment on whether the implementation is faithful or not.
I have a few minor comments, please check my comments. Not all of that is critical. My main concern is that, if I'm not mistaken, only one prompt_tuning_init
argument is tested. Therefore, I would like to see the tests extended to cover the other types.
As a nice-to-have feature, it would great if there were a few explanations in the notebook about what's going on + a link to the paper.
I just noticed that there isn't any addition to the docs for MPT. Could you please add some documentation there? Or are we fine with adding it in a separate PR? |
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.
Hello @mayank31398, Thank you for iterating! I believe the PR can be merged once the conflicts are resolved as there is an example showing usage and related tests. Thank you for all the work and for your kind patience 🤗
Hey @pacman100 let me add some comments in the code. |
@pacman100 I have addressed the suggestions. |
@pacman100 @younesbelkada lets merge this? |
Hey, guys still waiting for an update :) |
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 a lot for your patience @mayank31398 and sorry for the delay , I left two tiny comments, apart from that I trust other maintainers review. Can you please revert the changes in the unrelated files?
Thanks!
examples/image_classification/image_classification_timm_peft_lora.ipynb
Outdated
Show resolved
Hide resolved
examples/multilayer_perceptron/multilayer_perceptron_lora.ipynb
Outdated
Show resolved
Hide resolved
@younesbelkada addressed the comments |
@mayank31398 Just to let you know: We'll probably soon merge #807 which adds a new folder structure to the tuners. If you want to, you can already adjust your code to use the new structure. Otherwise, it's also fine as is, then we'll do the refactoring later. |
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 again!
I prefer leaving as is for now. |
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.
Thank you @mayank31398 for iterating multiple times and for your patience, LGTM! 🔥🚀✨
@mayank31398 Why can't I locate the distillation process of the source prompt in the code? Am I missing something? 😭 |
@junzhang-zj yeah, it doesn't have that. |
@mayank31398 Thank you for your answer, which addressed my concern about carelessness! |
Adds code for the paper: https://arxiv.org/abs/2303.02861