[k8s] Clarified the usage of shared memory. #4341
Conversation
jsuchome
requested review from
zhaochenyang20,
zhyncs,
HaiShaw,
merrymercy and
Ying1123
as code owners
jsuchome
changed the title
| @@ -21,6 +21,7 @@ python -m sglang.launch_server --model-path meta-llama/Meta-Llama-3-8B-Instruct | |||
| ``` | |||
|
|
|||
| - See [hyperparameter tuning](hyperparameter_tuning.md) on tuning hyperparameters for better performance. | |||
| - For docker and Kubernetes runs, you need to set up shared memory which is used for communication between processes. See `--shm-size` for docker and `/dev/shm` size update for Kubernetes manifests. | |||
There was a problem hiding this comment.
good summary. but it seem this doc server_arguments.md is majorly for CLI (python run) use case.
if we aim for "docker"/"k8s", maybe this file is not suitable?
https://github.com/sgl-project/sglang/blob/main/docs/start/install.md
There was a problem hiding this comment.
well, server_arguments.md is the only place where it summarize the parallelism runs, so I believe it should be mentioned thare (maybe among other places?)
There was a problem hiding this comment.
also, as you wrote, in Docker part of https://github.com/sgl-project/sglang/blob/main/docs/start/install.md shared memory is already mentioned. And in Kubernetes section there's just a link to manifest, so I'm suggesting:
- to modify single node manifest
- to update the docs in the place where
--tpis actually mentioned as server argument
|
cc @whybeyoung could you take a look? thanks! |
jsuchome
force-pushed
the
k8smanifest
branch
2 times, most recently
from
865d476 to
3852603
Compare
|
@whybeyoung is it approved by you? if so, pass the CI . I can merge it |
|
I will remind @whybeyoung again. |
|
/approve |
|
/lgtm |
jsuchome
force-pushed
the
k8smanifest
branch
2 times, most recently
from
da6e5ec to
546144e
Compare
Added extended shared memory for default kubernetes manigest.
|
I don't expect I am supposed to merge the PR, right? I keep rebasing it because it's offent out-of-sync, but than the approval status is lost... |
no need to rebase. We will merge this |
|
Hey, so what do you think about this? It's quite minor change, is there anything wrong with it? |
Added extended shared memory for default kubernetes manigest. Also, point various cache directories to writable /tmp so that pod does not need to be run as root.
Motivation
Added some more clarification for the usage of shared memory increase. It's necessary for single node installation too, if one wants to use some parallelism techniques. See #4259
Also, k8s pod does not need to run with root security context, but it needs some tinkering.Modifications
- additionally added env variables to enforce creating caches in writable volume, so that the pod can be run as non-rootChecklist