-
Notifications
You must be signed in to change notification settings - Fork 389
add peft to recipe qwen3vl #2023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
9432fc2
af84274
55c5569
106080b
69935a6
6700f60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,14 +216,27 @@ def main() -> None: | |
| use_preloaded_flag = bool(args.data_path) or bool(getattr(args, "use_preloaded", False)) | ||
| dataset_type = args.dataset_type or ("preloaded" if use_preloaded_flag else "mock") | ||
|
|
||
| cfg: ConfigContainer = pretrain_config( | ||
| dataset_type=dataset_type, | ||
| train_data_path=args.data_path, | ||
| valid_data_path=None, | ||
| test_data_path=None, | ||
| image_folder=args.image_folder, | ||
| pretrained_checkpoint=args.pretrained_checkpoint, | ||
| ) | ||
| peft_value = None | ||
| for override in cli_overrides: | ||
| if override.startswith("peft=") or override.startswith("+peft="): | ||
| peft_value = override.split("=", 1)[1] | ||
| break | ||
|
|
||
| # Build recipe kwargs | ||
| recipe_kwargs = { | ||
| "dataset_type": dataset_type, | ||
| "train_data_path": args.data_path, | ||
| "valid_data_path": None, | ||
| "test_data_path": None, | ||
| "image_folder": args.image_folder, | ||
| "pretrained_checkpoint": args.pretrained_checkpoint, | ||
| } | ||
|
|
||
| # Add peft parameter if specified | ||
| if peft_value is not None: | ||
| recipe_kwargs["peft"] = peft_value | ||
|
|
||
| cfg: ConfigContainer = pretrain_config(**recipe_kwargs) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid clobbering PEFT objects with Hydra overrides. You extract 🐛 Suggested fix (filter out peft override before Hydra parsing)- peft_value = None
- for override in cli_overrides:
- if override.startswith("peft=") or override.startswith("+peft="):
- peft_value = override.split("=", 1)[1]
- break
+ peft_value = None
+ filtered_cli_overrides: list[str] = []
+ for override in cli_overrides:
+ if override.startswith("peft=") or override.startswith("+peft="):
+ peft_value = override.split("=", 1)[1]
+ continue
+ filtered_cli_overrides.append(override)
+ cli_overrides = filtered_cli_overrides🤖 Prompt for AI Agents |
||
| logger.info("Loaded base configuration") | ||
|
|
||
| if get_rank_safe() == 0: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add implementation details for PEFT setup.
This section has motivation and examples, but it doesn’t yet explain how PEFT is wired in the recipe (configuration keys, defaults, and override behavior). The docs requirement for new key features explicitly calls for implementation details.
📌 Suggested doc addition
As per coding guidelines: All new key features (enabling a new model, enabling a new parallelism strategy) must include documentation update explaining motivation, technical approach, usage examples, and implementation details.
📝 Committable suggestion
🤖 Prompt for AI Agents