[misc] chore: add optional vLLM cache config for fully-async NPU script#6932
[misc] chore: add optional vLLM cache config for fully-async NPU script#6932lxb007981 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates configuration parameters for Qwen3-VL-8B training, including increasing the maximum response length to 8192, adjusting the PPO maximum token length per GPU to 10240, and introducing a VLLM_CACHE_CONF configuration option. Feedback on the changes highlights critical bash syntax issues in geo3k_qwen3vl_8b_fsdp2_16_16_npu.sh: a trailing backslash-quote in the commented-out VLLM_CACHE_CONF definition that would cause an unexpected EOF error if uncommented, and a missing line continuation backslash after ${VLLM_CACHE_CONF} within the ACTOR_CONFIG multi-line string.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Leave empty by default. Uncomment this if the vLLM version is < v0.22.0. | ||
| # See: https://github.com/vllm-project/vllm/pull/43001/ | ||
| VLLM_CACHE_CONF="" | ||
| # VLLM_CACHE_CONF="+actor_rollout_ref.rollout.engine_kwargs.vllm.mm_processor_cache_gb=0 \" |
There was a problem hiding this comment.
The trailing backslash-quote (") inside the double-quoted string escapes the closing double quote. If this line is uncommented, the string will remain open, leading to a bash syntax error (unexpected EOF while looking for matching '"'). To avoid this and keep the configuration clean, we should define VLLM_CACHE_CONF without any trailing backslash, and instead handle the line continuation backslash directly within the ACTOR_CONFIG definition. Additionally, ensure we use + instead of ++ as the prefix for overriding configuration values in Hydra.
| # VLLM_CACHE_CONF="+actor_rollout_ref.rollout.engine_kwargs.vllm.mm_processor_cache_gb=0 \" | |
| # VLLM_CACHE_CONF="+actor_rollout_ref.rollout.engine_kwargs.vllm.mm_processor_cache_gb=0" |
References
- Use
+instead of++as the prefix for overriding configuration values in Hydra.
| actor_rollout_ref.actor.fsdp_config.entropy_checkpointing=True \ | ||
| actor_rollout_ref.actor.fsdp_config.param_offload=False \ | ||
| actor_rollout_ref.actor.fsdp_config.optimizer_offload=False \ | ||
| ${VLLM_CACHE_CONF} |
There was a problem hiding this comment.
Add a trailing backslash to ensure proper line continuation within the ACTOR_CONFIG multi-line string, regardless of whether VLLM_CACHE_CONF is empty or populated. This keeps the variable definition clean and prevents syntax/formatting issues.
| ${VLLM_CACHE_CONF} | |
| ${VLLM_CACHE_CONF} \ |
102cfe9 to
e90a87f
Compare
|
The script examples/grpo_trainer/run_qwen3_vl_8b_fsdp.sh is generic. Are there any precision and performance experiments comparing GPU and NPU, before and after the changes? |
|
What is the motivation behind these changes? As an example, increasing the number of nodes from 8 to 16 would raise the cost of the experiment. |
Understood. Since we haven't done extensive performance experiments yet, I will keep this PR restricted strictly to the bugfix. |
e90a87f to
e0a2c6f
Compare
Hi, I've pushed the updated changes. Could you help take a look? Thanks! |
|
The failed ci is fixed by #6937 |
e0a2c6f to
bde5480
Compare
Now the change only affects NPU. Can you confirm this issue only exists on NPU? |
…22.0 Disable vLLM's multimodal processor cache by default for vLLM older than 0.22.0 to avoid cache desync across pause/resume. Keep explicit user overrides intact.
bde5480 to
fc17868
Compare
Thanks for the feedback. The upstream vLLM issue vllm-project/vllm#42995 was reproduced on GPU. I moved the parameter override to the python configuration side so that both GPU/NPU could benefit. Would you like to take a look? |
What does this PR do?
Disable vLLM's multimodal processor cache by default for vLLM older than 0.22.0 to avoid cache desync across pause/resume. Keep explicit user overrides intact. Otherwise an
AssertionError: Expected a cached item for mm_hash='...'will be raised during verlupdate_weights()calling vllmpause_generation(clear_cache=True). See vllm-project/vllm#43001 for details.Checklist Before Starting
Search for similar PRs. Paste at least one query link here: ...
Format the PR title as
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,veomni,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,fully_async,one_step_off,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.