fixed bug where new runs would always be 50 percent, and added a headless execution option #20
Conversation
WalkthroughAdds CLI flags Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as CLI User
participant C as CLI (args)
participant M as main()
participant MM as ModelModifier
participant I as Interactive select
participant S as SNR assessor
participant O as Outputs (JSON/YAML)
U->>C: Invoke with args (--batch-size?, --top-percent?, --all-layer-types?)
C->>M: main(args)
M->>M: Check for existing SNR results file
alt Results found
M->>MM: Load modifier
MM->>M: get_weight_types()
M->>O: Print weight types + generate unfrozen params YAML
else Results not found
alt batch-size provided
C->>MM: pass batch_size=args.batch_size
else
M->>U: prompt for batch_size
U-->>MM: provide batch_size
end
M->>MM: Instantiate ModelModifier(batch_size, top_percent)
alt --all-layer-types set
MM->>M: get_weight_types()
M->>MM: sort_weight_types()
M->>MM: set modifier.layer_types (non-interactive)
else interactive
M->>I: interactive_select_weights()
I-->>MM: selected layer types
end
alt any selected
M->>S: assess_layers_snr(modifier)
S-->>O: save_snr_to_json()
M->>U: print completion message
else none selected
M->>U: print no-selection message
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spectrum.py (1)
245-246: Guard against None CLI top_percent
spectrum.py:245-246: When--top-percentis omitted,args.top_percentisNone, which overrides the default50and leads to aTypeErroringenerate_unfrozen_params_yaml. Only passtop_percentwhen it’s notNone, or useargs.top_percent if args.top_percent is not None else 50.
🧹 Nitpick comments (1)
spectrum.py (1)
236-236: Align the argument default withModelModifierto prevent None propagation.The
--top-percentargument defaults toNone, which when explicitly passed toModelModifieroverrides its parameter default of50. This misalignment causes the bug described in the next comments.Apply this diff to align the defaults:
- parser.add_argument('--top-percent', type=int, default=None, help='Top percentage of layers to select, overriding the default') + parser.add_argument('--top-percent', type=int, default=50, help='Top percentage of layers to select (default: 50)')
| batch_size = input_dialog(title="Batch Size", text="Enter the batch size:").run() | ||
| batch_size = int(batch_size) if batch_size else 1 | ||
| modifier = ModelModifier(model_name=args.model_name, batch_size=batch_size) | ||
| modifier = ModelModifier(model_name=args.model_name, batch_size=batch_size, top_percent=args.top_percent) |
There was a problem hiding this comment.
Critical: Explicitly passing None overrides the default and causes TypeError.
When the user doesn't provide --top-percent, args.top_percent is None. Passing top_percent=None explicitly to ModelModifier overrides the default parameter value of 50 in the constructor (line 13), setting self.top_percent = None. This None value propagates through save_snr_to_json() → generate_unfrozen_params_yaml() and causes a TypeError at line 197: int(len(layers) * None / 100).
Apply this diff to only pass top_percent when it's not None:
- modifier = ModelModifier(model_name=args.model_name, batch_size=batch_size, top_percent=args.top_percent)
+ modifier = ModelModifier(
+ model_name=args.model_name,
+ batch_size=batch_size,
+ top_percent=args.top_percent if args.top_percent is not None else 50
+ )Alternatively, if you adopt the refactor suggested in the previous comment (setting the argparse default to 50), the current code would work correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modifier = ModelModifier(model_name=args.model_name, batch_size=batch_size, top_percent=args.top_percent) | |
| modifier = ModelModifier( | |
| model_name=args.model_name, | |
| batch_size=batch_size, | |
| top_percent=args.top_percent if args.top_percent is not None else 50 | |
| ) |
🤖 Prompt for AI Agents
In spectrum.py around line 251, passing args.top_percent (which may be None)
into ModelModifier overrides the constructor default of 50 and later causes a
TypeError when used in arithmetic; fix by only including top_percent in the
ModelModifier call when args.top_percent is not None (e.g., construct with
ModelModifier(model_name=..., batch_size=batch_size) and only add
top_percent=args.top_percent when args.top_percent is not None), or
alternatively set the argparse default for --top-percent to 50 so
args.top_percent is never None.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
spectrum.py (1)
262-262: Critical: PassingNonefortop_percentoverrides the constructor default.When
--top-percentis not provided,args.top_percentisNone. Explicitly passingtop_percent=NonetoModelModifieroverrides the default value of50in the constructor (line 13), settingself.top_percent = None. This causes aTypeErrorat line 197:int(len(layers) * None / 100)when calculating the number of top layers.Apply this diff to only pass
top_percentwhen it's notNone:- modifier = ModelModifier(model_name=args.model_name, batch_size=batch_size, top_percent=args.top_percent) + modifier = ModelModifier( + model_name=args.model_name, + batch_size=batch_size, + top_percent=args.top_percent if args.top_percent is not None else 50 + )Alternatively, set the argparse default to
50on line 236:- parser.add_argument('--top-percent', type=int, default=None, help='Top percentage of layers to select, overriding the default') + parser.add_argument('--top-percent', type=int, default=50, help='Top percentage of layers to select, overriding the default')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spectrum.py(3 hunks)
🔇 Additional comments (3)
spectrum.py (3)
254-260: LGTM!The batch size handling correctly supports both CLI input and interactive prompts, with a safe fallback to 1 if the user cancels the dialog.
264-270: LGTM!The
--all-layer-typesflag correctly implements non-interactive selection by fetching, sorting, and assigning all weight types, bypassing the interactive dialog when enabled.
280-280: LGTM!Standard Python idiom for module execution.
|
|
||
| weight_types = modifier.get_weight_types() | ||
| print(weight_types) |
There was a problem hiding this comment.
Critical: AttributeError when model is None.
When an existing SNR file is found, ModelModifier is instantiated without model_name (line 247), setting self.model = None (line 49). Calling modifier.get_weight_types() on line 250 then fails because it tries to iterate over self.model.named_modules() (line 56) when self.model is None.
Apply this diff to pass the model name when an existing SNR file exists:
if os.path.exists(snr_file_path):
print(f"Found existing SNR results file for {args.model_name}")
- modifier = ModelModifier(top_percent=args.top_percent)
+ modifier = ModelModifier(model_name=args.model_name, top_percent=args.top_percent)
modifier.generate_unfrozen_params_yaml(snr_file_path, args.top_percent)
weight_types = modifier.get_weight_types()
print(weight_types)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| weight_types = modifier.get_weight_types() | |
| print(weight_types) | |
| if os.path.exists(snr_file_path): | |
| print(f"Found existing SNR results file for {args.model_name}") | |
| modifier = ModelModifier(model_name=args.model_name, top_percent=args.top_percent) | |
| modifier.generate_unfrozen_params_yaml(snr_file_path, args.top_percent) | |
| weight_types = modifier.get_weight_types() | |
| print(weight_types) |
🤖 Prompt for AI Agents
spectrum.py around lines 249 to 251: when an existing SNR file is found the code
instantiates ModelModifier without a model name so self.model is None and
calling modifier.get_weight_types() raises AttributeError; fix this by passing
the current model_name (or the variable that holds the model identifier) into
the ModelModifier constructor where the existing SNR file branch creates the
modifier so that ModelModifier initializes with a valid model instance before
get_weight_types() is called.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
giuseppe-zappia
changed the title
|
If you'd like these as separate feature PRs I can split them, but I found them both pretty important. |
1 participant
When setting up a model that hasn't already been scanned, the resulting output will always be the 50% scan regardless of what the user enters. Supplying
top_percentwhen no SNR file exists addresses this issue.Summary by CodeRabbit
New Features
Bug Fixes