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.

Sign up for GitHub

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

Jump to bottom

Refactor Target Platform Capabilities - Phase 2 #1290

Merged

lior-dikstein merged 8 commits into main from tpc_refator_phase2

Dec 12, 2024

Collaborator

lior-dikstein commented Dec 10, 2024 •

edited

Loading

Convert all schema classes to immutable dataclasses, replacing existing methods with equivalent dataclass methods (e.g., replace).
Ensure all schema classes are strictly immutable to enhance reliability and maintain consistency.
Update target platform model versions to align with the new class structure.
Refactor tests to support and validate the updated class types and functionality.

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).


          **Refactor Target Platform Capabilities - Phase 2**

951568c

- Convert all schema classes to immutable dataclasses, replacing existing methods with equivalent dataclass methods (e.g., `replace`).
- Ensure all schema classes are strictly immutable to enhance reliability and maintain consistency.
- Update target platform model versions to align with the new class structure.
- Refactor tests to support and validate the updated class types and functionality.

lior-dikstein requested review from ofirgo and irenaby

December 10, 2024 09:01

github-actions bot added auto:target_platform_capabilities auto:tests labels

lior-dikstein changed the title ~~**Refactor Target Platform Capabilities - Phase 2**~~ Refactor Target Platform Capabilities - Phase 2

liord added 3 commits

December 10, 2024 12:32


          Add "# pragma: no cover" to Logger critical errors

f183073


          Fix OperatorSetNames Enum

eb5bbb2


          Add show function to tp model

52b00d0

irenaby reviewed

View reviewed changes

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-                  def __init__(self,
-                               quantization_config_list: List[OpQuantizationConfig],
-                               base_config: OpQuantizationConfig = None):
+                  quantization_config_list: List[OpQuantizationConfig]

Collaborator

irenaby Dec 10, 2024

Do we want to convert lists into tuples to be really frozen? List elements can still be updated.

Collaborator Author

lior-dikstein Dec 11, 2024

I agree. This will be removed in the next PR when the context manager mechanism is removed.

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-                               add_metadata: bool = True,
-                               name="default_tp_model"):
+                  default_qco: QuantizationConfigOptions
+                  tpc_minor_version: Optional[int]

Collaborator

irenaby Dec 10, 2024

Should we really allow None in version and platform type? Also consider unifying version into a single field (unless it was an explicit requirement for some reason)

Collaborator

ofirgo Dec 11, 2024

versions are separated feildes by design.
regarding the None value, I would like to have @haihabi 's opinion

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+                  tpc_minor_version: Optional[int]
+                  tpc_patch_version: Optional[int]
+                  tpc_platform_type: Optional[str]
+                  add_metadata: bool = True

Collaborator

irenaby Dec 10, 2024

Is add_metadata really relevant to tpc?

Collaborator Author

lior-dikstein Dec 11, 2024

I discussed this with @ofirgo and @haihabi and we decided to keep it here

model_compression_toolkit/target_platform_capabilities/schema/v1.py

+                  tpc_platform_type: Optional[str]
+                  add_metadata: bool = True
+                  name: str = "default_tp_model"
+                  operator_set: List[OperatorsSetBase] = field(default_factory=list)

Collaborator

irenaby Dec 10, 2024

I strongly recommend removing the default values here and in other fields, except maybe the name (not sure what it is for). I don't think there exists any meaningful default, certainly not an empty set.

Collaborator

irenaby Dec 10, 2024

This also applies to other classes.

Collaborator Author

lior-dikstein Dec 11, 2024

I agree. This will be removed in the next PR when the context manager mechanism is removed.

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated

+                      if len(self.default_qco.quantization_config_list) != 1:
+                          Logger.critical("Default QuantizationConfigOptions must contain exactly one option.") # pragma: no cover
+                  def get_config_options_by_operators_set(self, operators_set_name: str) -> QuantizationConfigOptions:

Collaborator

irenaby Dec 10, 2024

Is there any reason that this and the following methods should be part of the schema? Maybe we can create MCT derived class as Hai suggested and add the utility methods it needs.

Collaborator

ofirgo Dec 11, 2024

either a utility sub-class or an external util function (similar to what I wrote regarding max_activation_n_bits function).
Either way, it shouldn't be part of the schema

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated

-                      return self
-                  def __validate_model(self):
+                  def __validate_model(self) -> None:

Collaborator

irenaby Dec 10, 2024

Why do you need both post_init and validate?

Collaborator Author

lior-dikstein Dec 11, 2024

I agree. Currently, this is part of the context manager. It will be removed in the next PR when the context manager mechanism is removed.

ofirgo requested changes

View reviewed changes

model_compression_toolkit/target_platform_capabilities/schema/v1.py Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated Show resolved Hide resolved

model_compression_toolkit/target_platform_capabilities/schema/v1.py

-                               add_metadata: bool = True,
-                               name="default_tp_model"):
+                  default_qco: QuantizationConfigOptions
+                  tpc_minor_version: Optional[int]

Collaborator

ofirgo Dec 11, 2024

versions are separated feildes by design.
regarding the None value, I would like to have @haihabi 's opinion

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated

+                      if len(self.default_qco.quantization_config_list) != 1:
+                          Logger.critical("Default QuantizationConfigOptions must contain exactly one option.") # pragma: no cover
+                  def get_config_options_by_operators_set(self, operators_set_name: str) -> QuantizationConfigOptions:

Collaborator

ofirgo Dec 11, 2024

either a utility sub-class or an external util function (similar to what I wrote regarding max_activation_n_bits function).
Either way, it shouldn't be part of the schema

model_compression_toolkit/target_platform_capabilities/schema/v1.py Outdated

-                  def append_component(self,
-                                       tp_model_component: TargetPlatformModelComponent):
+                  def append_component(self, tp_model_component: TargetPlatformModelComponent) -> None:

Collaborator

ofirgo Dec 11, 2024

Just to make sure, this function will be removed in the next PR that eliminates the "with" initialization mechanism, is that correct?

Collaborator Author

lior-dikstein Dec 11, 2024

I agree. This will be removed in the next PR when the context manager mechanism is removed.


          Remove functionality from schema to schema_functions

0e71a87

github-actions bot added the auto:core label

liord added 3 commits

December 11, 2024 12:58


          Fix typo

24ea028


          Delete unused function

276bacc


          Delete unused function

274e8e3

ofirgo approved these changes

View reviewed changes

lior-dikstein merged commit dbc93b7 into main

42 checks passed

lior-dikstein deleted the tpc_refator_phase2 branch

December 12, 2024 09:42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:core auto:target_platform_capabilities auto:tests