Add TAMUNA baseline

[Crabzmatic]

Implementation for #2033.

[Crabzmatic]

Hi @jafermarq, @gsmalinovsky and I made a first draft of the pull request for TAMUNA algorithm. Could we discuss this with you in the near future?

[jafermarq]

Hi @Crabzmatic @gsmalinovsky , sure. Let's discuss over Slack for a time to chat?

[Crabzmatic]

I added a couple of extra minor comments. In addition to those could you:

• rename the docs directory you use for the images in the readme as _static, this will make it easier to integrate parts of your readme into the Flower docs.
• Since we created the baseline_template directory and its content we have made some changes to its pyproject.toml, most notably raising the mypy version. Could you update your pyproject.toml with the new content in https://github.com/adap/flower/blob/main/baselines/baseline_template/pyproject.toml?

done

[jafermarq]

Thanks for incorporating those changes.

I have done another pass and included some small comments. In addition to those:

• Are you planning to also include results from Figure 2&3 as discussed in TAMUNA #2033?
• I see the formatting script passes perfectly! Great!
• The tests script however (i.e. ./dev/test-baseline.sh) finds a few errors (mostly easy to fix)

[Crabzmatic]

Thanks for incorporating those changes.

I have done another pass and included some small comments. In addition to those:

• Are you planning to also include results from Figure 2&3 as discussed in TAMUNA #2033?
• I see the formatting script passes perfectly! Great!
• The tests script however (i.e. ./dev/test-baseline.sh) finds a few errors (mostly easy to fix)

• Neural network experiments on MNIST, which were not present in the paper, seem more logical to me as baselines in Flower. In my opinion, it makes sense to not include original logistical regression experiments on w8a and real-sim datasets in Flower. It would make more sense if Scaffnew, Scaffold and 5GCS were already implemented, but as of now neither of these algorithms are. I would be okay with adding them later once these are implemented. Let me know what you think about this.
• I fixed most of the mypy errors, one remains:

tamuna/strategy.py:152: error: Argument 2 to "FitIns" has incompatible type "dict[str, int]"; expected "dict[str, bool | bytes | float | int | str]"  [arg-type]
tamuna/strategy.py:152: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
tamuna/strategy.py:152: note: Consider using "Mapping" instead, which is covariant in the value type

If I understand this correctly, problem can be solved by changing type hint in the definition of FitIns (see https://stackoverflow.com/questions/73603289/why-doesnt-parameter-type-dictstr-unionstr-int-accept-value-of-type-di)

as for the pylint warnings, I fixed some that made more sense, these remain:

************* Module tamuna.client
tamuna/client.py:19:0: R0902: Too many instance attributes (9/7) (too-many-instance-attributes)
tamuna/client.py:24:4: R0913: Too many arguments (6/5) (too-many-arguments)
tamuna/client.py:133:8: C0103: Attribute name "lr" doesn't conform to snake_case naming style (invalid-name)
tamuna/client.py:122:4: R0913: Too many arguments (6/5) (too-many-arguments)
tamuna/client.py:201:0: C0103: Argument name "lr" doesn't conform to snake_case naming style (invalid-name)
************* Module tamuna.dataset_preparation
tamuna/dataset_preparation.py:103:0: R0913: Too many arguments (6/5) (too-many-arguments)
tamuna/dataset_preparation.py:103:0: R0914: Too many local variables (22/15) (too-many-locals)
tamuna/dataset_preparation.py:178:8: E1121: Too many positional arguments for method call (too-many-function-args)
************* Module tamuna.models
tamuna/models.py:72:0: C0103: Argument name "lr" doesn't conform to snake_case naming style (invalid-name)
tamuna/models.py:72:0: R0913: Too many arguments (10/5) (too-many-arguments)
tamuna/models.py:140:0: C0103: Argument name "lr" doesn't conform to snake_case naming style (invalid-name)
tamuna/models.py:174:0: C0103: Argument name "lr" doesn't conform to snake_case naming style (invalid-name)
tamuna/models.py:174:0: R0913: Too many arguments (6/5) (too-many-arguments)
tamuna/models.py:229:0: C0103: Argument name "lr" doesn't conform to snake_case naming style (invalid-name)
tamuna/models.py:229:0: R0913: Too many arguments (6/5) (too-many-arguments)
************* Module tamuna.strategy
tamuna/strategy.py:47:0: R0902: Too many instance attributes (8/7) (too-many-instance-attributes)
tamuna/strategy.py:50:4: R0913: Too many arguments (6/5) (too-many-arguments)

Let me know if I need to fix these too.

[jafermarq]

For mypy issue you could do:

local_epochs: Scalar = self.epochs_per_round
config = {"epochs": local_epochs}

after importing Scalar from flwr.common. I can't tell immediately why this error is being raised...

For the pylint you can add use the ignore syntax to skip all those saying too-many-.... Those for invalid-name, you can for instance replace lr with l_r.

[Crabzmatic]

For mypy issue you could do:

local_epochs: Scalar = self.epochs_per_round
config = {"epochs": local_epochs}

after importing Scalar from flwr.common. I can't tell immediately why this error is being raised...

For the pylint you can add use the ignore syntax to skip all those saying too-many-.... Those for invalid-name, you can for instance replace lr with l_r.

Done, test-baseline.sh script reports no errors/warnings now.

[jafermarq]

Hi @Crabzmatic,

I have left a couple of comments. The main one is regarding how you define strategies: I think it would be better if you build them on top of the native FedAvg (there will be less code and will make them easier to maintain in the long term). Let me know if you have any doubts on how to do this. At least your implementation of FedAvg should be fairly straightforward to implement using the native one and passing the on_fit_config and evaluate_fn arguments to it. Let me know what you think!

I think after that we can start wrapping this up and start the process of merging your baseline with Flower's main branch 🙌

[jafermarq]

Hi @Crabzmatic,

Just small comment. I think this looks ready to merge.

[jafermarq]

Hi @Crabzmatic,

Your baseline is ready to be merged. Just a couple of suggestions before going ahead:

• Let's not make use of client_device passed from the config to the functions that return client objects. Instead, you can let it be "discovered" (as suggested) based on the client_resources passed to start_simulation. It works fine. All but one suggestion below refer to this. you might need to re-run the formatting checks afterwards.
• Small change is needed to the authors field in the pyproject.toml.

Ping me once done, then we can merge Tamuna.

[jafermarq]

@Crabzmatic, this looks great! merging...