Skip to content

fix: validate ngram_size in get_repetition_penalty_reward - #7129

Closed
CJstate wants to merge 1 commit into
huggingface:mainfrom
CJstate:fix-7015-ngram-size-validation
Closed

CJstate wants to merge 1 commit into
huggingface:mainfrom
CJstate:fix-7015-ngram-size-validation

Conversation

@CJstate

@CJstate CJstate commented Sep 9, 2026

Copy link
Copy Markdown

What does this PR do?

Adds construction-time validation for ngram_size in get_repetition_penalty_reward. Zero or negative values were accepted at construction time but caused a ZeroDivisionError (or silently produced empty n-grams) during reward computation, as reported in #7015.

The fix adds a check consistent with the existing max_penalty validation:

if ngram_size <= 0:
    raise ValueError(f"ngram_size {ngram_size} should be greater than 0")

Also adds a parametrized regression test covering ngram_size=0 and ngram_size=-1.

Fixes #7015

Before submitting

AI writing disclosure

  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.


Note

Low Risk
Small input-validation change in a reward helper with regression tests; no auth, data, or training-path behavior changes beyond failing fast on invalid config.

Overview
get_repetition_penalty_reward now rejects non-positive ngram_size at construction with a ValueError, matching the existing max_penalty guard. Previously, 0 or negative values could slip through and fail during scoring (e.g. ZeroDivisionError when ngram_size=0).

A parametrized test in tests/test_rewards.py asserts that ngram_size 0 and -1 raise ValueError.

Reviewed by Cursor Bugbot for commit 4eec16b. Bugbot is set up for automated code reviews on this repo. Configure here.

Zero or negative ngram_size values were accepted at construction time but
caused a ZeroDivisionError (or silently empty n-grams) during reward
computation. Add construction-time validation consistent with the existing
max_penalty check.

Fixes huggingface#7015.
@albertvillanova

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_repetition_penalty_reward accepts non-positive ngram_size

2 participants