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

[2.4] Integrated ReliableMessage with XGBoost #2399

Merged
merged 8 commits into from
Mar 16, 2024
Merged

[2.4] Integrated ReliableMessage with XGBoost #2399

merged 8 commits into from
Mar 16, 2024

Conversation

nvidianz
Copy link
Collaborator

@nvidianz nvidianz commented Mar 13, 2024 •
edited

Description

Integrated ReliableMessage with XGBoost. By default, the simple sender will be used. To use ReliableMessage, add this component and specify the sender_id in executor,

        {
            "id": "reliable_sender",
            "path": "nvflare.apis.utils.reliable_sender.ReliableSender",
            "args": {
                "max_request_workers": 16
            }
        }

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.

@nvidianz nvidianz force-pushed the flare-1846-add-xgb-reliable-sender branch from 3f846fa to 3cd2bd2 Compare March 13, 2024 15:25
@nvidianz nvidianz force-pushed the flare-1846-add-xgb-reliable-sender branch from 3cd2bd2 to aa821b8 Compare March 13, 2024 15:45
yanchengnv
yanchengnv requested changes Mar 13, 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.

See my comments.

nvflare/app_opt/xgboost/histogram_based_v2/adaptor_controller.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/sender.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/sender.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/sender.py Outdated Show resolved Hide resolved
yanchengnv
yanchengnv requested changes Mar 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 change sender signature as commented.

@nvidianz nvidianz force-pushed the flare-1846-add-xgb-reliable-sender branch from 81d58fa to 663a60e Compare March 14, 2024 20:08
yanchengnv
yanchengnv requested changes Mar 15, 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.

Don't think RequestSender is needed.

nvflare/app_opt/xgboost/histogram_based_v2/request_sender.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/request_sender.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/request_sender.py Outdated Show resolved Hide resolved
@chesterxgchen chesterxgchen changed the title Integrated ReliableMessage with XGBoost [2.4] Integrated ReliableMessage with XGBoost Mar 15, 2024
@nvidianz nvidianz force-pushed the flare-1846-add-xgb-reliable-sender branch 2 times, most recently from 750666b to 3670222 Compare March 15, 2024 20:27
YuanTingHsieh
YuanTingHsieh reviewed Mar 15, 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 LGTM, Have a question

nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Show resolved Hide resolved
@nvidianz nvidianz force-pushed the flare-1846-add-xgb-reliable-sender branch from 3670222 to fe31704 Compare March 15, 2024 20:57
yanchengnv
yanchengnv requested changes Mar 15, 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.

See comments.

nvflare/apis/utils/sender.py Outdated Show resolved Hide resolved
nvflare/apis/utils/sender.py Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor_executor.py Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptors/grpc_client_adaptor.py Outdated Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/sender.py Show resolved Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptor.py Show resolved Hide resolved
yanchengnv
yanchengnv previously approved these changes Mar 15, 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

yanchengnv
yanchengnv previously approved these changes Mar 15, 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

@YuanTingHsieh
Copy link
Collaborator

/build

@nvidianz nvidianz merged commit 0966722 into NVIDIA:2.4 Mar 16, 2024
16 checks passed
@nvidianz nvidianz deleted the flare-1846-add-xgb-reliable-sender branch March 16, 2024 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuanTingHsieh YuanTingHsieh YuanTingHsieh approved these changes

@yanchengnv yanchengnv yanchengnv left review comments

@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.

None yet
3 participants
@nvidianz @YuanTingHsieh @yanchengnv