-
Notifications
You must be signed in to change notification settings - Fork 23
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
Jean/refacto benchmark #150
Conversation
ac1d63d
to
a4e437a
Compare
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.
Hi,
Thanks for starting the refactoring. Here are a few comments to go even further. I will not mark anything as compulsory given the time constraints, but it would be great to save what will not be done as an issue for future work.
- Optimization: in
fed_benchmark.py
, in as much as possible, initialize dataloaders after checking if the experiment needs to be run or not - Cutting
main
infed_benchmark.py
into sub-functions to make it even more readable (it's > 370 lines long at the moment...). I give more actionable suggestions below.
- Split
main
into sub-functionspooled_training
,local_training
,strategy_training
. That will make thismain
much more simpler (but +1 for thesingle_centric_training
which is much better than before). - The logic for the tests if the exps are finished or not should be hidden in sub-functions for readability.
- replace all the
len(index_of_interest) < (NUM_CLIENTS + 1)
tests to check if experiment is finished by a more explicit function likeif check_exp_finished(df, hyperparameters, sname, num_updates)
- same for all the statements
#dealing with edge cases that should not happen
--> in sub-functions
- replace all the
- Lots of variables are assigned very short names, like
training_dls
fortraining_dataloaders
,s
forstrategy
,m
formodel
... Better to use larger names - the logic for filling the hyperparameters' param could also be delegated in sub-functions.
conf.py
should be renamed into something more explicit, likeconfiguration_utils.py
a4e437a
to
f236007
Compare
Created an issue #154 so that this review is tracked but merging in the meantime. |
This PR is aimed at making the benchmark logic both simpler and more robust.