Repository navigation
Patch readline-import issue which causes VLLMGenerator hang - #3950
Conversation
meta-cla
Bot
added
the
CLA Signed
| # vLLM -> tilelang -> tvm -> tvm's base.py -> import readline | ||
| # https://github.com/vllm-project/vllm/blob/b23bd73f540175f9e117eaee5029cd7d8df63964/vllm/utils/jit_monitor.py#L451 | ||
| # https://github.com/tile-ai/tvm/blob/28a0d34420d2fa9bc71fc891445a3f1396fca759/python/tvm/base.py#L71 |
There was a problem hiding this comment.
My read is monarch is not compatiable with vllm. It should be either fixed in monarch or vllm, not user side? And this patch is hacking and hard to understand from RL infra user level
There was a problem hiding this comment.
The culprit is readline. For some versions' readline, you cannot import it in a background process. This is an old problem. The oldest one I found was in 2012: python/cpython#59097
It also bit torch.distrubuted recently: pytorch/pytorch#159645. In that issue, the user reported that they could not run torch.distributed in a background process. This issue is very similar to what we saw here: vllm does import readline unintentionally, which blocks vllm from being used in a background process.
Monarch is being caught in the crossfire, because it uses background processes to run its actors. But using background process is not wrong. That is why we do not want to make a big change to Monarch just to cater this decade old issue.
Can we patch it in vllm, or maybe its dependencies, e.g. tvm? It is possible. I have not looked into that. But even if that is technically possible, it might take a while to resolve, so we still need some patch here to unblock.
There was a problem hiding this comment.
I understand it's easier to fix from user side, but I still think it should not be a user side fix. I guess a more clear fix path is landing this temporary fix, and raise an github issue in tvm (even a direct PR in tvm)
background process is not wrong
Choosing background process as the design for monarch sounds correct, but the error is originally caused by vllm upstream. Just want to fix at the right place
There was a problem hiding this comment.
I submitted a PR and opened an issue as well in case they prefer a different fix:
| 4. Install PyTorch nightly, pre-built vllm wheel (based on PyTorch nightly version), and torchcomms nightly. | ||
| 4. Install PyTorch and torchvision nightlies, pre-built vllm wheel (based on PyTorch nightly version), and torchcomms nightly. | ||
|
|
||
| `torchvision` is only needed because the current vllm nightly imports it during kernel warmup; TorchTitan RL does not otherwise require it. |
There was a problem hiding this comment.
why wouldn't vllm nightly wheel already depend on this and automatically install?
There was a problem hiding this comment.
vllm is installed from the wheel, and torchvision is not in the wheel's Requires-Dist. Probably that is because vllm excludes it here (disclaim: I have not checked e2e):
…3950) The reason is explained in the in-line comments. This issue seems to be caused by a [recent change in vllm](vllm-project/vllm#46718), which introduces tilelang import, and subsequently triggers the import chain. Note that this is not a monarch bug. Although one can argue if monarch spawns the ProcMesh in a new session, [i.e. change `setpgid` to `setsid`](https://github.com/meta-pytorch/monarch/blob/a4af2fe08e8fd7fceac736089631bacf0baf5c7b/hyperactor_mesh/src/proc_launcher/native.rs#L355), readline-import's behavior will be tolerant, but the Monarch team does not think simply tolerating such behavior justifies an critical infra change on the Monarch side. More importantly, the Monarch side is not sure whether it might require sharing session in the future, and it does not want to lose that option too casually. As a result, the Monarch prefers to workaround it on the user side, especially given the workaround is reasonably small. cc: @shayne-fletcher In addition, add `torchvision` to README's installation instruction since it is required by vllm recently.
Absorbs 4 upstream commits (checkpoint load/resume clarify pytorch#3732, override kwargs from CLI+config pytorch#3894, [spmd_types] VarlenAttention un-hardcode pytorch#3937, VLLMGenerator readline hang patch pytorch#3950). No conflicts; spmd_types regression verified -- GQA MoE TP+EP+typecheck (12.48->6.82) and DSA GLM-5 TP=2 (12.53->6.82) both clean.
3 participants
The reason is explained in the in-line comments. This issue seems to be caused by a recent change in vllm, which introduces tilelang import, and subsequently triggers the import chain.
Note that this is not a monarch bug. Although one can argue if monarch spawns the ProcMesh in a new session, i.e. change
setpgidtosetsid, readline-import's behavior will be tolerant, but the Monarch team does not think simply tolerating such behavior justifies an critical infra change on the Monarch side. More importantly, the Monarch side is not sure whether it might require sharing session in the future, and it does not want to lose that option too casually. As a result, the Monarch prefers to workaround it on the user side, especially given the workaround is reasonably small. cc: @shayne-fletcherIn addition, add
torchvisionto README's installation instruction since it is required by vllm recently.