fix: resolve duplicate -g flag and type=bool argparse bug in sft.py#1329
Open
vominh1919 wants to merge 1 commit into
Open
fix: resolve duplicate -g flag and type=bool argparse bug in sft.py#1329vominh1919 wants to merge 1 commit into
-g flag and type=bool argparse bug in sft.py#1329vominh1919 wants to merge 1 commit into
Conversation
Two bugs in the SFT training script's argument parser:
1. Both --gradient-accumulation-steps and --max-grad-norm use '-g' as
their short flag. The second definition shadows the first, so
`python sft.py -g 4` silently sets max_grad_norm=4.0 instead of
gradient_accumulation_steps=4.
2. `type=bool` doesn't work as expected in argparse — bool('False') is
True in Python, so --push-to-hub False still sets the value to True.
Use BooleanOptionalAction instead, which provides --no-push-to-hub.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d2ae717. Configure here.
| parser.add_argument("--weight-decay", "-w", type=float, default=0.01) | ||
| parser.add_argument("--max-grad-norm", "-g", type=float, default=0.1) | ||
| parser.add_argument("--push-to-hub", "-p", type=bool, default=True) | ||
| parser.add_argument("--push-to-hub", "-p", action=argparse.BooleanOptionalAction, default=True) |
There was a problem hiding this comment.
Parsed push_to_hub value never used in SFTConfig
High Severity
The --push-to-hub argparse fix is incomplete. While the CLI now correctly parses --no-push-to-hub via BooleanOptionalAction, the SFTConfig on line 49 hardcodes push_to_hub=True instead of using args.push_to_hub. This means --no-push-to-hub is silently ignored — the model is always pushed to the hub regardless of the flag.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d2ae717. Configure here.
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.


Problem
Two bugs in
scripts/sft.py's argument parser:Duplicate
-gflag: Both--gradient-accumulation-steps(line 69) and--max-grad-norm(line 73) use-gas their short flag. The second definition shadows the first, sopython sft.py -g 4silently setsmax_grad_norm=4.0instead ofgradient_accumulation_steps=4.type=booldoesn't work in argparse:bool("False")isTruein Python — any non-empty string is truthy. The only way to disable--push-to-hubvia CLI is--push-to-hub "", which is unintuitive.Fix
--gradient-accumulation-stepsshort flag from-gto-G(uppercase, since--max-grad-normalready owns-g)type=boolwithaction=argparse.BooleanOptionalAction, which provides both--push-to-huband--no-push-to-hubBefore vs After
sft.py -g 4max_grad_norm=4.0(wrong!)max_grad_norm=4.0(correct,-Gneeded for grad accum)sft.py -G 4gradient_accumulation_steps=4✓sft.py --no-push-to-hubpush_to_hub=False✓sft.py --push-to-hub Falsepush_to_hub=True(wrong!)--no-push-to-hubinsteadTests
Verified syntax with
python3 -c "import ast; ast.parse(open('scripts/sft.py').read())"Note
Low Risk
Low risk: only adjusts CLI argument definitions in
scripts/sft.py, changing how flags are parsed but not training logic or data handling.Overview
Fixes the
scripts/sft.pyCLI so--gradient-accumulation-stepsno longer conflicts with--max-grad-normby switching its short flag from-gto-G.Updates
--push-to-hubto useargparse.BooleanOptionalAction, enabling--push-to-hub/--no-push-to-hubinstead of the brokentype=boolparsing.Reviewed by Cursor Bugbot for commit d2ae717. Bugbot is set up for automated code reviews on this repo. Configure here.