-
Notifications
You must be signed in to change notification settings - Fork 10
[Transforms] Apply, serialize, deserialize #276
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
base: transform_arg_support
Are you sure you want to change the base?
Conversation
@@ -104,8 +132,92 @@ def load_pretrained_quantization(model: Module, model_name_or_path: str): | |||
) | |||
|
|||
|
|||
def process_transforms_config( | |||
transforms_config: TransformationConfig, |
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.
Another example where I think consistency in naming convention will help users. Something like this?
transforms_config: TransformationConfig, | |
transform_config: TransformConfig, |
358075b
to
fadaaf8
Compare
e7cdea4
to
063d62d
Compare
fadaaf8
to
86e805d
Compare
28c7af5
to
579337d
Compare
state_dict[weight_name] = f.get_tensor(weight_name) | ||
|
||
for name, submodule in iter_named_leaf_modules(model): | ||
transform_data = getattr(submodule, "transform_data", None) |
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.
TransformData includes a dictionary of all the transforms-relevant runtime data, and is attached to the layer as "transform_data"
To me, it seems like this information is essentially duplicating the information of the scheme/args? Why not attach the scheme/args to the module, rather than creating a new abstraction?
@@ -104,8 +137,94 @@ def load_pretrained_quantization(model: Module, model_name_or_path: str): | |||
) | |||
|
|||
|
|||
def process_transforms_config( |
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.
I missed that this is actually modifying model
registering transforms to it. The name process_transforms_config
is pretty innocuous for something like this, given it's modifying model
significantly. Consider renaming to add_transforms_to_model
or something more explicit?
Summary
Process
_
transform_
{idx} e.g. "weight_transform_0" or "input_activation_transform_0" to try and match the convention of weights, input_activations, and output_activations.TransformData
includes a dictionary of all the transforms-relevant runtime data, and is attached to the layer as "transform_data" . The keys of the dictionary correspond to the transform parameter names. Note: in the future, if we decide to add another layer to infer runtime/call args on the fly, we can potentially remove TransformData but that is an optimization we can talk about in the future.Apply
apply_transforms_to_parameter
andapply_inverse_transforms_to_parameter
which sandwich QDQSerialize/Deserialize
quantization_config
is also extended with atransforms_config
in config.jsonload_transforms
function has been added to load the parameters from disk and add the relevant runtime information. However, this requires the above transformers PR indicated aboveExamples:
Testing
Dependencies: