Skip to content

fix: preference filter applies both length limits#1761

Open
Chessing234 wants to merge 2 commits into
allenai:mainfrom
Chessing234:fix/preference-filter-both-length-limits
Open

fix: preference filter applies both length limits#1761
Chessing234 wants to merge 2 commits into
allenai:mainfrom
Chessing234:fix/preference-filter-both-length-limits

Conversation

@Chessing234

Copy link
Copy Markdown
Contributor

Summary

  • Preference filter used a nested ternary: if max_prompt_token_length was set, max_token_length was never checked.
  • Match SFT: AND both checks independently.

GPU_TESTS=bypass

Test plan

  • Unit: preference row with ok prompt length but oversize chosen/rejected is filtered when both limits set

Made with Cursor

Chessing234 and others added 2 commits July 17, 2026 09:08
Nested ternary skipped max_token_length whenever max_prompt_token_length was set.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the preference dataset filter in open_instruct/dataset_processor.py to apply both max_prompt_token_length and max_token_length constraints simultaneously, rather than treating them as mutually exclusive. A review comment suggests optimizing this filtering function by returning early (short-circuiting) as soon as any length check fails, which avoids unnecessary evaluations on large datasets.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +250 to +261
max_prompt_token_length_ok = True
if self.config.max_prompt_token_length is not None:
max_prompt_token_length_ok = len(row[INPUT_IDS_PROMPT_KEY]) <= self.config.max_prompt_token_length

max_token_length_ok = True
if self.config.max_token_length is not None:
max_token_length_ok = (
len(row[INPUT_IDS_CHOSEN_KEY]) <= self.config.max_token_length
and len(row[INPUT_IDS_REJECTED_KEY]) <= self.config.max_token_length
if self.config.max_token_length is not None
else True
)
)

return max_prompt_token_length_ok and max_token_length_ok

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

We can optimize this filtering function by returning early (short-circuiting) as soon as any length check fails. This avoids unnecessary list length evaluations and boolean operations on subsequent checks, which can improve performance when processing large datasets.

Suggested change
max_prompt_token_length_ok = True
if self.config.max_prompt_token_length is not None:
max_prompt_token_length_ok = len(row[INPUT_IDS_PROMPT_KEY]) <= self.config.max_prompt_token_length
max_token_length_ok = True
if self.config.max_token_length is not None:
max_token_length_ok = (
len(row[INPUT_IDS_CHOSEN_KEY]) <= self.config.max_token_length
and len(row[INPUT_IDS_REJECTED_KEY]) <= self.config.max_token_length
if self.config.max_token_length is not None
else True
)
)
return max_prompt_token_length_ok and max_token_length_ok
if self.config.max_prompt_token_length is not None:
if len(row[INPUT_IDS_PROMPT_KEY]) > self.config.max_prompt_token_length:
return False
if self.config.max_token_length is not None:
if (
len(row[INPUT_IDS_CHOSEN_KEY]) > self.config.max_token_length
or len(row[INPUT_IDS_REJECTED_KEY]) > self.config.max_token_length
):
return False
return True

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