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

Fobs auto register #2567

Merged
merged 23 commits into from May 20, 2024
Merged

Fobs auto register #2567

merged 23 commits into from May 20, 2024

Conversation

yhwen
Copy link
Collaborator

@yhwen yhwen commented May 8, 2024

Fixes # .

Description

Add the ability to automatically register all the Fobs decomposers.

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.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

@yanchengnv
Copy link
Collaborator

Why do we need to do this now? If not functionally required, we should wait until after 2.5.0.

@chesterxgchen
Copy link
Collaborator

chesterxgchen commented May 8, 2024 via email

Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

should we add unit tests to show that certain components are registered and available for decomposers, and certain objects are not

nvflare/fuel/utils/fobs/fobs.py Outdated Show resolved Hide resolved
nvflare/apis/utils/decomposers/flare_decomposers.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@chesterxgchen chesterxgchen left a comment

Choose a reason for hiding this comment

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

please add unit tests, looks good in general

@yhwen
Copy link
Collaborator Author

yhwen commented May 10, 2024

Added unit test.

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

Looks good in general. Please see my comments for changes.

nvflare/client/flare_agent.py Show resolved Hide resolved
nvflare/fuel/utils/fobs/fobs.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/server/server_train.py Outdated Show resolved Hide resolved
nvflare/private/fed/app/client/client_train.py Outdated Show resolved Hide resolved
nvflare/private/fed/utils/fed_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, Yan has a point if we need to register custom decomposer at SP/CP.

What are the cases when users will send back custom data types to SP/CP?

@yhwen
Copy link
Collaborator Author

yhwen commented May 16, 2024

Removed the custom decomposers register for SP and CP.

Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

LGTM

@yhwen
Copy link
Collaborator Author

yhwen commented May 20, 2024

/build

1 similar comment
@yhwen
Copy link
Collaborator Author

yhwen commented May 20, 2024

/build

@yhwen yhwen merged commit 03f6a00 into NVIDIA:main May 20, 2024
16 checks passed
nvidianz pushed a commit to nvidianz/NVFlare that referenced this pull request May 22, 2024
* WIP: auto register fobs.

* WIP

* Made changes to server and client fobs_initialize().

* Added auto fobs regiater.

* codestyle fix.

* Adjust to use register_nvflare_decomposers() for IPCAgent.

* add some cleanup.

* codestyle fix.

* Codestyle fix.

* re-design the custom Fobs register to only scan from nvflare_decomposers folder, and user defined decomposer_module.

* removed no use import.

* Added unit test for custom_fobs_initialize().

* Addressed the PR reviews.

* changed a log warning message.

* Added None handling for register_ext_decomposers.

* Fixed unit test.

* fixed unit test.

* fix unit test.

* codestyle fix.

* Removed the custom decomposer regoister for SP and CP. Added the register for simulator_worker.

* removed no use import.

---------

Co-authored-by: Yuan-Ting Hsieh (謝沅廷) <yuantingh@nvidia.com>
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.

None yet

4 participants