Refactor Target Platform Capabilities - Phase 4 #1301

lior-dikstein · 2024-12-25T13:00:50Z

Replace schema classes from dataclass to pydantic 'BaseModel'.
Fix tests to support pydantic schema classes.
Add test for exporting and loading tp model to json.

Pull Request Description:

Checklist before requesting a review:

I set the appropriate labels on the pull request.
I have added/updated the release note draft (if necessary).
I have updated the documentation to reflect my changes (if necessary).
All function and files are well documented.
All function and classes have type hints.
There is a licenses in all file.
The function and variable names are informative.
I have checked for code duplications.
I have added new unittest (if necessary).

Replace schema classes from dataclass to pydantic 'BaseModel'. Fix tests to support pydantic schema classes. Add test for exporting tp model to json

model_compression_toolkit/target_platform_capabilities/schema/v1.py

ofirgo · 2024-12-25T14:43:47Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+            'quantization_configurations': updated_configs
+        })
+
+    def clone_and_edit_weight_attribute(self, attrs: Optional[List[str]] = None, **kwargs) -> 'QuantizationConfigOptions':


I don't remember if we discussed this, but this function and clone_and_map_weights_attr_keys don't seem like something that should be part of the schema, but rather an external functionality (although it is probably not possible because of the immutability? is that the problem?)

If it stays, in any case it makes more sense that OpQuantizationConfig updates its attributes. This class can call configs methods, but it shouldn't go inside and update them itself.

ofirgo · 2024-12-25T14:47:33Z

tests/common_tests/helpers/generate_test_tp_model.py

@@ -39,7 +39,7 @@ def generate_test_tp_model(edit_params_dict, name=""):
    base_config, op_cfg_list, default_config = get_op_quantization_configs()

    # separate weights attribute parameters from the requested param to edit
-    weights_params_names = [name for name in schema.AttributeQuantizationConfig.__init__.__code__.co_varnames if
+    weights_params_names = [name for name in schema.AttributeQuantizationConfig.model_fields.keys() if


Can we maybe create a property within the class to return this? because .model_fields.keys() is kind of cryptic

ofirgo · 2024-12-25T14:50:12Z

tests/keras_tests/feature_networks_tests/feature_networks/manual_bit_selection.py

@@ -165,8 +165,8 @@ def get_tpc(self):
        quantization_configurations.extend([
            tpc.layer2qco[tf.multiply].base_config.clone_and_edit(activation_n_bits=4),
            tpc.layer2qco[tf.multiply].base_config.clone_and_edit(activation_n_bits=2)])
-        tpc.layer2qco[tf.multiply] = replace(tpc.layer2qco[tf.multiply], base_config=base_config,
-                                             quantization_configurations=tuple(quantization_configurations))
+        tpc.layer2qco[tf.multiply] = tpc.layer2qco[tf.multiply].model_copy(


what's model_copy? is it a pydantic method? is it replacing the clone_and_edit ins some way?

it is a method to copy the instance with the new attributes (since the original instance is immutable)

irenaby

Only looked at v1.py. And sorry :)

irenaby · 2024-12-25T13:25:34Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-        if self.lut_values_bitwidth is not None and not isinstance(self.lut_values_bitwidth, int):
-            Logger.critical("lut_values_bitwidth must be an integer or None.") # pragma: no cover
+    @field_validator("weights_n_bits")
+    def validate_weights_n_bits(cls, value):


Do you intentionally not use PositiveInt in field definition?

irenaby · 2024-12-25T13:27:34Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+        return value
+
+    @field_validator("lut_values_bitwidth", mode="before")
+    def validate_lut_values_bitwidth(cls, value):


Isn't it validated automatically?

irenaby · 2024-12-25T15:25:27Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-        elif not isinstance(self.supported_input_activation_n_bits, tuple):
+
+        # When loading from JSON, lists are returned. If the value is a list, convert it to a tuple.
+        if isinstance(v, list):


I think the following flow is would be clearer (at least switch int and list order)

if isinstance(v, int): return (v,) # or v = (v,) if isinstance(v, list): v = tuple(v) validate v return v

irenaby · 2024-12-25T15:32:27Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+    # Pydantic v2 configuration for immutability
+    model_config = ConfigDict(frozen=True)
+
+    @model_validator(mode='before')


Did you consider using "after" or model_post_init? This way pydantic will do the initial validations for you and you can also access the fields via self.

irenaby · 2024-12-25T15:34:31Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-        if not isinstance(self.quantization_configurations, tuple):
+        quantization_configurations = values.get('quantization_configurations', ())
+        num_configs = len(quantization_configurations)
+        base_config = values.get('base_config')


Same as Ofir's comment, shouldn't base_config exist? If it's None that's fine, but it should be its value, not get's default

irenaby · 2024-12-25T17:07:06Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+            )  # pragma: no cover
+
+        # Validate that there are at least two operator groups
+        if len(fusing.operator_groups) < 2:


Field(min_length=2)

irenaby · 2024-12-25T17:07:38Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-        # Validate the operator_groups
-        if not isinstance(self.operator_groups, tuple):
+        # Validate operator_groups is a tuple
+        if not isinstance(fusing.operator_groups, tuple):


isnt it validated automatically?

irenaby · 2024-12-25T17:08:57Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-        operator_set (Tuple[OperatorsSetBase]): Tuple of operator sets within the model.
-        fusing_patterns (Tuple[Fusing]): Tuple of fusing patterns for the model.
+        operator_set (Tuple[OperatorsSetBase, ...]): Tuple of operator sets within the model.
+        fusing_patterns (Tuple[Fusing, ...]): Tuple of fusing patterns for the model.
        is_simd_padding (bool): Indicates if SIMD padding is applied.
        SCHEMA_VERSION (int): Version of the schema for the Target Platform Model.
    """
    default_qco: QuantizationConfigOptions


please remove default values, unless there is a good reason

irenaby · 2024-12-25T17:09:33Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

        """
        # Validate `default_qco`
-        if not isinstance(self.default_qco, QuantizationConfigOptions):
+        default_qco = tp_model.default_qco
+        if not isinstance(default_qco, QuantizationConfigOptions):


isnt is validated automatically?

irenaby · 2024-12-25T17:10:17Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

            Logger.critical("'default_qco' must be an instance of QuantizationConfigOptions.") # pragma: no cover
-        if len(self.default_qco.quantization_configurations) != 1:
+
+        if len(default_qco.quantization_configurations) != 1:


shouldnt it be checked at QuantizationConfigOptions level?

this is the default qco so it should have only one option. a regular qco can have multiple options

irenaby · 2024-12-25T17:19:47Z

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+            opsets_names = [
+                op.name.value if isinstance(op, OperatorSetNames) else op.name
+                for op in operator_set
+            ] if operator_set else []


you dont need if else []. if will already be an empty list

Replace MaxTensor with MaxCut for activation mixed precision (Experimental).

… tensorflow <=2.13) Fix PR comments

Refactor Target Platform Capabilities - Phase 4

ca7c104

Replace schema classes from dataclass to pydantic 'BaseModel'. Fix tests to support pydantic schema classes. Add test for exporting tp model to json

lior-dikstein requested review from ofirgo and irenaby December 25, 2024 13:00

github-actions bot added auto:target_platform_capabilities auto:tests labels Dec 25, 2024

Add pydantic to requirements file

20fa29f

ofirgo requested changes Dec 25, 2024

View reviewed changes

irenaby reviewed Dec 25, 2024

View reviewed changes

elad-c and others added 4 commits December 31, 2024 15:30

Replace max tensor with max cut (#1295)

5502435

Replace MaxTensor with MaxCut for activation mixed precision (Experimental).

Fix bug (#1303)

a5f1899

Move splitting ops from default to qpreserving configs in TPCv4. (#1304)

8863eec

Downgrade to pydantic v1.10 (since pydantic v2 is not compatible with…

6dfd044

… tensorflow <=2.13) Fix PR comments

github-actions bot added the auto:core label Dec 31, 2024

liord added 2 commits December 31, 2024 15:45

Downgrade to pydantic v1.10 (since pydantic v2 is not compatible with…

2eba10e

… tensorflow <=2.13) Fix PR comments

merge to main

e89e1e8

github-actions bot removed the auto:core label Dec 31, 2024

Remove imports from test

cd0de86

ofirgo approved these changes Dec 31, 2024

View reviewed changes

liord added 3 commits December 31, 2024 16:47

Changed pydantic version

9715e6f

Changed pydantic version

eb60dc2

Changed pydantic version

bc26fc5

lior-dikstein merged commit afed6e3 into main Dec 31, 2024
42 checks passed

lior-dikstein deleted the tpc_refactor_phase4 branch December 31, 2024 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Target Platform Capabilities - Phase 4 #1301

Refactor Target Platform Capabilities - Phase 4 #1301

lior-dikstein commented Dec 25, 2024 •

edited

Loading

ofirgo Dec 25, 2024

irenaby Dec 25, 2024

ofirgo Dec 25, 2024

ofirgo Dec 25, 2024

lior-dikstein Dec 31, 2024

irenaby left a comment

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

irenaby Dec 25, 2024

lior-dikstein Dec 31, 2024

irenaby Dec 25, 2024

Refactor Target Platform Capabilities - Phase 4 #1301

Refactor Target Platform Capabilities - Phase 4 #1301

Conversation

lior-dikstein commented Dec 25, 2024 • edited Loading

Pull Request Description:

Checklist before requesting a review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irenaby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lior-dikstein commented Dec 25, 2024 •

edited

Loading