-
Notifications
You must be signed in to change notification settings - Fork 182
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
[2.4] Remove ipcl requirement for other training mode except vertical secure #2672
[2.4] Remove ipcl requirement for other training mode except vertical secure #2672
Conversation
…ing nvflare plugin
nvflare/app_opt/xgboost/histogram_based_v2/sec/client_handler.py
Outdated
Show resolved
Hide resolved
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.
See my comments.
nvflare/app_opt/xgboost/histogram_based_v2/sec/client_handler.py
Outdated
Show resolved
Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/sec/client_handler.py
Outdated
Show resolved
Hide resolved
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.
See my comments about the aborting process.
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.
Very nice!
See my comment about Yuan-Ting's PR #2674, especially the changes to notify_client_done. Make sure that system_panic() is not called multiple times.
If you put all his changes to your PR, then his PR won't be needed.
Please coordinate with YT.
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
/build |
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.
Need to change XGBController accordingly since we changed the behavior of self._abort() of SecurityHandler. See my comments for detail.
/build |
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
Fixes FLARE-2053
Description
Made changes so ipcl-python package is only required for vertical secure training using nvflare plugin.
Types of changes
./runtest.sh
.