Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/SK-521 | Global model not created if the combiner terminates based on timeout #478

Merged
merged 23 commits into from
Nov 9, 2023

Conversation

ahellander
Copy link
Member

@ahellander ahellander commented Aug 22, 2023

Description

Rewrites some of the polling in the controller in a more robust way. Increases robustness and removes risks of race condition when combiner times out.

@ahellander ahellander requested a review from Wrede August 22, 2023 13:37
@Wrede Wrede changed the title Feature/sk 521 Feature/SK-521 | Global model not created if the combiner terminates based on timeout Oct 3, 2023
@ahellander ahellander removed the HOLD label Oct 19, 2023
Copy link
Member

@Wrede Wrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the "status" update to rounds. We should also add session_id as a field in rounds in statestore. Regarding the implementation of round, it's quite hard to read, I would say one reason for this are the nested functions. I have added many comments, also please add docs strings, we want to have a complete API ref for v1.0

fedn/fedn/common/tracer/mongotracer.py Outdated Show resolved Hide resolved
fedn/fedn/common/tracer/mongotracer.py Show resolved Hide resolved
fedn/fedn/common/tracer/mongotracer.py Show resolved Hide resolved
fedn/fedn/common/tracer/mongotracer.py Show resolved Hide resolved
fedn/fedn/common/tracer/mongotracer.py Show resolved Hide resolved
fedn/fedn/network/controller/controlbase.py Outdated Show resolved Hide resolved
fedn/fedn/network/controller/controlbase.py Outdated Show resolved Hide resolved
fedn/fedn/network/controller/controlbase.py Outdated Show resolved Hide resolved
fedn/fedn/network/controller/controlbase.py Show resolved Hide resolved
fedn/fedn/network/controller/controlbase.py Outdated Show resolved Hide resolved
@ahellander ahellander requested a review from Wrede October 30, 2023 13:36
@Wrede
Copy link
Member

Wrede commented Oct 30, 2023

@ahellander can you please write a better description of the PR, with more information what in introduces/fixes?

@ahellander
Copy link
Member Author

Ok, I have updated the message.

round_data['round_config'] = round_config
# Wait until participating combiners have produced an updated global model,
# or round times out.
def do_if_round_times_out(result):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahellander why wouldn't the library be able to use functions in the global scope?

@Wrede Wrede merged commit ae06b84 into develop Nov 9, 2023
15 checks passed
@Wrede Wrede deleted the feature/SK-521 branch November 9, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants