-
Notifications
You must be signed in to change notification settings - Fork 170
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
Support http communication implement for DLRover Master and Agent. #1429
Support http communication implement for DLRover Master and Agent. #1429
Conversation
# Conflicts: # dlrover/python/master/servicer.py # dlrover/python/util/common_util.py
# Conflicts: # dlrover/python/elastic_agent/torch/training.py
# Conflicts: # dlrover/python/common/constants.py # dlrover/python/tests/test_utils.py # scripts/ci_install.sh
# Conflicts: # dlrover/python/master/args.py # dlrover/python/tests/test_args.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1429 +/- ##
==========================================
+ Coverage 81.40% 81.53% +0.12%
==========================================
Files 238 240 +2
Lines 23238 23592 +354
==========================================
+ Hits 18917 19235 +318
- Misses 4321 4357 +36 ☔ View full report in Codecov by Sentry. |
It is very meaningful job. Can we remove the grpc in the following verison? |
Maybe. We need more time and practice to verify the performance, reliability, flexibility, etc., of the HTTP approach. |
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.
so far the code LGTM. but we need some follow up
- perf report comparing grpc with http
- security extensions in the future, e.g. basic authentication, crypt/decrypt etc. for now the message with auth is needed to prevent bot sending deprecated messages
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
Note: The current implementation only indicates that the 'HTTP mode' is basically functional, but it does not imply that it is suitable for large-scale use in a production environment. We will continue to iterate and enhance the relevant parts until the functionality is stable and the performance meets expectations. |
1c4109d
into
intelligent-machine-learning:master
What changes were proposed in this pull request?
Why are the changes needed?
#1366
Does this PR introduce any user-facing change?
User can specify which service to use for communication between the master and agent by using the parameter
--service_type=(http/grpc)
. For details, please refer: dlrover/docs/deployment/argument.md.How was this patch tested?
UT + training with grpc type service + training with http type service.