fix(rollout): fix nested group sorting in fully async rollout - #2345
Open
looput wants to merge 1 commit into
Open
fix(rollout): fix nested group sorting in fully async rollout#2345looput wants to merge 1 commit into
looput wants to merge 1 commit into
Conversation
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.
Summary
list[list[Sample]]groups in fully async rollout sorting.Sample.indexvalue.list.indexmethod as a sample index.Bug Trigger Cause
The fully-async rollout path assumes that each rollout group is a flat:
list[Sample]However, the multi-agent(
examples/multi_agent) custom generation function may return multiple samples for a single input sample. After generate_and_rm_group gathers the results, theactual structure becomes:
list[list[Sample]]The original sorting logic directly reads .index from the first-level item:
index = getattr(sample, "index", None)In the nested case, sample is a Python list. Since Python lists already have a built-in index() method, getattr(sample, "index") returns a
builtin_function_or_method instead of a numeric Sample.index.
The code then executes:
int(index)which raises:
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'builtin_function_or_method'
In short, the sorting logic did not support nested sample groups and mistakenly treated Python's list.index method as the sample index.
Validation
TypeError.python -m py_compile slime/rollout/fully_async_rollout.py.#2344