Skip to content

fix: correct generated group type annotations - #2241

Open
guapisolo wants to merge 1 commit into
mainfrom
jiajun/fix-generated-group-type
Open

fix: correct generated group type annotations#2241
guapisolo wants to merge 1 commit into
mainfrom
jiajun/fix-generated-group-type

Conversation

@guapisolo

Copy link
Copy Markdown
Collaborator

Summary

Correct generated rollout group annotations to include expanded sample lists.

Symptom & Reproduction

  • Symptom: generate_and_rm_group declares a flat list[Sample] even though one trajectory can expand into list[Sample], so type consumers receive a false flat-only contract.
  • Reproduction: Inspect typing.get_type_hints(generate_and_rm_group)["return"] while tracing the asyncio.gather inputs from generate_and_rm.

Root Cause

  1. generate_and_rm returns Sample | list[Sample] per trajectory.
  2. asyncio.gather preserves each trajectory result inside the group list.
  3. generate_and_rm_group and its direct sync consumer declared only list[Sample].

Fix

Declare the producer result and the direct sync consumer as list[Sample | list[Sample]], matching the existing runtime shape without changing generation, ordering, reward, or filtering behavior.

Verification

  • New / updated test: No permanent test was needed.
  • Type contract: A focused get_type_hints() assertion verifies the corrected contract.
  • Existing test suite: 30 rollout tests verify preserved behavior.

Review Focus

  • Scrutinize the annotations in generate_and_rm_group and generate_rollout_async.
  • Confirm the diff contains no runtime statement changes.

generate_and_rm_group gathers per-sample results that may each expand to multiple samples. Describe the produced group and its direct consumer with the actual nested union type.
@guapisolo
guapisolo changed the base branch from main to yueming/async-buffer-staleness August 7, 2026 03:54
Base automatically changed from yueming/async-buffer-staleness to main August 8, 2026 03:02
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.

1 participant