rename uid by hhaAndroid · Pull Request #1923 · InternLM/xtuner

hhaAndroid · 2026-06-17T11:34:12Z

No description provided.

hhaAndroid · 2026-06-17T11:34:28Z

claude · 2026-06-17T11:40:23Z

Claude: ## Summary

Clean rename of three RolloutState fields: uid → rollout_id, message_uid → group_id, session_uid → session_id. Also adds English docstrings to the renamed fields and always assigns group_id in the sampler (previously conditional on XTUNER_DETERMINISTIC).

Issues

Critical

sampler.py:107-110 — The old session_uid = new_data.uid assignment in the XTUNER_DETERMINISTIC branch was dropped instead of renamed. This leaves session_id as None, causing controller.py to fall back to uuid4().int, which breaks deterministic session routing. Fix: add new_data.session_id = new_data.rollout_id in the deterministic branch. (See inline comment with suggestion.)

Nit

The test file move tests/rl/fast/pr_fast/test_asyncio_diagnostics.py → tests/rl/test_asyncio_diagnostics.py is unrelated to the rename. One logical change per PR per project guidelines.

Verdict

REQUEST_CHANGES

hhaAndroid · 2026-06-17T12:09:04Z

@claude review

claude · 2026-06-17T12:14:14Z

+    def model_post_init(self, __context: Any) -> None:
+        if self.group_id is None:
+            self.group_id = self.message_uid
+        else:
+            self.message_uid = self.group_id
+
+        if self.rollout_id is None:
+            self.rollout_id = self.uid
+        else:
+            self.uid = self.rollout_id


Claude: Compat limitation — model_post_init only runs at construction time. Post-construction mutations like rollout_state.rollout_id = uuid.uuid4().int (done in agent_in_localhost_loop.py:175 and agent_in_sandbox_loop.py:229) won't update the deprecated uid field. If downstream code reads rollout_state.uid after such a mutation, it sees the stale init-time value.

If the compat layer is intended to keep the pairs always in sync, consider a Pydantic @model_validator(mode="wrap") or property-based approach. If init-only sync is sufficient for the expected downstream usage pattern, a brief docstring note would help future readers understand the limitation.

claude · 2026-06-17T12:14:34Z

Claude: ## Summary

Renames three RolloutState fields (uid → rollout_id, message_uid → group_id, session_uid → session_id) across ~26 files. Adds backward-compat shims for uid/message_uid via model_post_init. Removes the XTUNER_DETERMINISTIC branching in the sampler (all IDs are now sequential). Also adds an unrelated ray_debug_snapshot.sh dev script and moves a test file.

ProduceBatchResult impact: Field renames propagate through rollout_states in ProduceBatchResult — uid/message_uid fields are still populated via the compat layer, so downstream consumers reading those fields from produced batches should be unaffected.

RoutedExperts impact: not affected.

Issues

Critical

sampler.py:106-110 — session_id is never assigned in the sampler. The old code set session_uid = new_data.uid in the deterministic branch. After the rename and removal of the XTUNER_DETERMINISTIC branch, session_id stays None, so controller.py always falls back to uuid4().int, breaking deterministic session routing. (Same issue as prior review — still present after commits 2-3.)

Warning

rl_data.py:99-100 — No backward-compat shim for session_uid → session_id, unlike the other two renames. With extra="forbid", downstream code passing session_uid=... to RolloutState() will get a ValidationError.
rl_data.py:142-151 — model_post_init only syncs deprecated fields at construction time. Post-construction mutations (e.g., rollout_state.rollout_id = uuid4().int in agent_in_localhost_loop.py:175) won't update uid, leaving the compat pair out of sync.

Nit

ray_debug_snapshot.sh and the test file move (tests/rl/fast/pr_fast/ → tests/rl/) are unrelated to the rename — per project guidelines, one logical change per PR.

Verdict

REQUEST_CHANGES

hhaAndroid · 2026-06-17T12:22:05Z

@claude review

claude · 2026-06-17T12:26:42Z

            data = cast(RolloutState, next(self.dataloader_iter)[0])
            data = put_to_ray(data)

-        if XTUNER_DETERMINISTIC:
-            message_uid = self._consumed_samples
-            uid_base = self._consumed_samples * self.prompt_repeat_k
+        group_id = self._consumed_samples
+        rollout_id_base = self._consumed_samples * self.prompt_repeat_k

        group_data = []
        for item_idx in range(self.prompt_repeat_k):
            new_data = copy.deepcopy(data)
+            rollout_id = rollout_id_base + item_idx
+            new_data.group_id = group_id
+            new_data.rollout_id = rollout_id
            if XTUNER_DETERMINISTIC:
-                new_data.message_uid = message_uid
-                new_data.uid = uid_base + item_idx
-                new_data.session_uid = new_data.uid
-            else:
-                new_data.uid = uuid4().int
+                new_data.session_id = rollout_id
+            # Deprecated compatibility field for downstream libraries.
+            # TODO: remove after callers migrate to rollout_id.
+            new_data.uid = rollout_id


Claude: Behavioral change — In the old code, non-deterministic mode used uuid4().int for uid, guaranteeing global uniqueness. Now rollout_id is always consumed_samples * prompt_repeat_k + item_idx — sequential from 0. If multiple Sampler instances exist (e.g., across distributed ranks), their rollout_id values will collide.

This affects export_training_trace.remote(str(rollout_state.rollout_id), ...) in agent_in_localhost_loop.py and trajectory logging in rl_trainer.py. If the store or trace consumers assume uniqueness, collisions could overwrite data.

If this is intentional (e.g., one sampler per cluster, or uniqueness isn't required), please note it in the PR description so future readers understand the contract.

claude · 2026-06-17T12:26:44Z

+    # TODO: remove after callers migrate to ``rollout_id``.
    uid: int | None = None
    task_name: str | None = None
    status: Status = Status.INIT
    error_msg: str | None = None


Claude: Inconsistent compat — uid → rollout_id has a deprecated field + model_post_init sync, but message_uid → group_id and session_uid → session_id have no compat shim at all. With extra="forbid", any downstream code constructing RolloutState(message_uid=...) or RolloutState(session_uid=...) will get a ValidationError.

If uid needs compat for external consumers, please document why the other two don't (e.g., "no known external callers use message_uid/session_uid as constructor kwargs"). If they do have external callers, they need the same treatment.

claude · 2026-06-17T12:27:06Z

Claude: ## Summary

Renames uid → rollout_id, message_uid → group_id, session_uid → session_id across ~26 files. Keeps a backward-compat shim for uid only. Switches rollout_id assignment from uuid4().int to sequential integers in all modes. Adds an unrelated ray_debug_snapshot.sh dev script and moves a test file.

ProduceBatchResult impact: rollout_states in ProduceBatchResult carry the renamed fields. The uid compat shim keeps uid populated via model_post_init, but message_uid and session_uid are gone — any downstream reader accessing those fields on produced batches will get AttributeError.

RoutedExperts impact: not affected.

Issues

Warning

sampler.py:96-112 — rollout_id changed from uuid4().int (globally unique) to sequential integers in non-deterministic mode. If multiple samplers run across ranks, IDs will collide. Document whether this is intentional.
rl_data.py:131-135 — Inconsistent compat: uid gets a deprecated field + model_post_init sync, but message_uid and session_uid have no compat shim. With extra="forbid", downstream code constructing RolloutState(message_uid=...) will break.
rl_data.py:139 — model_post_init only syncs uid ↔ rollout_id at construction. Post-construction mutations (e.g., rollout_state.rollout_id = uuid4().int in agent_in_localhost_loop.py:175) won't update uid. (Same issue as prior review — still present.)

Fixed since prior review

session_id assignment in sampler: commit 4 restores XTUNER_DETERMINISTIC branching, matching the old session_uid behavior. ✓

Nit

ray_debug_snapshot.sh and the test file move are unrelated to the rename — one logical change per PR.
PR description is empty. A rename of a core data model field across 26 files deserves a brief rationale and migration note.

Verdict

COMMENT — The critical session_id bug from prior reviews is fixed. Remaining items are warnings that the author should address or explicitly acknowledge as intentional. No blocking correctness issue found.

hhaAndroid · 2026-06-17T12:34:59Z

@claude review and approve

claude · 2026-06-17T12:37:45Z

Claude: ## Summary

Renames uid → rollout_id, message_uid → group_id, session_uid → session_id across ~25 files. Adds a backward-compat shim for uid only (model_post_init). Switches agent loops to use session_id (instead of rollout_id) as the trace-store key and rollout-item identity. Makes group_id always assigned in the sampler (previously only under XTUNER_DETERMINISTIC).

ProduceBatchResult impact: rollout_states carry renamed fields. The uid compat shim keeps uid populated via model_post_init. message_uid and session_uid are removed without shims — external consumers reading those fields on produced batches will get AttributeError.

RoutedExperts impact: not affected.

Issues

Warning (previously raised, noting status)

rl_data.py model_post_init — uid ↔ rollout_id sync is construction-only. Post-construction mutations won't update the deprecated uid. After patch 5 this is less risky since agent loops now mutate session_id instead of rollout_id, but the trap remains for future code.
rl_data.py — Asymmetric compat: uid gets a shim, message_uid/session_uid don't. With extra="forbid", downstream constructors passing those old names will raise ValidationError. Intentional if no external callers exist, but worth a one-line comment.

Fixed since prior review

session_id assignment in sampler restored under XTUNER_DETERMINISTIC (patch 4). ✓
ray_debug_snapshot.sh added then removed — net zero in final diff. ✓

Nit

Test file move (tests/rl/fast/pr_fast/ → tests/rl/) is unrelated to the rename.

Verdict

APPROVE — The critical session-routing bug is resolved. Remaining items are acknowledged warnings from prior rounds; no blocking correctness issue.

claude

Claude: Approving — the critical session_id bug is fixed and the rename is mechanically correct across all 25 files. The remaining compat asymmetry warnings from prior rounds are design choices, not blockers.

rename uid

cabb110