Repository navigation
Decentralized: Replace ConnectionFail with CrashClient - #1276
leonard-amsler wants to merge 7 commits into
Conversation
JulienVig
left a comment
JulienVig
left a comment
There was a problem hiding this comment.
Thanks for the follow-up!
I think that the current way to abort waiting for getPeers could be too superficial. Even with the abort signal the method can continue trying to establish webRTC connections
I have published a PR improving the robustness of decentralized learning, notably by creating a stopSignal that combines crash signal and disconnected signal (new one). Can you rebase your PR onto it so that you can replace this.crashSignal by this.stopSignal?
| const ws = this.connections.get(nodeId); | ||
| // Cannot happen because a node leaving the session | ||
| // is also removed from #connectFinishedNodes | ||
| if (ws === undefined) return; |
There was a problem hiding this comment.
If something that can't happen happens then should we either fail with an error or at least log that it happened
There was a problem hiding this comment.
Makes sense, let's make it crash then.
| } else if ( | ||
| e instanceof Error && | ||
| e.message.includes("Client disconnected after connection failure") | ||
| ) { | ||
| toaster.error( | ||
| "Client disconnected after multiple peer connection failure. Please rejoin the training.", | ||
| ); |
There was a problem hiding this comment.
Why not keep this toaster error and just update the string to match?
| // weight update and adds it to our aggregator upon reception | ||
| (conn) => this.receivePayloads(conn), | ||
| ), | ||
| this.crashSignal, |
There was a problem hiding this comment.
Could you rebase this branch on mine (#1277) and use this.stopSignal instead here?
| (conn) => this.receivePayloads(conn), | ||
| // Making it abortable with the crash signal, | ||
| // otherwise the client may be stuck waiting for connections indefinitely | ||
| const connections = await abortable( |
There was a problem hiding this comment.
Can you add some tests for this?
| this.#pool.getPeers( | ||
| peers, | ||
| this.server, | ||
| // Init receipt of peers weights. this awaits the peer's | ||
| // weight update and adds it to our aggregator upon reception | ||
| (conn) => this.receivePayloads(conn), | ||
| ), |
There was a problem hiding this comment.
If an abort signal comes up, we only stop waiting for the promise but the peer connection loop keeps running in this.#pool.getPeers(). You could pass the signal as an argument to getPeers so that it can interrupt its loop
52570e5 to
4701566
Compare
Decentralized training had two different messages for the server to remove a client:
CrashClientandConnectionFail. This PR removesConnectionFailand usesCrashClientfor both.Changes
ConnectionFail, it now uses thecrashClient()helper to notify the clients about the crash and also closes their socket.getPeerswhen it is told to crash, so a client stuck connecting to peers fails with aClientCrashErrorinstead of waiting forever.MType.ConnectionFailand the webapp error branch for that error. Updated the documentation to match the implementation.CrashClientmessage and that their WebSocket is closed.