-
Notifications
You must be signed in to change notification settings - Fork 216
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
Implement task_group filtering for FedEval #1226
Implement task_group filtering for FedEval #1226
Conversation
f1fcdcf
to
93248b8
Compare
93248b8
to
baeae12
Compare
d63c561
to
2331f75
Compare
9aba086
to
e8ca274
Compare
193ddf7
to
d2b46f0
Compare
d2b46f0
to
57f5094
Compare
57f5094
to
10b6993
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @ishaileshpant, I think it's looking good at this stage!
I just have a couple of minor remaining comments:
tests/openfl/component/assigner/test_random_grouped_assigner.py
Outdated
Show resolved
Hide resolved
10b6993
to
c181635
Compare
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just modify based on Teo's suggestions.
d819f41
to
3792997
Compare
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
233feed
to
db33263
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @ishaileshpant. The scenario of multiple user defined task groups needs to be considered before this can be approved. Details are provided in the comments. I can go into more depth on the Assigner design during our standup as well.
…group' in Assigner class - update all the sub-classes that use task_groups to use the decorator - update fedeval sample workspace to use default assigner, tasks and aggregator - use of federated-evaluation/aggregator.yaml for FedEval specific workspace example to use round_number as 1 - removed assigner and tasks yaml from defaults/federated-evaluation, superseded by default assigner/tasks - added additional checks for assigner sub-classes that might not have task_groups - Addressing review comments - Updated existing test cases for Assigner sub-classes - Remove hard-coded setting in assigner for torch_cnn_mnist ws, refer to default as in other Workspaces - Use aggregator supplied --task_group to override the assinger selected_task_group - update existing test cases of aggregator cli - add test cases for the decorator - rebased 25-Jan.1 - implemented the support of multiple task_group without selection - defaulting of selected_task group 'percentage' to 1.0 post successful filtering - updated test cases for multiple task group support Signed-off-by: Shailesh Pant <shailesh.pant@intel.com>
db33263
to
0b51e3d
Compare
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
tests/openfl/component/assigner/test_random_grouped_assigner.py
Dismissed
Show dismissed
Hide dismissed
@kta-intel for the last task as discussed in #1258 i've created an issue #1321 and will track that fix via that |
implement a new task_group filtering decorator 'with_selected_task_group' in Assigner class
update all the sub-classes that use task_groups to use the decorator
update fedeval sample workspace to use default assigner, tasks and aggregator
use of federated-evaluation/aggregator.yaml for FedEval specific workspace example to use round_number as 1
removed assigner and tasks yaml from defaults/federated-evaluation, superseded by default assigner/tasks
added additional checks for assigner sub-classes that might not have task_groups
Addressing review comments
Updated existing test cases for Assigner sub-classes
Remove hard-coded setting in assigner for torch_cnn_mnist ws, refer to default as in other Workspaces
Use aggregator supplied --task_group to override the assinger selected_task_group
update existing test cases of aggregator cli
add test cases for the decorator
rebased 25-Jan.1
Signed-off-by: Shailesh Pant shailesh.pant@intel.com
Testcases for task_group_filtering decorator
Testing for all existing workspaces
we shouldn't be saving any new models if running in evaluation mode.
Change all workspaces to use new Assigner [Not needed as defaults ensure all workspace, by default use new assigner]