converge the Group type alias next to Sample - #2266
Closed
yueming-yuan wants to merge 4 commits into
Closed
yueming-yuan wants to merge 4 commits into
yueming-yuan wants to merge 4 commits into
Conversation
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.
yueming-yuan
requested review from
Shi-Dong,
Zhichenzzz,
fzyzcjy,
guapisolo,
jybsuper and
maocheng23
as code owners
August 7, 2026 22:11
Zhichenzzz
approved these changes
Aug 7, 2026
…p-alias # Conflicts: # miles/rollout/fully_async_rollout.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2241, which corrected
generate_and_rm_group's return annotation to the shape it actually produces. That fix spelled the type out at the call site, making it the third place the same union is written; this converges them and applies the same correction to the legacy producer #2241 did not cover.What was wrong
Group = list[Sample | list[Sample]]was defined independently in two modules (fully_async_rollout.py,multi_lora/async_rollout.py), and spelled out inline in two more after #2241. Consumers rediscovered the shape on their own — hence the hand-rolledisinstance(group[0], list)unwrapping scattered acrossfirst_sample/iter_samples/rollout_data_conversion._first_sample.sglang_rollout.generate_and_rm_groupstill declared-> list[Sample]while doing the sameasyncio.gatherover agenerate_and_rmthat returnsSample | list[Sample]— the exact bug #2241 fixed on the new path.Change
One definition in
miles/utils/types.py, next toSample:types.pyis a leaf module (stdlib + numpy/torch only) and is already imported by every consumer, so this adds no import edges and cannot cycle. Placing it ininference_rollout_common.pynext to the producer was the other candidate, butmulti_loraconsumes the legacysglang_rolloutproducer and would then depend on the new inference path for a type alone.Both producers now declare
-> Group; the four consumers import it instead of redefining or respelling it.Annotations only — no runtime statement changes.
Notes
jiajun/fix-generated-group-type, plus a merge of current main). Until that merges, the diff here shows its commit too; it rebases away cleanly afterwards.py_compileandpre-commit run --all-filesonly. Please runtests/fast/rollout/before merging.🤖 Generated with Claude Code