[Bugfix] Refactor QuantizationMixin to use resolved config #1912
Conversation
Signed-off-by: Brian Dellabetta <bdellabe@redhat.com>
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
|
|
||
| config_groups: Optional[Dict[str, QuantizationScheme]] = None | ||
| targets: Union[str, List[str]] = Field(default_factory=list) | ||
| targets: Union[str, List[str]] = Field(default_factory=lambda: ["Linear"]) |
There was a problem hiding this comment.
this returns to the default behavior. the setting to ["Linear"] was part of the validation layer that is now removed
There was a problem hiding this comment.
Is this going to conflict with plans to use AWQ without quantization targets? You could also potentially set the AWQ subclass default to be None
There was a problem hiding this comment.
Consider adding a comment encouraging use of resolved_targets
There was a problem hiding this comment.
Regarding AWQ, I think we can get by with this default, but allowing the type to be Optional so that a user can set it to None. I wanted to do that with an example in a follow-up PR.
Regarding a comment, I did so in the docstring. If changing the field name to unresolved_targets is preferable, I can do that at as well. WDYT? It can still be serialized as targets by setting alias="targets" in the pydantic Field inputs
Signed-off-by: Brian Dellabetta <bdellabe@redhat.com>
| """ | ||
| if self._resolved_targets is None: | ||
| targets = [] | ||
| for config_group in self.resolved_config.config_groups.values(): |
There was a problem hiding this comment.
Consider using a set
return (
set(cg.targets for cg in self.resolved_config.config_groups.values())
)There was a problem hiding this comment.
yeah, i wasn't sure about order, but it looks like match_named_modules doesn't care about order. will switch to set
There was a problem hiding this comment.
Switched to set, but that for comprehension doesn't work. set([1,2,3,4]) is acceptable but set([[1,2],[3,4]]) is not
|
|
||
| config_groups: Optional[Dict[str, QuantizationScheme]] = None | ||
| targets: Union[str, List[str]] = Field(default_factory=list) | ||
| targets: Union[str, List[str]] = Field(default_factory=lambda: ["Linear"]) |
There was a problem hiding this comment.
Is this going to conflict with plans to use AWQ without quantization targets? You could also potentially set the AWQ subclass default to be None
|
|
||
| config_groups: Optional[Dict[str, QuantizationScheme]] = None | ||
| targets: Union[str, List[str]] = Field(default_factory=list) | ||
| targets: Union[str, List[str]] = Field(default_factory=lambda: ["Linear"]) |
There was a problem hiding this comment.
Consider adding a comment encouraging use of resolved_targets
Signed-off-by: Brian Dellabetta <bdellabe@redhat.com>
2 participants
SUMMARY:
Fixes #1906
This refactors QuantizationMixin to not update any pydantic fields during validation. Rather than modifying them in order to make them the source of truth, this adds properties
resolved_config&resolved_targetsthat all modifiers should instead use as source of truth. These are resolved once, when needed, and not serialized, which should fix the bug in #1906TEST PLAN:
"please outline how the changes were tested"