[fix] Render multimodal sample prompts under the base model (#1860)#1907
Open
sunnyadn wants to merge 1 commit into
Open
[fix] Render multimodal sample prompts under the base model (#1860)#1907sunnyadn wants to merge 1 commit into
sunnyadn wants to merge 1 commit into
Conversation
…AI#1860) vLLM's /v1/chat/completions/render does not resolve LoRA adapter names, so _render_for_sample 404s for any image-bearing prompt when LoRA is in use. Rendering is adapter-independent, so always target the base model; generation still targets the adapter on /inference/v1/generate.
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Author
|
The close/reopen pairs on the timeline were CI retriggers during the July 16 GitHub incident; no code changed between them. Current status: all test jobs pass, and the two remaining red checks (skyrl_gpu_tests, Vercel) fail because fork PRs cannot access ANYSCALE_CLI_TOKEN and the deploy secrets. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #1860.
_render_for_sampleposts the resolved model to/v1/chat/completions/render. Underuses_lora_weight_sync=Truethe resolved model is the LoRA adapter name, which the render endpoint does not resolve, so every image-bearing Tinker sample request 404s. Following @benrhodes26's diagnosis in #1860, the render request now always targets the base model (self.model_name): rendering (chat-template application, tokenization, multimodal preprocessing) is adapter-independent. Generation still targets the adapter on/inference/v1/generate.The
modelparameter of_render_for_samplebecomes unused and is removed.Tests
New
test_sample_with_image_renders_under_base_model: image chunks +uses_lora_weight_sync=True+ explicit adapter model, asserts the render sub-request targets the base model while generate targets the adapter. On main it fails withAssertionError: assert 'lora-adapter' == 'base-model'; with this fix the full file passes (57 passed,pytest tests/backends/skyrl_train/inference_servers/test_remote_inference_client.py -m "not vllm").Also verified live against vLLM 0.23.0 (the version pinned in
pyproject.toml) on an L40S, servingQwen/Qwen2.5-VL-3B-Instructwith a LoRA adapter via--enable-lora --lora-modules:POST /v1/chat/completions/renderwith the adapter name returns 404 (The model 'lora-test' does not exist), even though/v1/modelslists the adapter.POST /v1/chat/completionswith an image under the adapter name returns 200, confirming generation resolves adapters while render does not.On the parity point raised in #1860 (base-model render output matching what the adapter would produce): in vLLM 0.23.0 this holds by construction. The render module (
vllm/entrypoints/serve/render/) has no LoRA code path, and its model check goes throughOpenAIModelRegistry, documented as having "no engine client and no LoRA support". Adapters also cannot alter tokenization in this version (LoRAConfighas no extra-vocab field), so there is no adapter-dependent render output to diverge from.