Skip to content

docker : enable RPC for docker images #13474

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rgerganov
Copy link
Collaborator

No description provided.

@rgerganov rgerganov requested a review from ngxson as a code owner May 12, 2025 08:58
Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a small concern: some users might be worried about the security implications of including RPC in the docker image by default.

While I understand that having RPC in the image doesn’t mean it’s enabled at runtime, I’m concerned that not all users will realize this. In the past, I’ve had users and clients reach out to me with concerns when security reports involving RPC came up. At the time, I was able to reassure them that RPC wasn’t even part of the image.

Btw, I really appreciate the work you've done on the RPC backend - it’s impressive. That said, I do think we should be cautious about including it by default, which may not be what most users expect.

@github-actions github-actions bot added the devops improvements to build systems and github actions label May 12, 2025
@weikinhuang
Copy link

While that make sense. This still requires a extra cli arg to enable when running the server. I think the general case of making it easier for people to run in a distributed manner (like those in a homelab environment) without having to rebuild it makes a lot more sense.

@rgerganov
Copy link
Collaborator Author

I have a small concern: some users might be worried about the security implications of including RPC in the docker image by default.

While the server part (rpc-server) is still considered experimental and insecure, I think the client part (the RPC backend) is generally safe. The purpose of this PR is to include client-side support for RPC for docker users who want to experiment with this feature. But I understand the concerns, so I am OK to close this.

While that make sense. This still requires a extra cli arg to enable when running the server.

I am not sure I understand. Do you mean llama-server or rpc-server?

@weikinhuang
Copy link

I'm talking about the client side. Like you said there's no (rpc) server component with this change, so there's nothing extra exposed for any users of llama-server. The server part is still completely separate and there's no rpc listeners associated with llama-server.

@ngxson
Copy link
Collaborator

ngxson commented May 12, 2025

I know this change is technically safe. But I mean, in UX design, there is a concept called "Users are stupid"

Even when we make it super hard to (accidentally) enable RPC code path in prod, most users only care about: (1) RPC is included in the build and (2) there was (or will be) some security reports involving RPC.

@rgerganov rgerganov marked this pull request as draft May 12, 2025 14:31
@rgerganov
Copy link
Collaborator Author

Let's park this until we have at least some coverage guided fuzzing for the RPC or some other automated security tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops improvements to build systems and github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants