Skip to content

5821 6303 Optimize MonaiAlgo FL based on BundleWorkflow#6158

Merged
Nic-Ma merged 43 commits intoProject-MONAI:devfrom
Nic-Ma:5821-optimize-monaialgo
Apr 19, 2023
Merged

5821 6303 Optimize MonaiAlgo FL based on BundleWorkflow#6158
Nic-Ma merged 43 commits intoProject-MONAI:devfrom
Nic-Ma:5821-optimize-monaialgo

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Mar 16, 2023 •
edited
Loading

part of #5821
Fixes #6303

Description

This PR simplified the MONAI FL MonaiAlgo module to leverage BundleWorkflow.
The main point is to decouple the bundle read / write related logic with FL module and use predefined required-properties.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 16, 2023 •
edited
Loading

CC @holgerroth to the loop.
I think the MONAI FL module is still in "experimental" stage, so I made some slightly breaking change directly.
I already updated MonaiAlgoStats and its unit tests.

Thanks.

@Nic-Ma Nic-Ma changed the title [WIP] 5821 Optimize MonaiAlgo FL based on BundleWorkflow 5821 Optimize MonaiAlgo FL based on BundleWorkflow Mar 17, 2023
@Nic-Ma Nic-Ma marked this pull request as ready for review March 17, 2023 09:30
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 17, 2023

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 17, 2023

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 17, 2023

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 17, 2023

/build

@Nic-Ma Nic-Ma requested review from ericspod, holgerroth and wyli and removed request for holgerroth March 17, 2023 10:40
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 17, 2023

Hi @holgerroth ,

I updated the MonaiAlgo and MonaiAlgoStats in this PR, mainly note:

  1. It decoupled bundle config processing from MonaiAlgo.
  2. Bundle don't need to write in JSON or YAML config anymore, any SupervisedTrainer based BundleWorkflow can work in the MonaiAlgo.
  3. I expect to set BundleWorkflow as a NVFlare component in the fl_client config file, and pass it to MonaiAlgo as an object arg. @KumoLiu confirmed it's doable.
  4. I dropped 2 minor features: (1) disable checkpoint loader, I feel this feature is very hacky, better to let users manually disable it in the bundle config directly before running FL program. (2) no trainer and no evaluator but get_weights, I think get_weights should work at least with trainer exists. Please feel free to correct me if I misunderstand.

Could you please help review it?

Thanks in advance.

@Nic-Ma Nic-Ma requested a review from KumoLiu March 17, 2023 10:51
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Mar 20, 2023

/build

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 20, 2023

I think this pr will also close #4942?

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 7, 2023

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 7, 2023

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 9, 2023

/black

holgerroth
holgerroth reviewed Apr 11, 2023
View reviewed changes
@Nic-Ma Nic-Ma mentioned this pull request Apr 18, 2023
Closed
7 tasks
@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 18, 2023

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 18, 2023

/build

@Nic-Ma Nic-Ma requested a review from holgerroth April 18, 2023 07:56
holgerroth
holgerroth approved these changes Apr 18, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@holgerroth holgerroth left a comment

Choose a reason for hiding this comment

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

Looks great. Tested with nvflare 2.3.0 and works fine.

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Apr 18, 2023

/build

@Nic-Ma Nic-Ma enabled auto-merge (squash) April 18, 2023 23:54
@Nic-Ma Nic-Ma merged commit d8eb68a into Project-MONAI:dev Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@holgerroth holgerroth holgerroth approved these changes

@wyli wyli Awaiting requested review from wyli

@ericspod ericspod Awaiting requested review from ericspod ericspod is a code owner

@KumoLiu KumoLiu Awaiting requested review from KumoLiu KumoLiu is a code owner

Assignees

@Nic-Ma Nic-Ma

Labels

None yet

Projects

No open projects
MONAI v1.2
Status: Done

Milestone

Bundle class [P1 v1.2]

Development

Successfully merging this pull request may close these issues.

MonaiAlgo's convert_global_weights broken for multi-gpu clients

4 participants

@Nic-Ma @wyli @holgerroth @monai-bot