Skip to content

refactor: mirror slime structure in update_weight + vllm_rollout - #197

Merged
princepride merged 4 commits into
mainfrom
mirror-slime-broad-refactor
Jun 9, 2026
Merged

refactor: mirror slime structure in update_weight + vllm_rollout#197
princepride merged 4 commits into
mainfrom
mirror-slime-broad-refactor

Conversation

@aoshen02

@aoshen02 aoshen02 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • update_weight_from_distributed.py: Add update_weight_metrics + pop_metrics(), _on_chunk() hook, restructure _send_weights() dual-pass to call _on_chunk before each broadcast, align _iter_non_expert_chunks buffer accounting to convert-first-then-measure (matches slime), remove _send_hf_chunk/_update_weights_vllm_packed wrappers, remove early-exit guard in _ep_gather_and_convert
  • vllm_rollout.py: Add docstrings and inline comments matching slime@44d29ee (generate_rollout, generate_rollout_async, eval_rollout_single_dataset, generate_and_rm_group, generate_and_rm)
  • vllm_utils/: No changes needed — vllm_config.py already identical, arguments.py and vllm_engine.py are genuine sglang→vLLM divergences

Reference: slime commit 44d29ee59dfa1489c369937779cbbdb1798ce701

Test plan

  • ruff check + ruff format pass
  • tests/unit/backends/megatron_utils/update_weight/test_update_weight_from_distributed.py — 22/22 pass
  • tests/unit/rollout/test_vllm_rollout.py — 26/26 pass
  • Integration test on h200 (colocate + non-colocate weight sync)

🤖 Generated with Claude Code

…vllm_rollout

Align with slime@44d29ee to reduce structural divergence:

update_weight_from_distributed.py:
- Add update_weight_metrics + pop_metrics() (actor-side metric drain)
- Add _on_chunk() hook (no-op base, override point for subclasses)
- Restructure _send_weights() to call _on_chunk before each broadcast
- Remove _send_hf_chunk / _update_weights_vllm_packed wrappers
- Align _iter_non_expert_chunks buffer accounting to convert-first-then-measure
- Remove early-exit guard in _ep_gather_and_convert
- Update class docstring to document subclass extension points

vllm_rollout.py:
- Add docstrings matching slime (generate_rollout, generate_rollout_async,
  eval_rollout_single_dataset, generate_and_rm_group)
- Add inline comments matching slime (generate_and_rm, generate_rollout_async)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the weight update and rollout generation logic, introducing subclass hooks, weight update metrics, and improved documentation/comments across the codebase. A critical issue was identified in update_weight_from_distributed.py where removing the safety check on pbar before calling pbar.update(1) can lead to an AttributeError when pbar is None.

ray.get(self.rollout_engine_lock.release.remote())
if pbar is not None:
pbar.update(1)
pbar.update(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Unconditionally calling pbar.update(1) will raise an AttributeError if pbar is None. Since pbar is typed as tqdm | None = None and can be None (for example, on non-PP-source ranks or when not provided), we should restore the if pbar is not None: guard.

Suggested change
pbar.update(1)
if pbar is not None:
pbar.update(1)

aoshen02 and others added 3 commits June 9, 2026 01:34
…_metrics)

These were introduced by slime PR #1806 (delta weight sync) which maps
to vime #150 — not planned for inclusion yet. Revert to pre-#1806 base
class interface while keeping the other structural alignments.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ocstring additions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@princepride
princepride merged commit c431f7f into main Jun 9, 2026
11 of 14 checks passed
momo609 pushed a commit that referenced this pull request Jun 10, 2026
* refactor: mirror slime structure in update_weight_from_distributed + vllm_rollout

Align with slime@44d29ee to reduce structural divergence:

update_weight_from_distributed.py:
- Add update_weight_metrics + pop_metrics() (actor-side metric drain)
- Add _on_chunk() hook (no-op base, override point for subclasses)
- Restructure _send_weights() to call _on_chunk before each broadcast
- Remove _send_hf_chunk / _update_weights_vllm_packed wrappers
- Align _iter_non_expert_chunks buffer accounting to convert-first-then-measure
- Remove early-exit guard in _ep_gather_and_convert
- Update class docstring to document subclass extension points

vllm_rollout.py:
- Add docstrings matching slime (generate_rollout, generate_rollout_async,
  eval_rollout_single_dataset, generate_and_rm_group)
- Add inline comments matching slime (generate_and_rm, generate_rollout_async)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: drop delta-sync additions (pop_metrics, _on_chunk, update_weight_metrics)

These were introduced by slime PR #1806 (delta weight sync) which maps
to vime #150 — not planned for inclusion yet. Revert to pre-#1806 base
class interface while keeping the other structural alignments.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: restore slime multi-line formatting (revert ruff line-collapse)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: revert ruff line-collapse in vllm_rollout, keep only comment/docstring additions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CalvinXKY
CalvinXKY deleted the mirror-slime-broad-refactor branch June 16, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants