client
client copied to clipboard
triton-inference-server
Soften grpcio requirement in python library
Show warning when the imported version has memory leakage issue, but do allow users to proceed on their own risk
As there are many packages depending on grpc, it is becoming exceedingly difficult to manage a pinned version. Also, depending on use case, memory leakage problem might be worked around if we regularly restart or otherwise manage servers and working nodes, which are often the case.
I think this change is fine, @Tabrizian we probably need to update the CI dockerfile to install 1.41 explicitly?
Thanks! wrt CLA, I'm just checking with the company and usually should be able to get it signed soon. Will keep y'all posted!
Hi, I also agree with @chajath's points. We are also having lots of problems maintaining our other external libraries that now require more recent versions of grpc. I wasn't able to run the memory leak test but is there a chance that we can test it with the latest version of grpc if the issue is still relevant?
And @chajath, if it's difficult from your side to sign the CLA maybe I can help you with it?
Sorry I was caught up with other priorities. Just sent out the signed CLA. PTAL thanks!
@aeroith Nope, I get an "Access denied" email when trying to email a signed PDF to [email protected]. Is there any other address I can send CLA to?
@aeroith Nope, I get an "Access denied" email when trying to email a signed PDF to
[email protected]. Is there any other address I can send CLA to?
hey @chajath , I think it should be [email protected] instead of [email protected].
After a second thought, I'm a bit hesitant to move forward with this change. This will result in the default installation of tritonclient from pip to contain a memory leak since it will automatically install the latest version (assuming that the latest grpcio version still contains the memory leak @GuanLuo reported).
Ok thanks. I've just sent CLA.
I understand the hesitation, but a) it doesn't look like Google is gonna address the issue anytime soon and b) since grpc is used widely, it creates a massive pain to manage the dependency going forward. In my case, a lot of workloads were burst workloads like batch inference, so worst case I'll have to tear down and restart the worker node, and that hasn't happened while we run inference at a scale.
So on the balance of maintainability vs correctness, I don't think we are compromising too much here. Just my 2cents.
So on the balance of maintainability vs correctness, I don't think we are compromising too much here. Just my 2cents.
Hi @chajath since the newer versions still have this memory issue I don't think we will go forward with this plan with changing our requirements.txt since we have many other customers who maintain their pods for a long time. Would it be possible for you clone our client repo and work off of that?
Hi everybody! Just to add another bit on this issue ... I've tried to use tritonclient on my mac M1 and I've encountered this error
File "/.../lmv1_triton_inference_client.py", line 7, in <module> import tritonclient.grpc as tritongrpcclient # type: ignore File "/.../venv/lib/python3.10/site-packages/tritonclient/grpc/__init__.py", line 29, in <module> import grpc File "/.../venv/lib/python3.10/site-packages/grpc/__init__.py", line 22, in <module> from grpc import _compression File "/.../venv/lib/python3.10/site-packages/grpc/_compression.py", line 15, in <module> from grpc._cython import cygrpc ImportError: dlopen(/.../venv/lib/python3.10/site-packages/grpc/_cython/cygrpc.cpython-310-darwin.so, 0x0002): tried: '/.../venv/lib/python3.10/site-packages/grpc/_cython/cygrpc.cpython-310-darwin.so' (mach-o file, but is an incompatible architecture (have (x86_64), need (arm64e)))
With some investigation I discovered that this issue has been resolved in grpcio v1.47 as described in this issue grpc
My issue seems to be also solvable using grpcio from conda-forge but maybe you'd like to be informed about it. Thanks for your work!
Yet another issue: grpc < 1.42 doesn't support python >= 3.10 https://github.com/grpc/grpc/issues/27888
In https://github.com/grpc/grpc/issues/28513 @GuanLuo says no memory leak was detected in versions before 1.43.0. So pinning the version to 1.42.0 may resolve this one issue.
+1 to this, both for M1 support and python 3.10 support.
This will result in the default installation of tritonclient from pip to contain a memory leak since it will automatically install the latest version
Not sure I understand excluding users who need grpcio>1.41, who are blocked from using Triton entirely by this limitation, to service users who could just hard-pin their version of grpcio themselves?
@Tabrizian @jbkyang-nvi can y'all re-review the state of this PR?
If it's not going to be submitted, please close this PR and perhaps create other issues to address the remaining issues discussed in this PR.
@chajath @aeroith @ozen @alecgunny
To help us understand the final goal you would like. I have a few questions:
- Is the issue related to only
requirements.txt? Or is the issue that you cannot runpip3 install tritonclient[all]and have any package >= 1.41.0? - Would changing the requirement to be
grpcio>=1.41.0,<=1.42.0
be an improvement?
Specifically regarding this comment:
Not sure I understand excluding users who need grpcio>1.41, who are blocked from using Triton entirely by this limitation, to service users who could just hard-pin their version of grpcio themselves?
We want the default installation of using pip3 install tritonclient[all] to have a non leaked version. Are you maybe suggesting that it is possible for the user to do pip3 install --upgrade grpcio==1.41.0?
It's not just requirements.txt. All modern python toolchains poetry etc respects version pinning, and will basically give up if they can't find the suitable version of dependent packages (on M1 or py310, for example)
Changing to grpcio>=1.41.0,<=1.42.0 is no improvement
Concerned users will be able to see the import warning, and should they follow up with the warning, they will subsequently be able to pin grpcio version if feasible.
Echoing what @chajath has brought up. Modern Python tools like Poetry help make environments reproducible outside of full containerization, which might not be feasible or practical in many situations. However, any tool like this that tries to solve an environment's or even an individual library's dependencies will hit a brick wall if the downstream user specifying the environment needs grpcio > 1.41.
The reality is that tritonclient will never exist in a vacuum: it will need to coexist in an environment where its dependencies have to be balanced against the dependencies of other libraries and the needs of the user. For some users, this will mean specifying grpcio==1.41 in their environment. But expecting a naive pip3 install tritonclient[all] to magically install all the correct dependencies each user needs is disconnected from most practical or scalable development models.
Thanks for your feedback! We hear your frustration. However, since we do have some customers who does use
pip3 install tritonclient[all] and they have long running clients that do care about memory, we cannot accept
this change so long as grpc memory leak issue exists.
The client can be be built from source, let us know if there's any other issues you are facing from that end. If you want to accelerate this issue, I would suggest voting for the above issue in the grpc repo
No one is suggesting you don't have users who do that. What we're suggesting is that asking them to simply specify grcp<=1.41 is infinitely more sensible than excluding users who don't want that constraint from pip installation entirely.
Now that I've spent some time looking at the motivating gRPC issue, all of this is frankly even more confusing. If the issue is repeated client creation/request/deletion, why don't you just encourage these users to reuse the same client (and even the same input and output buffers)? I'm sure there are some use cases where this isn't possible, but this seems like much more sensible behavior that would cover a large majority of use cases.
It just seems like in the pursuit of making naive out-of-the-box implementations work, this decision is sacrificing the usability of a lot of much more practical use cases.
I am looking to use grpc streaming over TLS, and have found that there was an issue with the TLS when using Let's Encrypt for versions prior to v1.41.1
So at a minimum, I would request that this library be updated to grpcio==1.41 .1 to allow this to work.
@jbkyang-nvi I saw on https://github.com/grpc/grpc/issues/28513 last comment, the gRPC team suggests memory leak is not from grpc. In this case, is it possible to soften the grpc requirement (allow installation of latest versions)? I have to use tritonclient with other python packages that requires newer grpcio, so currently I have to manually patch them everytime.
