Fix suspicious unused loop iteration variable#41
Open
odaysec wants to merge 1 commit into
Open
Conversation
A for loop iteration variable is not used in the body of the loop, and the loop does not count the number of items in the sequence. This is suspicious as there is rarely any reason to iterate over a sequence and not use the contents. Not using the loop variable can often indicate a logical error. fix this issue you should either (a) remove the unused loop variable/loop if it serves no purpose, or (b) actually use the loop variable inside the loop body in a way that reflects the intended behavior. Here we want to preserve existing functionality (returning a list of zeros of the same length as `completions`) while satisfying the linter and keeping the code ready for future customization, so the best fix is to use `completion` in a no-op way that does not change the result but makes it clear where custom logic will go. The simplest non-invasive change is to add a dummy reference to `completion` in the loop body, such as `_ = completion` directly under the TODO comment. This keeps the behavior identical (Python ignores the value; no side effects), documents that `completion` is the per-item text to be scored, and removes the static analysis warning about the unused variable. Concretely, in `trainers/rewards/custom_rewards.py`, inside `custom_reward_func`, modify the loop starting at line 41 by inserting a single line (e.g. `_ = completion # placeholder use; replace with real scoring logic`) right after the `for completion in completions:` or under the TODO comment. https://docs.python.org/reference/compound_stmts.html#the-for-statement https://docs.python.org/tutorial/controlflow.html#for-statements
Contributor
Author
|
Hi @youngwanlim, could you please review it! and merged this request as for patched Kind regards |
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.
A for loop iteration variable is not used in the body of the loop, and the loop does not count the number of items in the sequence. This is suspicious as there is rarely any reason to iterate over a sequence and not use the contents. Not using the loop variable can often indicate a logical error.
fix this issue you should either (a) remove the unused loop variable/loop if it serves no purpose, or (b) actually use the loop variable inside the loop body in a way that reflects the intended behavior. Here we want to preserve existing functionality (returning a list of zeros of the same length as
completions) while satisfying the linter and keeping the code ready for future customization, so the best fix is to usecompletionin a no-op way that does not change the result but makes it clear where custom logic will go.The simplest non-invasive change is to add a dummy reference to
completionin the loop body, such as_ = completiondirectly under the TODO comment. This keeps the behavior identical (Python ignores the value; no side effects), documents thatcompletionis the per-item text to be scored, and removes the static analysis warning about the unused variable. Concretely, intrainers/rewards/custom_rewards.py, insidecustom_reward_func, modify the loop starting at line 41 by inserting a single line (e.g._ = completion # placeholder use; replace with real scoring logic) right after thefor completion in completions:or under the TODO comment.python-statement
python controlflow