refactor(baselines) Upgrade FedProx Baselne to new flwr format #4937
Conversation
maddox-j
requested review from
chongshenng,
jafermarq,
tanertopal and
danieljanes
as code owners
There was a problem hiding this comment.
Hey @maddox-j , this looks pretty good!! I added a few minor comments below.
I was able to run all the experiments running your script but then it seems the notebook ins't creating the plots. I believe some parts need (maybe just the process_data() function) adjustments since the results structure is different since we aren't using Hydra's --multirun anymore.
baselines/fedprox/README.md
Outdated
| @@ -60,50 +58,47 @@ To construct the Python environment, simply run: | |||
|
|
|||
| ```bash | |||
| # Set directory to use python 3.10 (install with `pyenv install <version>` if you don't have it) | |||
| pyenv local 3.10.12 | |||
| pyenv virtualenv 3.10.14 <name-of-your-baseline-env> | |||
There was a problem hiding this comment.
| pyenv virtualenv 3.10.14 <name-of-your-baseline-env> | |
| pyenv virtualenv 3.10.14 fedprox |
There was a problem hiding this comment.
I'm thinking it's better to give it a reasonable name directly so people that want to run the baseline can simply copy/paste the commands
baselines/fedprox/README.md
Outdated
|
|
||
| # Tell poetry to use python3.10 | ||
| poetry env use 3.10.12 | ||
| pyenv activate <name-of-your-baseline-env> |
There was a problem hiding this comment.
| pyenv activate <name-of-your-baseline-env> | |
| pyenv activate fedprox |
baselines/fedprox/pyproject.toml
Outdated
| "Topic :: Software Development :: Libraries :: Python Modules", | ||
| "Typing :: Typed", | ||
| dependencies = [ | ||
| "flwr[simulation]>=1.15.0", |
There was a problem hiding this comment.
| "flwr[simulation]>=1.15.0", | |
| "flwr[simulation]>=1.15.2", |
There was a problem hiding this comment.
just bumping to the latest version 👼
baselines/fedprox/fedprox/dataset.py
Outdated
| batch_size=dataset_config.batch_size, | ||
| shuffle=True, | ||
| ), | ||
| None, |
There was a problem hiding this comment.
I wonder if we should return a DataLoader for the validation split too (i.e. partition_train_test["tests"]). The reason being that pretty much everything in the app is ready to work with it, even the configs support specifying a fraction_evaluate(but if users set it to >0, the code will crash due to theNone` above). Wdyt? I don't think it's necessary too run new experiments though.
baselines/fedprox/fedprox/model.py
Outdated
| def set_weights(net, parameters): | ||
| """Apply parameters to an existing model.""" | ||
| params_dict = zip(net.state_dict().keys(), parameters) | ||
| state_dict = OrderedDict({k: torch.tensor(v) for k, v in params_dict}) |
There was a problem hiding this comment.
| state_dict = OrderedDict({k: torch.tensor(v) for k, v in params_dict}) | |
| state_dict = OrderedDict({k: torch.from_numpy(v) for k, v in params_dict}) |
There was a problem hiding this comment.
a small change but it helps with certain types of models (e.g. when there a BN layer that's uninitialized, they way pytorch and numpy represent an empty tensor is different and therefore issues can arise).
| "flwr-datasets[vision]>=0.5.0", | ||
| "torch==2.5.1", | ||
| "torchvision==0.20.1", | ||
| "easydict==1.11" |
There was a problem hiding this comment.
Is easydict similar to Omegaconf's DictConfig?
There was a problem hiding this comment.
Yes. The reason I used easydict is to have a neater access structure for the configs (i.e not accessing with . and [""] in the same line
There was a problem hiding this comment.
In these config files, is it necessary to specify all the settings in the experiment? or is it enough to only set those that are different from those in pyproject.toml. For example, fraction_evaluate is always the same.
There was a problem hiding this comment.
I changed this so the config files only specify the arguments that they are changing :D
baselines/fedprox/pyproject.toml
Outdated
| stragglers_fraction = 0.9 | ||
| learning_rate = 0.03 | ||
| mu = 1.0 | ||
| num_clients = 1000 |
There was a problem hiding this comment.
Let's indicate with a comment that this num_clients is here just for the ServerApp (and that it doesn't have an impact on the size of the federation). My understanding is that you are using this here because the ServerApp can't access the number of nodes connected outside the strategy and it can't read the node_config either. This won't be a problem with the upcoming Message API 👼
|
@jafermarq Thanks for all the comments. I have resolved your comments in one commit so that we keep the history a bit neater. With reference to the notebook: I did make these function changes initially. I added a fixed path to the results in the notebook now in case this was the error. :) |
Issue
Description
This PR upgrades the existing
FedProxbaseline from the previousflwrformat to the new version.Specifically:
flwr-datasetswithDistributionPartitionerinstead of manual handling.Serverclass definition.Related issues/PRs
N/A
Proposal
Explanation
This makes the
FedProxbaseline consistent with the currentflwrversion.Checklist
#contributions)Any other comments?
N/A