adding multiple grpc connections for python server and executor client #3356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @mwm5945. Thanks for your PR. I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository. |
mwm5945
commented
Jun 30, 2021
executor/api/grpc/seldon/client.go
Outdated
Comment on lines
62
to
99
| s.RLock() | ||
| randNum := rand.Intn(numConns) | ||
| k := fmt.Sprintf("%s:%d", host, port) | ||
| if conn, ok := s.conns[k]; ok { | ||
| return conn, nil | ||
| if nodeConns, ok := s.conns[k]; ok { | ||
| if c := nodeConns[randNum]; c != nil { | ||
| defer s.RUnlock() | ||
| return c, nil | ||
| } | ||
| s.RUnlock() | ||
|
|
||
| c, err := s.createNewConn(modelName, host, port) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s.Lock() | ||
| s.conns[k][randNum] = c | ||
| s.Unlock() | ||
|
|
||
| return s.conns[k][randNum], nil | ||
| } else { | ||
| opts := []grpc.DialOption{ | ||
| grpc.WithInsecure(), | ||
| } | ||
| opts = append(opts, grpc2.AddClientInterceptors(s.Predictor, s.DeploymentName, modelName, s.annotations, s.Log)) | ||
| conn, err := grpc.Dial(fmt.Sprintf("%s:%d", host, port), opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| s.conns[k] = conn | ||
| return conn, nil | ||
| s.RUnlock() | ||
| connList := make([]*grpc.ClientConn, numConns) | ||
|
|
||
| c, err := s.createNewConn(modelName, host, port) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| connList[randNum] = c | ||
|
|
||
| s.Lock() | ||
| s.conns[k] = connList | ||
| s.Unlock() | ||
|
|
||
| return s.conns[k][randNum], nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I've done this complicated stuff because it appears to better spread out the connections amongst the available processes on the server side. There's still no guarantee that each client will connect to a unique process though.
mwm5945
commented
Jun 30, 2021
mwm5945
commented
Jun 30, 2021
mwm5945
commented
Jun 30, 2021
|
i think it'd also be a good idea to add a new arg to the python server to match the new |
|
/ok-to-test |
|
@mwm5945 Can you run "make fmt" from python folder to solve lint issue? |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Looking great @mwm5945 ! It seems the Python tests are failing for GRPC (on the CI) - conscious that this woudl run the grpc service locally it may require some tweaks to work with the new implementation |
|
(The docs test failing is due to an issue which is now fixed in master so rebasing should address it) |
|
@axsaucedo do you know what version of python the Appears this might be an issue with Conda or even an issue with a specific version of Python 3 |
|
(also the classic "works on my machine" lol) |
ukclivecox
suggested changes
Jul 2, 2021
|
@mwm5945 Can you fix lint with |
mwm5945
force-pushed
the
multi-conn
branch
2 times, most recently
from
756bf39 to
d625585
Compare
|
@cliveseldon running make fmt in python directory doesn't change anything. looks like something else: |
|
/test notebooks |
Benchmark results - Testing Workers PerformanceResults table
|
Benchmark Results - Python Wrapper V1 vs V2
Python V1 Wrapper Results table
Python V2 MLServer Results table
|
Benchmark results - Testing Seldon V1 Data TypesResults for NDArray
Results for Tensor
Results for TFTensor
|
|
/test integration |
|
/test notebooks |
|
/test notebooks |
1 similar comment
|
/test notebooks |
axsaucedo
suggested changes
Jul 14, 2021
| @@ -60,7 +63,7 @@ def start_servers( | |||
| """ | |||
| p2 = None | |||
| if target2: | |||
| p2 = mp.Process(target=target2, daemon=True) | |||
|
It seems the custom metrics test is failing, it's not yet clear what may be causing this issue - trying to see whether it could be that the changes are somehow blocking the metrics port and the prometheus metrics are not being scraped, however I can't spot explicitly a reason why this could be the case. Not sure if you have any thouhts @mwm5945 ? I will try to run locally to validate |
|
/test notebooks |
|
/test integration |
|
All tests passing - merging |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it: Support multiple GRPC connections on the Python Server to allow for pre-fork multi-processing, and multiplex requests over multiple GRPC Client connections in the executor.
Which issue(s) this PR fixes:
Fixes #3334
Special notes for your reviewer:
Does this PR introduce a user-facing change?: