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] Timing Fed Components #530

Merged
merged 8 commits into from Dec 14, 2022

Conversation

sun1lach
Copy link
Collaborator

@sun1lach sun1lach commented Oct 10, 2022

Core timeout functionality is in place.
To apply timeouts to any function/method in openfl codebase, import and decorate the function with @fedtiming.

from openfl.utilities.fed_timer import fedtiming

@fedtiming(timeout=10)    #10 seconds
def some_function(*args, **kwargs):
    pass

Update: 20th Oct

The @fedtiming can now be decorated on normal python functions or async co-routine likewise. Usage remains the same.

@sun1lach sun1lach self-assigned this Oct 10, 2022
@psfoley psfoley changed the title Implemented fed component timeout decorator WIP: Implemented fed component timeout decorator Oct 10, 2022
Copy link
Contributor

@igor-davidyuk igor-davidyuk left a comment

Choose a reason for hiding this comment

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

Consider using functool.wraps to keep function method names for logging and debugging.
https://stackoverflow.com/questions/308999/what-does-functools-wraps-do

@sun1lach
Copy link
Collaborator Author

@igor-davidyuk
@psfoley

Extended the capability of the @fedtiming decorator to support asynchronous co-routine methods as well.
Tested with below async methods in director.py

  • start_experiment_execution_loop
  • wait_experiment

Usage remains the same. Decorate the async methods with
@fedtiming(timeout=<seconds>)

Copy link
Contributor

@igor-davidyuk igor-davidyuk left a comment

Choose a reason for hiding this comment

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

  1. Please scan this module with flake8, it will highlight a lot of issues.
  2. Please add unit tests covering different use cases.

@sun1lach
Copy link
Collaborator Author

@igor-davidyuk @psfoley

  • Refactored from decorator function to decorator class.
  • Removed redundant and repetitive code.
  • sync and async decorator factory for better managing decorator state.
  • Implemented contextmanager to better handle repetitive/redundant code across async and sync wrappers.
  • flake8 compliant.

@igor-davidyuk
Copy link
Contributor

  • flake8 compliant.

Looks like our flake8 config is broken. There is no way flake8 would pass with no comment lines

@sun1lach
Copy link
Collaborator Author

  • flake8 compliant.

Looks like our flake8 config is broken. There is no way flake8 would pass with no comment lines

I see!
I tested the module with the by default/standard flake8 config in my local.
Do let me know if there is any custom flake8 config used in the OpenFL CI to make it compliant, I can validate the same in my local.

Also yes, will add comments and docstrings :)

@sun1lach
Copy link
Collaborator Author

Looks like by default flake doesn't capture naming_conventions and docstrings etc according to pycodestyle.
https://stackoverflow.com/questions/60403545/flake8-not-giving-errors-warnings-on-missing-docstring-or-code-not-following-pep

@sun1lach sun1lach marked this pull request as ready for review November 3, 2022 07:18
@sun1lach sun1lach changed the title WIP: Implemented fed component timeout decorator [Feature] Timing Fed Components Nov 3, 2022
@psfoley psfoley added this to To do in Sprint 6B Nov 8, 2022
@sun1lach
Copy link
Collaborator Author

This change can be merged independently.

Static timeout values defined in the director_config.yml would require code change in yml parse logic and in any one of the interactive example.

@sun1lach sun1lach moved this from To do to Done in Sprint 6B Dec 10, 2022
@sun1lach sun1lach force-pushed the timing_federation branch 3 times, most recently from 1d61ff5 to 0e0f601 Compare December 10, 2022 08:25
Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
Copy link
Contributor

@psfoley psfoley left a comment

Choose a reason for hiding this comment

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

@acharyasunil the changes look good on my end. Could you also add documentation for how someone would apply this feature to debug infrastructure / performance issues?

Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
@psfoley
Copy link
Contributor

psfoley commented Dec 13, 2022

  1. Please scan this module with flake8, it will highlight a lot of issues.
  2. Please add unit tests covering different use cases.

From @igor-davidyuk's comment, @acharyasunil It looks like all that is missing at this point are unit tests. After that, this is ready to merge.

Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
openfl/utilities/fed_timer.py Show resolved Hide resolved
openfl/utilities/fed_timer.py Outdated Show resolved Hide resolved
openfl/utilities/fed_timer.py Outdated Show resolved Hide resolved
openfl/utilities/fed_timer.py Outdated Show resolved Hide resolved
tests/openfl/utilities/test_fed_timer.py Outdated Show resolved Hide resolved
tests/openfl/utilities/test_fed_timer.py Show resolved Hide resolved
@psfoley psfoley merged commit afe52cc into securefederatedai:develop Dec 14, 2022
Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
Signed-off-by: Sunil Acharya (sunil.acharya@intel.com) <sunil.acharya@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants