Skip to content

test: port test_value_temperature from slime #1928 (mirror-slime gap) - #158

Merged
CalvinXKY merged 1 commit into
mainfrom
sync/test-value-temperature
Jun 7, 2026
Merged

test: port test_value_temperature from slime #1928 (mirror-slime gap)#158
CalvinXKY merged 1 commit into
mainfrom
sync/test-value-temperature

Conversation

@aoshen02

@aoshen02 aoshen02 commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

slime #1928 landed both the critic-temperature fix (loss.py) and tests/test_value_temperature.py. vime #95 ported only the fix — the test was missed. This restores it (mirror-slime).

  • tests/test_value_temperature.py: slime verbatim, only slime.vime. (import + sys.modules paths). NUM_GPUS=0, 0 sglang refs. Passes locally (1 passed).
  • Wired into the num_gpus:0 unit matrix in pr-test.yml + pr-test.yml.j2. vime CI runs explicit per-file matrix entries (not bare pytest), so an unlisted test would never run.

AI-assisted. Signoff item 3.

@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 adds a new test file tests/test_value_temperature.py to verify that get_values does not apply rollout temperature. The review feedback suggests a more idiomatic and less error-prone approach to mock and restore sys.modules using pytest's monkeypatch.delitem instead of manual try...finally blocks.

Comment on lines +13 to +50
previous_loss = sys.modules.pop("vime.backends.megatron_utils.loss", None)
previous_cp_utils = sys.modules.pop("vime.backends.megatron_utils.cp_utils", None)

mpu_stub = types.SimpleNamespace(
get_context_parallel_world_size=lambda: 1,
get_context_parallel_rank=lambda: 0,
)
megatron_mod = types.ModuleType("megatron")
core_mod = types.ModuleType("megatron.core")
core_mod.mpu = mpu_stub
monkeypatch.setitem(sys.modules, "megatron", megatron_mod)
monkeypatch.setitem(sys.modules, "megatron.core", core_mod)

try:
from vime.backends.megatron_utils.loss import get_values

args = Namespace(qkv_format="thd", rollout_temperature=0.5, allgather_cp=False)
logits = torch.tensor([[[1.0], [2.0], [3.0], [4.0]]], dtype=torch.float32)
tokens = [torch.tensor([10, 11, 12, 13], dtype=torch.long)]

_, result = get_values(
logits,
args=args,
unconcat_tokens=tokens,
total_lengths=[4],
response_lengths=[2],
)

torch.testing.assert_close(result["values"][0], torch.tensor([2.0, 3.0]))
finally:
if previous_loss is None:
sys.modules.pop("vime.backends.megatron_utils.loss", None)
else:
sys.modules["vime.backends.megatron_utils.loss"] = previous_loss
if previous_cp_utils is None:
sys.modules.pop("vime.backends.megatron_utils.cp_utils", None)
else:
sys.modules["vime.backends.megatron_utils.cp_utils"] = previous_cp_utils

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.

medium

Instead of manually popping modules from sys.modules and restoring them in a try...finally block, you can use pytest's monkeypatch.delitem with raising=False. This is more idiomatic, less error-prone, and automatically handles the cleanup/restoration of sys.modules after the test completes.

    monkeypatch.delitem(sys.modules, "vime.backends.megatron_utils.loss", raising=False)
    monkeypatch.delitem(sys.modules, "vime.backends.megatron_utils.cp_utils", raising=False)

    mpu_stub = types.SimpleNamespace(
        get_context_parallel_world_size=lambda: 1,
        get_context_parallel_rank=lambda: 0,
    )
    megatron_mod = types.ModuleType("megatron")
    core_mod = types.ModuleType("megatron.core")
    core_mod.mpu = mpu_stub
    monkeypatch.setitem(sys.modules, "megatron", megatron_mod)
    monkeypatch.setitem(sys.modules, "megatron.core", core_mod)

    from vime.backends.megatron_utils.loss import get_values

    args = Namespace(qkv_format="thd", rollout_temperature=0.5, allgather_cp=False)
    logits = torch.tensor([[[1.0], [2.0], [3.0], [4.0]]], dtype=torch.float32)
    tokens = [torch.tensor([10, 11, 12, 13], dtype=torch.long)]

    _, result = get_values(
        logits,
        args=args,
        unconcat_tokens=tokens,
        total_lengths=[4],
        response_lengths=[2],
    )

    torch.testing.assert_close(result["values"][0], torch.tensor([2.0, 3.0]))

@aoshen02
aoshen02 marked this pull request as ready for review June 7, 2026 13:06
@aoshen02
aoshen02 force-pushed the sync/test-value-temperature branch 2 times, most recently from 32c80db to b695e6b Compare June 7, 2026 14:09

@CalvinXKY CalvinXKY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

slime #1928 landed both the fix (loss.py: don't apply rollout temperature to
critic values) AND tests/test_value_temperature.py. vime #95 ported only the
fix; the test was missed. Restore it to match slime.

- tests/test_value_temperature.py: slime verbatim, slime.→vime. only (import +
  sys.modules paths). NUM_GPUS=0, 0 sglang refs. Passes locally (1 passed).
- wire into the num_gpus:0 unit matrix in pr-test.yml + pr-test.yml.j2 (vime CI
  runs explicit per-file matrix entries, not bare pytest — an unlisted test
  would be dead).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@aoshen02
aoshen02 force-pushed the sync/test-value-temperature branch from b695e6b to 911498e Compare June 7, 2026 14:19
@CalvinXKY
CalvinXKY merged commit e46ca58 into main Jun 7, 2026
11 of 14 checks passed
@aoshen02
aoshen02 deleted the sync/test-value-temperature branch June 8, 2026 14:17
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