Fedpara updated

[yehias21]

Issue

#2031

Description

Updated draft

Remaining tasks

[ ] Finalizing personalized FedPara with femnist

[jafermarq]

Hey @yehias21,

This is just a preliminar review. I think before moving into adding FEMNIST experiments (which you can find how other baselines integrated that dataset here: https://github.com/adap/flower/tree/main/baselines/fedmeta -- i.e. cloning from LEAF directly), it's preferable to finalise the CIFAR-10 and CIFAR-100 experiments.

• Would it be possible to present the line plots in the same format as in Figure 3 in the paper. That means showing both VGG16_ori and VGG16_fedpara curves with the x-axis showing communication costs (instead of rounds).
• The environment setup section needs to be completed with the instructions to follow in order to construct the python environment. (likely you can borrow from what your colleagues did for FedExp: https://github.com/Roeia99/flower/tree/FedExP/baselines/fedexp#environment-setup)
• please include all necessary packages in pyproject.toml and detail how i can run the experiments (I see you provide a run.sh but you should mention this in the README.md in the Running the experiments section).

[yehias21]

Hi Javier, I've addressed all the comments related to the Cifar baselines as mentioned. Please inform me if there are any additional matters to attend to.

[jafermarq]

Hi @yehias21,

Just a small comment below in pyproject.toml. I also made some edits, primarily to the README.md.

I ran your CIFAR-10/100 experiments and all curves resulted very close to the ones you include in the README with the exception of the model.conv_type=standard CIFAR-10 iid setting. The results i get are more in line with those in the paper than what your plots show. For example, for CIFAR-10 non-iid crosses the 60% threshold approx after 180GB of comms (see Fig3d in the paper), I get something similar, but in your plot it seems this happens after just 75GB of communication. For the IID CIFAR-10 setting I observe a similar situation: paper and results after running the code show croshing the 70% threshold after 100GB of communication but your plot shows that can be achieve with 50GB of communication.

I wonder the setting you run for those experiments you made the plots from was slightly different for CIFAR-10 with model.conv_type=standard and that's why the communication costs seem to be about half. Also note that max comm costs shown in the plots is 175GB while when running the code it would be a bit over 350GB (which matches the logic of (2 x 200rounds x 16clients x 15.3M params x 4 / 1024) -- I multiply by 4 to "convert" to MB.

Finally do you have an update about FEMNIST?

[jafermarq]

Hi @yehias21 and team,

Following our last chat, i think we can start wrapping up this baseline and focus on CIFAR-10/100 results. It's fine leaving the personalization with MNIST ones as an extension (can be added as a new section at the end of the readme -- see my comments about that below).

I've pushed a commit that fixing formatting and some typing issues. Most issues are resolved except for a couple (one being passing an argument that's not expected to gen_evaluate_fn() and another in client's _get_keys_state_dict())

Please find my comments below. I applied them all locally, the code runs but I'm not seeing the global model improving. I ran this:

python -m fedpara.main --multirun model.conv_type=standard,lowrank 

It could be that I made a mistake.

Let me know when you have gone through my comments.

[jafermarq]

Hi @yehias21 and collaborators! Your implementation of FedPara is ready to be merged! Amazing work!!