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

Restore non-aio GRPC and a few improvements #2058

Merged
merged 11 commits into from
Oct 6, 2023
Merged

Restore non-aio GRPC and a few improvements #2058

merged 11 commits into from
Oct 6, 2023

Conversation

yanchengnv
Copy link
Collaborator

@yanchengnv yanchengnv commented Oct 4, 2023 •
edited
Loading

Fixes # .

Description

  • Restore non-asyncio GRPC driver. A site can choose which driver to use by setting "use_aio_grpc" in comm_config.json, or via system environment variable NVFLARE_USE_AIO_GRPC. The default is True to be backward compatible
  • Consolidated common functions used by grpc driver and aio-grpc driver.
  • Removed unnecessary use of multiprocess since it could cause semaphore leak at the end of process.
  • Changed to directly send messages to server instead of using pool map.
  • Fixed CP/SP heartbeat bug and use small timeout value.
  • Added retry logic for sending task results from CJ to SJ.
  • Fixed download_job bug in flare_api

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.

IsaacYangSLA
IsaacYangSLA previously approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

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

nvidianz
nvidianz previously approved these changes Oct 4, 2023
View reviewed changes
@IsaacYangSLA
Copy link
Collaborator

/build

@yanchengnv yanchengnv dismissed stale reviews from nvidianz and IsaacYangSLA via fbd014c October 5, 2023 14:40
YuanTingHsieh
YuanTingHsieh approved these changes Oct 5, 2023
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.

Thanks for the explanation, LGTM!

@yanchengnv yanchengnv merged commit 571f17f into NVIDIA:main Oct 6, 2023
15 checks passed
holgerroth pushed a commit to holgerroth/NVFlare that referenced this pull request Dec 4, 2023
@yanchengnv
Restore non-aio GRPC and a few improvements (NVIDIA#2058)
cc7ab10 
* restore non-aio grpc driver

* restore non-aio grpc driver

* fix unit tests

* fix drivers

* fix CP HB bug; add retry for result submit

* fix test cases

* fix test case

* address pr review

* fix download_job bug in flare_api
@yanchengnv yanchengnv deleted the non_aio_grpc branch December 27, 2023 15:54
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

@nvidianz nvidianz nvidianz left review comments

@IsaacYangSLA IsaacYangSLA IsaacYangSLA left review comments

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

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