Enable LoRA target names arg #2166
Conversation
divyashreepathihalli
changed the title
|
Could you add a little bit of context on why we need to make |
|
This would give the users flexibility to specify custom target names. It was a feature requested by internal customers teams. |
| target_names = self.get_lora_target_names() | ||
|
|
||
| if target_names is None: | ||
| target_names = self.get_lora_target_names() |
There was a problem hiding this comment.
Calling get_lora_target_names() on a model won't return target_names if target_names is not None which could be confusing.
There was a problem hiding this comment.
That is the expected behavior right?
if None -> get default name
else -> use the names provided
There was a problem hiding this comment.
The expected behavior for get_lora_target_names() doesn't seem correct to me. I expect get_lora_target_names() to always return the target_names that is passed as the argument but here it always returns defaults. For example, if you do this:
model.enable_lora(rank=2, target_names=["query"])
model.get_lora_target_names() # this will return ["query_dense", "value_dense", "query", "value"] but should return ["query"]
There was a problem hiding this comment.
get_lora_target_names() is not an exposed method right?
There was a problem hiding this comment.
Even if it's not exposed, I think it should have the correct behavior of returning the actual target names.
And here get_lora_target_names() is a public function and it's exposed. right?
There was a problem hiding this comment.
Hmmm.. okay - let me send a quick fix
There was a problem hiding this comment.
here - #2167
No description provided.