Add VLM Geo3k multiturn experiment - #120
Conversation
Signed-off-by: Dakai An <dakaian108@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the Geo3K multi-turn environment and rollout generation logic to support multi-turn interactions, boxed/text answer extraction when tool calls are missing, and prefix-stable rendering. It also updates the execution script configuration, including hardcoding model parameters and adjusting vLLM arguments. The code review feedback suggests making _image_to_render_url more robust by supporting pathlib.Path objects directly and adding a defensive check to ensure kwargs_data['image'] is a list before iterating over it.
| if isinstance(image, str): | ||
| if image.startswith("data:"): | ||
| # Some prepared Geo3K rows use data:image/None; vLLM expects a concrete media type. | ||
| return image.replace("data:image/None;", "data:image/png;", 1) | ||
| if image.startswith(("http://", "https://")): | ||
| return image | ||
| image_path = Path(image).expanduser() | ||
| if image_path.exists(): | ||
| with Image.open(image_path) as loaded_image: | ||
| return encode_image_for_rollout_engine(loaded_image) | ||
| raise ValueError(f"Unsupported image string for vLLM render: {image!r}") |
There was a problem hiding this comment.
Support pathlib.Path objects directly in _image_to_render_url. Currently, if image is a Path object, isinstance(image, str) evaluates to False, and it falls back to encode_image_for_rollout_engine(image) which expects a PIL Image and will fail. Checking for (str, Path) makes the utility more robust.
| if isinstance(image, str): | |
| if image.startswith("data:"): | |
| # Some prepared Geo3K rows use data:image/None; vLLM expects a concrete media type. | |
| return image.replace("data:image/None;", "data:image/png;", 1) | |
| if image.startswith(("http://", "https://")): | |
| return image | |
| image_path = Path(image).expanduser() | |
| if image_path.exists(): | |
| with Image.open(image_path) as loaded_image: | |
| return encode_image_for_rollout_engine(loaded_image) | |
| raise ValueError(f"Unsupported image string for vLLM render: {image!r}") | |
| if isinstance(image, (str, Path)): | |
| image_str = str(image) | |
| if image_str.startswith("data:"): | |
| # Some prepared Geo3K rows use data:image/None; vLLM expects a concrete media type. | |
| return image_str.replace("data:image/None;", "data:image/png;", 1) | |
| if image_str.startswith(("http://", "https://")): | |
| return image_str | |
| image_path = Path(image_str).expanduser() | |
| if image_path.exists(): | |
| with Image.open(image_path) as loaded_image: | |
| return encode_image_for_rollout_engine(loaded_image) | |
| raise ValueError(f"Unsupported image path or string for vLLM render: {image!r}") |
| if "image" not in kwargs_data: | ||
| return None |
There was a problem hiding this comment.
Add a defensive check to ensure kwargs_data["image"] is a list before iterating over it. If the rendered features contain a malformed or non-list value for "image", it could cause a runtime TypeError during rollout.
| if "image" not in kwargs_data: | |
| return None | |
| if "image" not in kwargs_data or not isinstance(kwargs_data["image"], list): | |
| return None |
Prefix Cache Hit Rate ComparisonI compared the vLLM engine-side
Both runs completed successfully:
|
| import vime.utils.misc as U | ||
| from vime.utils.external_utils.command_utils import execute_train | ||
|
|
||
| MODEL_NAME = os.environ.get("SLIME_SCRIPT_MODEL_NAME", "Qwen3-VL-2B-Instruct") |
There was a problem hiding this comment.
Remove "SLIME_SCRIPT_MODEL_NAME " from Readme. Consider refresh it too.
There was a problem hiding this comment.
Will fix in this PR: https://github.com/vllm-project/vime/pull/121/changes#diff-c538ffca60ebb211024d65a44fbca17242c074e6e02504542ec09fa20cade232. pr 105 only rename the path.
| f"{sample.metadata['multiturn_render']}" | ||
| ) | ||
|
|
||
| sample.multimodal_train_inputs = _multimodal_train_inputs_from_features(latest_features) |
There was a problem hiding this comment.
sample.tokens is stitched across turns, but multimodal features come from a single latest render. To assert that the count of image_token_id in sample.tokens matches image_grid_thw.shape[0] (or equivalent) before assigning multimodal_train_inputs, and fail loudly on mismatch rather than silently returning None. Is that better?
There was a problem hiding this comment.
I tested and generally these two are the same. But there may be some mismatches in other scenarios, so I have added a validation here.
aoshen02
left a comment
There was a problem hiding this comment.
Code-review notes (Codex strict pass + manual). Inline comments below; 2 are correctness (multi-turn token alignment), the rest are simplifications. Pre-existing items not in this PR's diff (dead _extract_balanced_json, optional orjson, bare env.close() except, commented-out eval_args) are noted separately, not inlined since they predate this PR.
| "Routing replay is not supported when appending an artificial EOS after a stop string, " | ||
| "because vLLM does not return routed experts for that extra token." | ||
| ) | ||
| train_tokens.append(int(eos_token_id)) |
There was a problem hiding this comment.
#1 (correctness, multi-turn off-by-one) — part 1/2. This synthetic EOS is appended to train_tokens only; it is not added to messages (L298 stores just response_text). So the next render's assistant segment has 1 fewer token than len(train_tokens). See the offset note on L318.
| sample.multimodal_train_inputs = _merge_multimodal_train_inputs(multimodal_train_inputs_buffer) | ||
| next_user_message = env.format_observation(observation) | ||
| messages.append(next_user_message) | ||
| pending_obs_offset = len(input_ids) + len(train_tokens) |
There was a problem hiding this comment.
#1 (correctness) — part 2/2. len(train_tokens) includes the synthetic EOS appended at L290, but the re-rendered conversation (messages has only response_text, no EOS) has 1 fewer token before the observation. So next turn obs_tokens = input_ids[pending_obs_offset:] (L244) drops the first real observation token, desyncing sample.tokens from the context the model actually generated against.
Fix: compute the render-prefix length from len(new_tokens) (exclude the synthetic EOS), keep train_tokens separate:
render_prefix_len = len(input_ids) + len(new_tokens)
pending_obs_offset = render_prefix_lenOnly triggers when append_stop_eos fires.
There was a problem hiding this comment.
These two parts are fixed by separating render-vs-training lengths:
- Keep appending synthetic EOS to train_tokens as before.
- Compute the render prefix with original generation tokens only: render_prefix_len = len(input_ids) + len(new_tokens) and use that for pending_obs_offset.
| messages.append(next_user_message) | ||
| pending_obs_offset = len(input_ids) + len(train_tokens) | ||
| rendered_body = await render() | ||
| latest_features = rendered_body.get("features") |
There was a problem hiding this comment.
Cleanup (redundant). This latest_features = rendered_body.get("features") is dead: the next loop iteration re-assigns it at the top (L241) from the same rendered_body, and the only consumer (L335) runs after a break that already saw L241's value. Safe to delete.
There was a problem hiding this comment.
True, this is redundant. Deleted
| "sample_len": len(sample.tokens), | ||
| "rendered_len": len(rendered_ids), | ||
| } | ||
| if getattr(args, "strict_multiturn_render_token_match", False) and not is_prefix_stable: |
There was a problem hiding this comment.
#2 (correctness) — the alignment guard is off by default. The prefix-stability check at L322 is exactly what catches the #1 desync (and any render-vs-accumulation mismatch), but it only raises when strict_multiturn_render_token_match is True — which defaults to False. By default a mismatch is silently recorded in metadata and training proceeds on misaligned tokens.
Suggest defaulting strict to True (raise on mismatch), or making rendered_ids canonical and rebuilding sample.tokens/loss_mask/rollout_log_probs from the rendered conversation.
There was a problem hiding this comment.
Yes this is false by default. Now I change it to fail fast whenever mismatch happens, without relying on "strict_multiturn_render_token_match" now.
| raise ValueError("Environment module must expose a callable `build_env(sample, args)`.") | ||
| try: | ||
| env = build_env(sample=sample, args=args) | ||
| except TypeError: |
There was a problem hiding this comment.
Cleanup. This except TypeError retry can swallow a real TypeError raised inside build_env (a constructor bug), making debugging hard. build_env here accepts keywords — just call env = build_env(sample=sample, args=args).
|
|
||
| score = self._score_answer(parsed_answer) | ||
| self.last_tool_score = score | ||
| self.correct = score == 1.0 |
There was a problem hiding this comment.
Cleanup (redundant state). self.correct is only read immediately below at L249; it doesn't need to live on the env. A local correct = score == 1.0 plus return obs, correct or is_final_turn, info removes the field and its init/reset at L42/L49.
There was a problem hiding this comment.
Fix. This is truly redundant.
|
|
||
| MODEL_NAME = os.environ.get("SLIME_SCRIPT_MODEL_NAME", "Qwen3-VL-2B-Instruct") | ||
| MODEL_NAME = "Qwen3-VL-2B-Instruct" | ||
| assert MODEL_NAME in { |
There was a problem hiding this comment.
Cleanup (dead assert). MODEL_NAME is a hardcoded constant immediately asserted to be in a set — the assert is always true. Either restore an env override (MODEL_NAME = os.environ.get(...)) or drop the assert and keep just the constant.
There was a problem hiding this comment.
Fixed. Use the "VIME_SCIRPT_MODEL_NAME" now.
Signed-off-by: Dakai An <dakaian108@gmail.com>
Signed-off-by: Dakai An <dakaian108@gmail.com>
Signed-off-by: Dakai An <dakaian108@gmail.com>
Signed-off-by: Dakai An <dakaian108@gmail.com>
Signed-off-by: Dakai An <dakaian108@gmail.com>

Summary
This PR fixes the Geo3K VLM multi-turn rollout path so that Geo3k multiturn with vLLM reward score can increase.
What Changed
Geo3K VLM multi-turn rollout
examples/geo3k_vlm_multi_turn/rollout.pyto use the full-conversation vLLM render route as the source of truth for prompt tokens and multimodal features.data:image/NoneURLs for render requests.Geo3K environment behavior
examples/geo3k_vlm_multi_turn/env_geo3k.pynow ends an episode as soon ascalc_scorereturns a correct answer.max_turns.Run script defaults
examples/geo3k_vlm_multi_turn/run_geo3k_vlm_multi_turn.pynow uses the validated Qwen3-VL-2B colocated vLLM/Megatron setup:Qwen3-VL-2B-Instruct--vllm-router-policy round_robin--vllm-max-model-len 32768--vllm-gpu-memory-utilization 0.9--vllm-generation-config vllm--vllm-enforce-eager--vllm-logprobs-mode processed_logprobs1e-6, global batch size512Validation
W&B run:
Why vLLM eager mode
Without
--vllm-enforce-eager, Qwen3-VL multimodal rollout showed a large silent parity drift between vLLM rollout logprobs and Megatron replay logprobs. The VLM two-turn diagnostic reached about0.1399train/rollout logprob absolute diff.With eager mode, the same path dropped to text-only-level parity, around
0.020in smoke diagnostics and around0.014-0.016in the long training run. This matches SkyRL's stability-oriented default of using eager vLLM inference for its training engine.cc @CalvinXKY