-
Notifications
You must be signed in to change notification settings - Fork 37
refactor diloco test #232
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
refactor diloco test #232
Conversation
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.
LGTM
torchft/diloco_trainer.py
Outdated
if self.manager.current_step() >= 4: | ||
break | ||
|
||
return all_state_dicts |
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.
Not directly related to this PR but wonder if we should just add a main check and use this for manual testing
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.
what do you mean by main
check? :)
torchft/diloco_trainer.py
Outdated
@@ -0,0 +1,310 @@ | |||
import copy |
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.
Does this makes sense under torchft/? Should we add an underscore to indicate it's internal or move it under some testing
dir or something?
Just don't want folks getting confused and importing this directly for whatever reason
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.
agree with this
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.
moved it to test
folder. will start putting everything test related there
0ce2173
to
56893f8
Compare
Summary: - move the training loop to a separate file - convert it into a class so that methods can be overridden without having to duplicate code
Summary:
Stack created with Sapling. Best reviewed with ReviewStack.