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.

Sign up for GitHub

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

Jump to bottom

FOBS Enhancement #2580

Merged
merged 19 commits into from
Jul 9, 2024
Merged

FOBS Enhancement #2580

merged 19 commits into from
Jul 9, 2024

Conversation

nvidianz
Copy link
Collaborator

@nvidianz nvidianz commented May 14, 2024 •
edited
Loading

Added 2 features to FOBS,

  1. On serializing, DataClassDecomposer will be registered for an object if no decomposer is registered for this class. This feature can be turned off by calling fobs.auto_register_data_classes(False)
  2. Decomposer registration is not needed on deserializing. The decomposer is stored in the serialized data and is automatically registered.

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
yanchengnv requested changes May 14, 2024
View reviewed changes
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 improvement.

nvflare/fuel/utils/fobs/fobs.py Show resolved Hide resolved
@nvidianz nvidianz requested a review from yanchengnv May 14, 2024 19:54
@nvidianz nvidianz force-pushed the flare-1967-fobs-enhancement branch from 49d622a to 5378841 Compare May 14, 2024 20:19
@nvidianz
Copy link
Collaborator Author

/build

@nvidianz nvidianz force-pushed the flare-1967-fobs-enhancement branch from a3a6fb3 to 210ea08 Compare May 22, 2024 14:46
@nvidianz
FOBS enhancements: 1) Added auto-registration of data class decompose…
b733df8 
…rs. 2). No need to register decomposers on deserializing
@nvidianz nvidianz force-pushed the flare-1967-fobs-enhancement branch from fb48d14 to b733df8 Compare May 22, 2024 14:49
@nvidianz
Copy link
Collaborator Author

/build

1 similar comment
@nvidianz
Copy link
Collaborator Author

/build

YuanTingHsieh
YuanTingHsieh reviewed May 23, 2024
View reviewed changes
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 good, have some questions

nvflare/fuel/utils/fobs/fobs.py Show resolved Hide resolved
nvflare/fuel/utils/fobs/fobs.py Show resolved Hide resolved
nvflare/fuel/utils/fobs/fobs.py Show resolved Hide resolved
yanchengnv
yanchengnv approved these changes Jul 3, 2024
View reviewed changes
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

@nvidianz
Copy link
Collaborator Author

nvidianz commented Jul 9, 2024

/build

@nvidianz nvidianz merged commit 56c9c4d into NVIDIA:main Jul 9, 2024
15 of 16 checks passed
@nvidianz nvidianz deleted the flare-1967-fobs-enhancement branch July 9, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuanTingHsieh YuanTingHsieh YuanTingHsieh left review comments

@yanchengnv yanchengnv yanchengnv approved these changes

@chesterxgchen chesterxgchen Awaiting requested review from chesterxgchen

@yhwen yhwen Awaiting requested review from yhwen

@IsaacYangSLA IsaacYangSLA Awaiting requested review from IsaacYangSLA

@SYangster SYangster Awaiting requested review from SYangster

@nvkevlu nvkevlu Awaiting requested review from nvkevlu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nvidianz @YuanTingHsieh @yanchengnv