feat(networks): expose networkx helpers and plotting

[adrianardv]

Summary

• expose Network graph helpers (degrees, edges) and add layout-validated plotting
• allow benchmark problems to request plotting during network creation and auto-plot in distributed/FL networks
• update user docs for plotting options and docs config for matplotlib type alias/intersphinx
• closes Better usage of the network backend #206

[nicola-bastianello]

this is great! just a few comments

[Simpag]

Nice features, network plotting is really needed. I have some minor refactoring comments/discussions and a comment regarding our lovely friend Sphinx autodoc... I spent many hours figuring stuff out, which is terribly documented, so if you ever need help feel free to reach out regarding Sphinx

[Simpag]

1. Would be nice if you could make layout a Literal so typechecking can catch incorrect values. Or you could make a Union of the actual networkx layouts and remove your _LAYOUT_FUNCS (should work but I am not sure). This would also link the layouts in the docs

2. When you are using TYPE_CHECKING import sphinx wont see this import (as TYPE_CHECKING is False when sphinx autodocs) so move the import outside of type checking. Then you can remove the type alias and nitpick ignore in docs/source/conf.py so that the parameter is correctly linked. This is fine as matplotlib already is a requirement for this project. There are some other solutions if you want to keep the type checking import, see utils/types.py - ArrayLike, but this one is the easiest. The autodoc_type_aliases setting in conf.py does not work as one might want/expect to... It is meant to be used on typing.TypeAlias, its really confusing and strange (and unnecessarily restrictive). See https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_type_aliases

3. Maybe we should move this away from the network object and into a networks utilities module? I think that the network object should only deal with network related tasks and this is more of a visualization/utility task. This would also allow us to remove the plot_network and plot_network_kwargs parameters from benchmark problem as these are not really benchmark problem related. These would instead be parameters of create_network_... or benchmark functions. Any thoughts @adrianardv @nicola-bastianello

[nicola-bastianello]

regarding 3.: I like @Simpag suggestion, it makes sense to keep plotting as a separate util, and avoid having additional arguments when creating the benchmark problem. this way the users don't need to worry about network plotting unless they are interested.

I'm now thinking that we could even do the following: we don't implement the plotting interface in decent-bench, rather we show in the user guide how to apply networkx plotting to the network.graph attribute. I think the benefit of this is that we don't introduce any dependence on the specific plotting functions in networkx. this means that we don't need to go back and update code if networkx changes the way plotting is done/the args or kwargs of the plotting functions. also, in a sense any documenting we do about plotting is redundant with networkx docs

we could have the section in the user guide be something like "additional network utils" and describe there 1) how to plot, 2) anything else we think is good to mention on how to use networkx (we can add in the future, so for now 1) is enough)

[Simpag]

I think we must decide what decent bench should be. If its a pure benchmarking framework then we should not include plotting in my opinion. But if we are going to implement other optimization strategies like hyperparameter tuning then it might be nice to have it since decent bench becomes more of a utility than a pure benchmarking framework

[nicola-bastianello]

that's a good point. I do want to go beyond just benchmarking, but we need to be careful how we do that. in any case, for now we can keep the graph plotting functionality as a separate util, and we can come back to this discussion to decide what is the best way to expand functionality without creating an un-maintanable code base

[adrianardv]

Okey, for now, I pulled plotting out of Network and the benchmark flags, and added network_utils.plot_network as an optional helper.

[nicola-bastianello]

looks great, thank you!

[nicola-bastianello]

looking back at the communication methods in FedNetwork, I'm a bit worried that they are more confusing than beneficial; we have 7 methods, all with similar names (send_to_all_clients, send_from_client, send_from_all_clients, receive_at_client, receive_at_all_clients, receive_from_client, receive_from_all_clients).

my worry is that users will be overwhelmed by these, and that it would be easier to have fewer methods and leave the users to read the docs of those. I think having fewer communication functions would also help making algorithms' implementations more readable

any thoughts?

[Simpag]

I personally like to have helper functions. However the names are very similar and could be mistaken at a glance. Might also be overwhelming at first. Could change the ”send to all” functions to broadcast instead in order to distinguish them more easily. I have no ideas currently about the receive methods

[adrianardv]

I agree on shrinking the comm methods.

I'm thinking:

• Drop the single-client aliases: send_to_client/receive_at_client and send_from_client/receive_from_client.
• Rename send_to_all_clients → broadcast_to_clients, keep (or drop) receive_at_all_clients as the counterpart (or drop both if we want an even smaller surface).
• Keep a synchronous “all clients push to server” helper (send_from_all_clients). Change it to collect_from_all_clients or gather_from_all_clients and drop receive_from_all_clients:

def collect_from_all_clients(self, msgs: Mapping[Agent, Array]) -> None:
    """
    Send messages from each client to the server (synchronous FL push) and receive at server.
    """
    clients = set(self.clients)
    senders = set(msgs)
    invalid = senders - clients
    if invalid:
        raise ValueError("All senders must be clients")
    missing = clients - senders
    if missing:
        raise ValueError("Messages must be provided for all clients")

    for client, msg in msgs.items():
        self.send(sender=client, receiver=self.server, msg=msg)

    self.receive(receiver=self.server, sender=None)

If this sounds good, I’ll make the changes.

[nicola-bastianello]

thanks Adriana for the changes! see below for my comments

I agree on shrinking the comm methods.

I'm thinking:

* Drop the single-client aliases: `send_to_client`/`receive_at_client` and `send_from_client`/`receive_from_client`.

I agree

* Rename `send_to_all_clients` → `broadcast_to_clients`, keep (or drop) `receive_at_all_clients` as the counterpart (or drop both if we want an even smaller surface).

I'm thinking that we could call them broadcast and receive_all, same as in P2PNetwork; and I would make them so that they allow to pass either the server or a client (documenting this properly)

* Keep a synchronous “all clients push to server” helper (`send_from_all_clients`).  Change it to `collect_from_all_clients` or `gather_from_all_clients` and drop `receive_from_all_clients`:

I'm not sure this method would be needed; the reason is that all algorithms will be written from the perspective of agent i. I think we can remove it

[Simpag]

Maybe move network_utils to the utils folder

[adrianardv]

About this:

I'm thinking that we could call them broadcast and receive_all, same as in P2PNetwork; and I would make them so that they allow to pass either the server or a client (documenting this properly)

Happy to align names with P2P. I’m thinking:

• broadcast(sender, msg): if sender is the server → deliver to all clients. If sender is a client, in FL the only target is the server (so it’s not really a broadcast).
• receive_all(receiver): if receiver is the server → drain all client messages. If receiver is a client, the only possible sender is the server (so ‘all’ is just legacy naming).

Is this the behaviour you want, or did you have something else in mind @nicola-bastianello ?
Given FL is strictly server↔client (no client↔client), allowing a client argument feels a bit awkward.

I’m now thinking broadcast(msg) for server → all clients, and receive_all() for the server draining all client messages; the rest of the comms can use the base send/receive, with broadcast/receive_all as convenience aliases for those server behaviours.

Then, the comm. methods would be reduced to: the canonical send/receive; and broadcast and receive_all aliases, in both P2P and FL.

[nicola-bastianello]

very good points. I like your plan of having send/receive for the clients, and broadcast/receive_all for the server. let's go for that, thank you!

[nicola-bastianello]

just a couple of comments on the user guide, and then it's ready to be merged. Thanks!

[nicola-bastianello]

this import is not needed in this code

[nicola-bastianello]

in the docs "Network utilities" is a section with subsections "Create problems using existing resources" and "Create problems from scratch". I think a better order would be to place "Network utilities" just after "Create problems from scratch"