Skip to content

fix(vime): avoid double temperature scaling for reused logits - #423

Merged
inaniloquentee merged 2 commits into
mainfrom
fix/vime-avoid-double-temperature-scaling
Sep 17, 2026
Merged

inaniloquentee merged 2 commits into
mainfrom
fix/vime-avoid-double-temperature-scaling

Conversation

@inaniloquentee

@inaniloquentee inaniloquentee commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • avoid applying rollout temperature twice for legacy VIME requests that reuse pre-scaled local logits
  • preserve one temperature application for current VIME requests with unscaled local logits
  • keep hidden-state recomputation on the original request temperature
  • add numerical regression coverage for both real VIME contracts at temperature 0.7
  • change RL-Kernel only; no VIME source changes

Contract details

The PR #400 experiment pins VIME commit c80200e. That revision divides request.logits by rollout_temperature before provider dispatch and sets metadata["logits_are_temperature_scaled"] = True. Reapplying request.temperature in RL-Kernel therefore turns 0.7 into an effective 0.49.

VIME PRs #423 and #424 use the newer contract: provider-bound logits remain unscaled, the metadata marker is absent, and request.temperature must still be applied once. This change supports both contracts. The marker is consumed only by the reused-local-logits path; hidden-state recomputation continues to use the request temperature.

Validation

  • python -m pytest -q tests/test_vime_linear_logp_provider.py: 10 passed
  • Black, isort, Flake8, and git diff --check passed for the changed files
  • patched PR docs(rocm): publish G10/G11 200-step RL results #400 G11 strict run: first 4,165 tokens had mismatch count 0 and max absolute logp diff 0.0

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d8c96ba9-56e8-4a5a-b5f9-511ad9e4a6a4

📥 Commits

Reviewing files that changed from the base of the PR and between c5836d3 and 06b6e1f.

📒 Files selected for processing (2)
  • rl_engine/integrations/vime/linear_logp_provider.py
  • tests/test_vime_linear_logp_provider.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • rl_engine/integrations/vime/linear_logp_provider.py

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The strict Vime provider now resolves local-logit temperature from scaling metadata. Reused pre-scaled logits skip further scaling, while hidden-logit recomputation still uses the request temperature. Tests verify both paths.

Changes

Vime local-logit temperature handling

Layer / File(s) Summary
Resolve and apply local-logit temperature
rl_engine/integrations/vime/linear_logp_provider.py
The strict path returns None when local logits are marked as temperature-scaled. Otherwise, it uses the request temperature for entropy checks and from_local_logits.
Validate temperature handling
tests/test_vime_linear_logp_provider.py
Tests verify single temperature application for pre-scaled and unscaled logits. A separate test confirms that hidden-logit recomputation still receives the request temperature. Formatting-only edits do not change test behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 06b6e

The updated strict-provider paths and their regression coverage consistently preserve single temperature application for reused logits while retaining scaling for hidden-state recomputation. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: preventing double temperature scaling for reused logits in VIME.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: lamentropetion <3051000145@qq.com>
Signed-off-by: lamentropetion <3051000145@qq.com>

@Flink-ddd Flink-ddd 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

@maxiaosong1124
maxiaosong1124 self-requested a review September 17, 2026 16:26

@maxiaosong1124 maxiaosong1124 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!

@inaniloquentee
inaniloquentee force-pushed the fix/vime-avoid-double-temperature-scaling branch from 06b6e1f to 9d13d2b Compare September 17, 2026 16:27
@inaniloquentee
inaniloquentee merged commit 78a0ab3 into main Sep 17, 2026
5 of 8 checks passed
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.

3 participants