Side effect driven design

[Nintorac]

Just trying to start a discussion here and get some inputs.

Working with the codebase I have come across several instances where state is being introduced to classes that isn't scoped by the calling functions. Done correctly this is probably fine but as the codebase grows more complex there is more opportunity to be bitten by some of the side effects

This does a few things

1. reduces surface area for bugs
2. makes it easier to follow program flow
3. reduces surface area for vulnerabilities

From what I see in a lot of the examples there is a lot of uses of side effects in this codebase.

Some examples:

• Cross Site model eval replaces the data of an already initialized DXO in a callback and removes the data afterwards.
• Weights aggeragator stores a list on itself that it appends weights to

In terms of solutions I am not overly confident in my own ideas here but it seems to me that some sort of async programming constructs would be invaluable for this project.

[holgerroth]

@yanchengnv @YuanTingHsieh @yanchengnv, moved this topic to discussion.

[yanchengnv]

This is a good/big topic. In any system development, there are intentional design decisions, and there are accidental decisions (side effects?) that just happened as part of the implementation detail. Intentional design decisions usually don't change easily (or major break would happen), whereas accidental decisions are subject to change.

NVFLARE Core (the whole NVFLARE codebase minus app_common) itself tries to make as few major decisions as possible (so it can give maximum freedom to the applications). The examples you gave actually belong to app area (that's why they are in app_common). Regardless, they need to make design decisions, and apps built on top of them must live with their decisions.

If you only look at the code without knowing the reasons of the coding, it won't be easy to know what is intentional (so you can depend on) and what is not. That's why we provided Best Practice for programming with NVFLARE. Still there are considerations that are not covered.

As to async programming construct, NVFLARE provides the Responder API. During run time, NVFLARE is essentially (async) message-driven in that the server responds to messages from clients. But Responder is hard for researchers for workflow development, since it is not the programming paradigm they are used to (procedural). Hence we built the Controller API on top of Responder, which gives the users the feel of procedural programming. But if you look into the detail of Controller API implementation, everything is async.

You can write a workflow as a Responder (instead of Controller). In fact, this is how we implemented the SAG behavior in old days.

[Nintorac]

Thanks this helped to clarify some thoughts.

I think it is still not possible to write a clean workflow even with the Responder API. You still need to write the process_task_request and process_submission functions which are called in separate threads when the client decides.

Perhaps my true issue is that the control flow is inverted. Really you want the server calling the shots. In such a situation writing a workflow is very simple for example.

def run_round(remote_sites, model_weights):
    task = Task("train", model_weights)
    results = []
    for site in remote_sites:
        result_weights.append(site.execute(task))
    model_weights = fedavg(results)

    return model_weights

In order to invert the control flow like this I think it would be necessary to rewrite the FederatedServer.

Some very rough pseudocode, there is probably a much cleaner way to do this without using nonlocal or asyncio.Event

async def registered_client():
    """
    Helper function to invert client-server 
    control
    
    """
    task_ready = asyncio.Event()
    task_complete = asyncio.Event()

    task = None
    result = None

    async def execute(task_item):
        nonlocal task, result

        # set the task object and signal task is ready
        task = task_item
        task_ready.set()

        # yield control until receive_result is executed
        await task_complete

        return result

    async def get_task():
        
        # yield control until a task is ready
        await task_ready

        return task

    async def receive_result(received_result):
        
        nonlocal result
        #store the result
        result = result
        
        # return control to the execute function
        task_complete.set()
    
    RegisteredClient = namedtuple('Client', ('execute', 'get_task', 'receive_result'))
    return RegisteredClient(execute, get_task, receive_result)


class AsyncServer(BaseServer, fed_service.FederatedTrainingServicer, admin_service.AdminCommunicatingServicer):

    clients = {}

    def Register(self, request, context):
        client_name = get_name(request)
        self.clients[client_name]=register_client()

    async def GetTask(self, request, context):

        get_task = self.clients[request.client].get_task
        return await get_task()

    def SubmitUpdate(self, request, context):
        
        receive_result = self.clients[request.client].receive_result
        receive_result(result)

        client_name = get_name(request)
        # refresh registered_client object for next task
        clients[client_name] = registered_client()

then rewrite your run_round function like so

async def run_round(remote_sites, model_weights):
    task = Task("train", model_weights)
    results = []
    for site in remote_sites:
        result_weights.append(site_a.execute(task))
    model_weights = fedavg(asyncio.gather(results))

Hope this make sense!

[yanchengnv]

Not sure I totally understand, but the control logic is already "almost" like what you suggested.

• The server is calling the shot. That's why the Controller/Responder runs on Server. Server creates and assigns tasks to Clients. If you take a look at the ScatterAndGather Controller in app_common/workflows, you can see it.

• The Responder is async in that it responds to Client's messages (which come in separate threads). Client only sends two kinds of messages: asking for a task, reporting result of an executed task. That's why the Responder must provides callbacks to process these two kinds of messages.

[Nintorac]

but the control logic is already "almost" like what you suggested

Not really, as you here in ScatterAndGather the control flow does a broadcast, the control logic for before and after are put into callbacks and the expected design pattern is that you modify state on your class which is then available to work on once your wait concludes.

For a researcher who then comes along and wants to write their own workflow they must be really careful about maintaining thread safety and also that their callbacks cannot upset the overall state of the controller/aggregator object. This isn't great IMO due to the side effect driven nature that I referred to in the OP.

The server is calling the shot. That's why the Controller/Responder runs on Server

No the server is just responding to the clients when they decide to phone in. So in the task wait phase the clients are in control. I guess this is kind of semantics though. What I want to see is a way to send tasks from the server thread directly, eg. the run_round function mentioned.

The Responder is async

Not really, the responder is using threads, similar but not the same. I don't think we are on the same page atm when when we talk about async.

Simplified SO explanation
More in depth SO explanation

[yanchengnv]

Thanks for the feedback.

You are right that we may not be on the same page about these things, which is fine.

The key to NVFLARE design is simply this:

• There are parties that know how to do things (with skills like performing a training task);
• FL is about collaboration of these skilled parties
• The collaboration needs to be coordinated/controlled).

The parties that have skills are FL Clients. The coordination is done by the controller running on the FL Server.

When I say "server is calling the shot", I'm not talking about who initiates the message at all. It is about the Server is determining what each client is supposed to do.

The underlying communication is client/server: the server listens on opened ports; whereas clients connect and initiate requests.

I'm sure there are other ways (better ways perhaps) to implement this kind of conceptual Controller/Worker task-based interaction/coordination.

BTW, all these before/after callbacks are indeed thread-based (well due to the nature of multi-threaded message driven), bu they are optional for efficiency optimization. If you look closely at how SAG supports aggregation. You can totally just wait for all results from all clients and the aggregate on received results. This could impose a big memory pressure when the result is huge (over 300M for medical imaging DL), and there are many clients. This is exactly one of the issues we encountered before. With the callback mechanism, you can do in-time aggregation.

You are also right in that multi-thread programming needs extra caution, which may be challenging for research community. We are happy to hear ideas for improving.

NVFLARE is designed to work for real-world apps over internet, which is not always stable. It is not always possible for the server to send a task to client, if the client is not even connected. For example, the targets of broadcast is not totally known at the time of broadcast call, since clients can join/connect at any time. If you look at the Controller API implementation, there is a lot of consideration on this.

[Nintorac]

You are right that we may not be on the same page about these things, which is fine.

Sorry I didn't mean to say we have a difference of opinion, I am not sure we are talking about the same subject

I am talking about a program design pattern that facilitates concurrency in a way that does not require side effects and maintains a linear flow of control which reduces surface area for bugs.

Here is the python library that implements that design pattern https://docs.python.org/3/library/asyncio.html

I hope that clears things, if I'm incorrect and we are both talking about the same thing. Then please could you go over why using callbacks and side effects is justified in this situation and why the common pattern does not apply.

server is calling the shot

Again sorry I am not clear here. I think what I mean to say is that clients thread is directly making changes to state that is in use by the workflow thread. This is known as a side effect, specifically it break referential transparency https://en.wikipedia.org/wiki/Referential_transparency

Since there is some strong security requirements for the app it is best to avoid such techniques as they make testing and verification difficult

Server is determining what each client is supposed to do. The underlying communication is client/server

Would you agree that this is backwards to normal? Normally clients determine what a server should do, eg my web browser is a client that talks to many servers to download images, text and javascript in order to display a page.

Really there is a bipartite relationship here, like you said there are the parties who know how to do things, but also there are the parties who want things to be done. In nvflare we have the implicit assumption that there is only one such party... this is perhaps a different conversation though.

could impose a big memory pressure

Anything possible with callbacks will be possible with async

---

Overall I think the messaging system approach is a good one. Flower https://flower.dev (what I see as the leading competitor in this space) goes the other approach, it's underlying library Ray uses gRPC calls directly which I don't see being able to scale as well since it must maintain a connection to each client for the duration of the computation which I'm sure you'll agree isn't good.

[Nintorac]

https://discuss.python.org/t/what-are-the-advantages-of-asyncio-over-threads/2112

Some discussion on the subject. You see how at the minimum you can clean up all the check abort calls with this kind of design.

And some info from 0mq guide

If you’ve spent years learning tricks to make your MT code work at all, let alone rapidly, with locks and semaphores and critical sections, you will be disgusted when you realize it was all for nothing. If there’s one lesson we’ve learned from 30+ years of concurrent programming, it is: just don’t share state. It’s like two drunkards trying to share a beer. It doesn’t matter if they’re good buddies. Sooner or later, they’re going to get into a fight. And the more drunkards you add to the table, the more they fight each other over the beer. The tragic majority of MT applications look like drunken bar fights.

• https://zguide.zeromq.org/docs/chapter2/#Multithreading-with-ZeroMQ

The key line here is just don’t share state

[chesterxgchen]

@Nintorac very thoughtful and interesting discussion. although this discussion happens before I join the project. It still interesting to read through the thread. you seem to promoting the function program style ( no state, no site effect). I need to read few more time to capture all the details of the thoughts. thank you