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

Distill trainer #166

Merged
merged 15 commits into from
Nov 15, 2022
Merged

Distill trainer #166

merged 15 commits into from
Nov 15, 2022

Conversation

orenpereg
Copy link
Collaborator

  1. Added Trainer for setfit distillation (file src/setfit/trainer_distill.py)
  2. updated fewshot_distillation script to work with DistilSetFitTrainer

@orenpereg orenpereg requested a review from lewtun November 10, 2022 18:59
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.

Thanks for adding this nice feature @orenpereg 🔥 !

I've left a few comments / questions and would love to see some unit tests in a test_trainer_distillation.py file (you can take inspiration from the existing SetFitTrainer ones)

Could you also share a code snippet of how this trainer works end-to-end? I know we have that logic in the script, but it would be great to have a condensed version we can later add to the README :)

scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
src/setfit/trainer_distill.py Outdated Show resolved Hide resolved
src/setfit/trainer_distill.py Outdated Show resolved Hide resolved
src/setfit/trainer_distill.py Outdated Show resolved Hide resolved
src/setfit/trainer_distill.py Outdated Show resolved Hide resolved
@orenpereg orenpereg requested a review from lewtun November 15, 2022 07:19
@orenpereg
Copy link
Collaborator Author

Hi @lewtun. Thanks for your thorough review and good comments🔥. I addressed all your comments and added tests under tests/test_trainer_distillation.py. can you pls take a look?
I also wrote a code snippet for the readme for using this trainer end-to-end:
Run distillation using trainer example.docx

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.

Thanks for iterating on this feature @orenpereg and adding some nice tests ❤️ !

I've left some final nits and then this should be good to merge :)

scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Outdated Show resolved Hide resolved
scripts/setfit/run_fewshot_distillation.py Show resolved Hide resolved
src/setfit/trainer.py Outdated Show resolved Hide resolved
src/setfit/trainer_distillation.py Show resolved Hide resolved
src/setfit/trainer_distillation.py Outdated Show resolved Hide resolved
src/setfit/trainer_distillation.py Outdated Show resolved Hide resolved
tests/test_trainer_distillation.py Outdated Show resolved Hide resolved
tests/test_trainer_distillation.py Outdated Show resolved Hide resolved
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.

Thanks for the last round of changes @orenpereg - this looks really nice 🔥

@lewtun lewtun merged commit 4a613b0 into main Nov 15, 2022
@lewtun lewtun deleted the distill_trainer branch November 15, 2022 12:31
@orenpereg
Copy link
Collaborator Author

thanks Lewis :)

PhilipMay pushed a commit to PhilipMay/setfit that referenced this pull request Nov 20, 2022
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.

2 participants