Addition of LoRA configuration control #175
Conversation
|
|
| @@ -25,6 +25,7 @@ def load_model( | |||
| revision: str = DEFAULT_FLORENCE2_MODEL_REVISION, | |||
| device: str | torch.device = "auto", | |||
| optimization_strategy: OptimizationStrategy = OptimizationStrategy.NONE, | |||
| peft_advanced_params: Optional[dict] = None, # added by me | |||
There was a problem hiding this comment.
let's drop # added by me comment here, and in all other places
| target_modules=["q_proj", "o_proj", "k_proj", "v_proj", "linear", "Conv2d", "lm_head", "fc2"], | ||
| task_type="CAUSAL_LM", | ||
| ) | ||
| default_params = { |
There was a problem hiding this comment.
Please move this to the top of the file, below DEFAULT_FLORENCE2_MODEL_REVISION and rename it as DEFAULT_FLORENCE2_PEFT_PARAMS.
| "task_type": "CAUSAL_LM", | ||
| } | ||
| if peft_advanced_params is not None: | ||
| default_params.update(peft_advanced_params) |
There was a problem hiding this comment.
what will happen if peft_advanced_params will contain unsupported keys? maybe we should validate it?
| @@ -79,6 +84,9 @@ def train( | |||
| metrics=metrics, | |||
| max_new_tokens=max_new_tokens, | |||
| random_seed=random_seed, | |||
| peft_advanced_params=json.loads(peft_advanced_params) | |||
There was a problem hiding this comment.
let's move json.loads(peft_advanced_params) if peft_advanced_params is not None else None above config initialization and assign it to a separate variable, before passing here. json.loads may also throw exception during parsing. I think we should wrap it in try-except clause and than use logger.error to provide users with usefull message.
|
Hi @shataxiDubey 👋🏻 very good PR! thank you so much for opening it. I left a few comments focusing primarily on the Florence-2 codebase, but please apply my remarks to all models. |
|
Hi @SkalskiP, I have made the required changes. |
|
Hi @shataxiDubey 👋🏻 thanks a lot. I'm a bit sick. I'll try to take a look on Monday. Looks like ruff is complaining about formatting https://results.pre-commit.ci/run/github/723015178/1740603048.OIUsnIyOS4CF5dfLiDbiEg. Could you try to solve those issues? |
|
Hi @shataxiDubey 👋🏻 I see you're working on improvements. Let me know if you need any help. |
|
Hi @SkalskiP , I have made the changes but the checks are completing with failure. As discussed earlier, we would send the peft_advanced_params as a string from cli, but checks are suggesting different datatype. |
|
Resolved the type checking issue. |
I am a Master's student from IITGN and a colleague of @patel-zeel
Resolved the issue as per the requirement discussed in #162.
Docs
Following files are updated for florence_2, paligemma_2, qwen_2_5_vl: