Fix decentralized learning fail #708
Conversation
JulienVig
commented
Jul 16, 2024
•
JulienVig
commented
- Improve the aggregator's logic
- Fix the displayed number of participants in federated and decentralized learning. Closes The number of participants is wrong for both federated and decentralized learning #707
- Closes Decentralized training on MNIST fails with error #694 and Closes Decentralized learning fails with error #667
- Add CLI support for decentralized tasks
| this.aggregationResult, | ||
| timeout(undefined, "Timeout waiting on the aggregation result promise to resolve") | ||
| ]) | ||
| } catch (e) { |
There was a problem hiding this comment.
hum, so if this timeouts, we will still procede to the next communication round, but the payload will be invalid (already used by current round and very probably changed by it). I think breaking would be safer.
also, by creating the timeout in there, it is a timeout per communication round, not for the whole onRoundEnd, which is a bit unexcepted.
There was a problem hiding this comment.
Ah yes I agree we should break/return here. However I don't see why a timeout per communication round is unexpected. Shouldn't we timeout whenever we may wait on a promise forever?
There was a problem hiding this comment.
However I don't see why a timeout per communication round is unexpected. Shouldn't we timeout whenever we may wait on a promise forever?
yes, we should timeout for sure, but as each aggregator has a various number of communication round, the "onRoundEnd" timeout varies here which I find a bit weird. you can create the timeout promise at the beginning of the function and reference the same timeout in every communication round.
but yeah, maybe I'm overthinking this (won't be the first time), if you don't find it strange, let's keep it as it is (we don't really have a policy on timeouts anyway).
