Conversation
Pull Request resolved: #17229 Introduce LoraConfig to hold lora parameters such as: - checkpoint - rank - target_modules (e.g. q_proj, k_proj, v_proj, up_proj, down_proj, gate_proj, o_proj) - lora_alpha LoraConfig validation done post-init. LoraConfig can be created with config.json file. Update cases of export_llama_lib to use LoraConfig instead of adapter_checkpoint and adapter_config. NOTE: we may need to extend this to support more customizable features like lora config per layer etc. cc @hakanb ghstack-source-id: 340930093 @exported-using-ghexport Differential Revision: [D92304723](https://our.internmc.facebook.com/intern/diff/D92304723/)
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17458
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 184 PendingAs of commit b25bd71 with merge base c12dc35 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a LoraConfig dataclass to consolidate LoRA adapter configuration parameters (checkpoint path, rank, alpha, and target modules) that were previously managed as separate fields (adapter_checkpoint and adapter_config) in BaseConfig. The refactoring improves organization by encapsulating related LoRA parameters into a single configuration object.
Changes:
- Introduced
LoraConfigdataclass with support for both JSON file-based and explicit parameter configuration - Refactored
BaseConfigto useLoraConfiginstead of separateadapter_checkpointandadapter_configfields - Updated model loading logic to use
LoraConfigfor adapter parameter resolution - Updated YAML configurations and test scripts to use the new
LoraConfigstructure
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| extension/llm/export/config/llm_config.py | Introduces LoraConfig dataclass; updates BaseConfig to use lora_config field; updates from_args to construct LoraConfig from CLI args |
| examples/models/qwen3/config/qwen3_xnnpack_lora.yaml | Adds lora_config nested structure with adapter_checkpoint and adapter_config fields using OmegaConf environment variable interpolation |
| examples/models/llama/model.py | Updates model initialization to use lora_config instead of separate adapter fields; adds logic to parse adapter_config JSON when needed; removes redundant validation code |
| .ci/scripts/test_lora.sh | Updates test script to use environment variables for LoRA config paths; switches to qwen3_xnnpack_lora.yaml config; adds override for lora_config=null in quantization test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
The LoraConfig dataclass lacks validation to ensure that either adapter_config is provided OR all explicit values (lora_rank, lora_alpha, target_modules) are provided. According to the documentation, these are two mutually exclusive ways to configure LoRA, but there's no post_init method to validate this constraint. This could lead to invalid configurations where adapter_checkpoint is provided but neither adapter_config nor the explicit parameters are set, resulting in runtime errors when the config is used in model.py (lines 72-77).
| def __post_init__(self) -> None: | |
| """ | |
| Validate that LoRA configuration is provided in exactly one of the two | |
| supported ways: | |
| 1. Via adapter_config JSON (adapter_config is not None), in which case | |
| explicit LoRA parameters must not be set. | |
| 2. Via explicit values (adapter_config is None), in which case all of | |
| lora_rank, lora_alpha, and target_modules must be provided. | |
| """ | |
| has_adapter_config = self.adapter_config is not None | |
| has_explicit_params = ( | |
| self.lora_rank != 0 | |
| or self.lora_alpha != 0 | |
| or bool(self.target_modules) | |
| ) | |
| # Enforce mutual exclusivity between adapter_config and explicit params. | |
| if has_adapter_config and has_explicit_params: | |
| raise ValueError( | |
| "LoraConfig must be configured either with 'adapter_config' or " | |
| "with explicit parameters ('lora_rank', 'lora_alpha', " | |
| "'target_modules'), but not both." | |
| ) | |
| # If no adapter_config is provided, require all explicit parameters. | |
| if not has_adapter_config: | |
| if self.lora_rank <= 0 or self.lora_alpha <= 0 or not self.target_modules: | |
| raise ValueError( | |
| "Invalid LoraConfig: when 'adapter_config' is not provided, " | |
| "'lora_rank' and 'lora_alpha' must be positive and " | |
| "'target_modules' must be a non-empty list." | |
| ) |
| if lora_config.adapter_config and lora_config.lora_rank == 0: | ||
| with open(lora_config.adapter_config, "r") as f: | ||
| cfg = json.load(f) | ||
| lora_config.lora_rank = cfg["r"] | ||
| lora_config.lora_alpha = cfg["lora_alpha"] | ||
| lora_config.target_modules = cfg["target_modules"] |
There was a problem hiding this comment.
Directly mutating dataclass fields can lead to unexpected behavior, especially when using OmegaConf which creates structured configs. Instead of mutating lora_config in-place, the config values should be validated and populated during initialization or in a post_init hook. This mutation breaks the principle that configs should be immutable after creation and can cause issues if the same config object is reused or if OmegaConf features like interpolation or merging are used.
| Note: user is responsible for parsing the config and | ||
| ensure it doesn't conflict with any explicit values. |
There was a problem hiding this comment.
The documentation note on lines 76-77 states "user is responsible for parsing the config and ensure it doesn't conflict with any explicit values", but this is misleading. Looking at the implementation in model.py (lines 72-77), the config JSON is only parsed when lora_rank is 0, and the values are directly assigned without any conflict checking. If a user provides both adapter_config and explicit values, the explicit values will be silently overwritten if lora_rank is 0. This behavior should either be prevented through validation, or the documentation should be updated to accurately reflect what happens.
| Note: user is responsible for parsing the config and | |
| ensure it doesn't conflict with any explicit values. | |
| Note: when adapter_config is provided and lora_rank is left at its | |
| default value (0), values loaded from the JSON (such as lora_rank, | |
| lora_alpha, and target_modules) will overwrite any explicit values | |
| passed to this dataclass; no conflict checking is performed. |
| # Resolve LoRA params from adapter_config JSON if not already set. | ||
| if lora_config.adapter_config and lora_config.lora_rank == 0: | ||
| with open(lora_config.adapter_config, "r") as f: | ||
| cfg = json.load(f) | ||
| lora_config.lora_rank = cfg["r"] | ||
| lora_config.lora_alpha = cfg["lora_alpha"] | ||
| lora_config.target_modules = cfg["target_modules"] |
There was a problem hiding this comment.
The condition 'lora_config.lora_rank == 0' is used to decide whether to parse adapter_config JSON. However, 0 is the default value for lora_rank, which creates an ambiguous situation: it's unclear whether lora_rank is 0 because it wasn't set (and should be loaded from JSON) or because the user explicitly wants a rank of 0. A rank of 0 would be invalid for LoRA (since it means no low-rank adaptation). Consider using None as the default value for lora_rank instead of 0, or checking if adapter_config is provided and any of the LoRA parameters are still at their defaults.
| # Resolve LoRA params from adapter_config JSON if not already set. | |
| if lora_config.adapter_config and lora_config.lora_rank == 0: | |
| with open(lora_config.adapter_config, "r") as f: | |
| cfg = json.load(f) | |
| lora_config.lora_rank = cfg["r"] | |
| lora_config.lora_alpha = cfg["lora_alpha"] | |
| lora_config.target_modules = cfg["target_modules"] | |
| # Resolve LoRA params from adapter_config JSON if provided, without | |
| # relying on a specific sentinel value for lora_rank. | |
| if lora_config.adapter_config: | |
| with open(lora_config.adapter_config, "r") as f: | |
| cfg = json.load(f) | |
| if not lora_config.lora_rank: | |
| lora_config.lora_rank = cfg["r"] | |
| if not lora_config.lora_alpha: | |
| lora_config.lora_alpha = cfg["lora_alpha"] | |
| if not lora_config.target_modules: | |
| lora_config.target_modules = cfg["target_modules"] |
| cfg = json.load(f) | ||
| lora_config.lora_rank = cfg["r"] | ||
| lora_config.lora_alpha = cfg["lora_alpha"] | ||
| lora_config.target_modules = cfg["target_modules"] |
There was a problem hiding this comment.
Missing error handling when loading adapter_config JSON. If the JSON file is malformed or missing required keys like 'r', 'lora_alpha', or 'target_modules', this will raise a KeyError without a clear error message. Consider wrapping this in a try-except block with a more informative error message, or add validation after loading to check that all required fields are present.
| cfg = json.load(f) | |
| lora_config.lora_rank = cfg["r"] | |
| lora_config.lora_alpha = cfg["lora_alpha"] | |
| lora_config.target_modules = cfg["target_modules"] | |
| try: | |
| cfg = json.load(f) | |
| except json.JSONDecodeError as e: | |
| raise ValueError( | |
| f"Failed to parse LoRA adapter config JSON file " | |
| f"'{lora_config.adapter_config}': {e}" | |
| ) from e | |
| try: | |
| lora_config.lora_rank = cfg["r"] | |
| lora_config.lora_alpha = cfg["lora_alpha"] | |
| lora_config.target_modules = cfg["target_modules"] | |
| except KeyError as e: | |
| missing_key = e.args[0] if e.args else "unknown" | |
| raise ValueError( | |
| "Missing required key " | |
| f"'{missing_key}' in LoRA adapter config JSON file " | |
| f"'{lora_config.adapter_config}'. Expected keys: " | |
| "'r', 'lora_alpha', 'target_modules'." | |
| ) from e |
| lora_config.lora_alpha = cfg["lora_alpha"] | ||
| lora_config.target_modules = cfg["target_modules"] | ||
|
|
||
| adapter_checkpoint_path = lora_config.adapter_checkpoint |
There was a problem hiding this comment.
Missing validation for adapter_checkpoint path. When lora_config is provided, adapter_checkpoint_path could be invalid (file doesn't exist, path is empty, etc.), but there's no explicit check before attempting to load it. The code will only fail with an unclear error when torch.load or the safetensors loader is called. Consider adding early validation to check that the file exists and is readable, with a clear error message.
Pull Request resolved: pytorch#17229 Introduce LoraConfig to hold lora parameters such as: - checkpoint - rank - target_modules (e.g. q_proj, k_proj, v_proj, up_proj, down_proj, gate_proj, o_proj) - lora_alpha LoraConfig validation done post-init. LoraConfig can be created with config.json file. Update cases of export_llama_lib to use LoraConfig instead of adapter_checkpoint and adapter_config. NOTE: we may need to extend this to support more customizable features like lora config per layer etc. cc @hakanb ghstack-source-id: 340930093 @exported-using-ghexport Differential Revision: [D92304723](https://our.internmc.facebook.com/intern/diff/D92304723/) Co-authored-by: Github Executorch <github_executorch@arm.com>
Pull Request resolved: #17229
Introduce LoraConfig to hold lora parameters such as:
LoraConfig validation done post-init. LoraConfig can be created with config.json file.
Update cases of export_llama_lib to use LoraConfig instead of adapter_checkpoint and adapter_config.
NOTE: we may need to extend this to support more customizable features like lora config per layer etc. cc @hakanb
ghstack-source-id: 340930093
@exported-using-ghexport
Differential Revision: D92304723