add resource utilization calculator #1315

irenaby · 2025-01-06T18:21:21Z

Pull Request Description:

Add resource utilization calculator.
Update resource utilization data facade to use the resource utilization calculator.
Update mixed precision search manager to use the resource utilization calculator. Remove ru functions mapping.
Update core runner to use the resource utilization calculator to compute the final utilization.

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

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

ofirgo · 2025-01-08T13:21:30Z

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

+        elif target_criterion == TargetInclusionCriterion.AnyQuantized:
+            nodes = [n for n in self.graph if n.has_any_weight_attr_to_quantize()]
+        elif target_criterion == TargetInclusionCriterion.QNonConfigurable:
+            # TODO this is wrong. Need to look at specific weights and not the whole node


What is wrong? (add details to the comment if you are not planning on taking care of it in this PR, so we understand what is the problem when we come to fix it)

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

ofirgo · 2025-01-08T13:30:55Z

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

+                continue
+            for n in cut_target_nodes:
+                qc = act_qcs.get(n) if act_qcs else None
+                util_per_cut_per_node[cut][n] = self.compute_node_activation_tensor_utilization(n, target_criterion,


I didn't get into this, but @elad-c you should take a careful look here to understand that the "target_criterion" here enforces the behavior that we agreed on for what memory elements to include in a cut's "memory estimation" computation.
Maybe we should even raise an exception if the target_criterion for activation (i.e., max cut) is not aligned with what we agreed on for the current behavior (TargetInclusionCriterion.AnyQuantized if I understand the flow of the code correctly)

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

elad-c

didn't comment in all places, but in general: fix or explain the combination of an argument which is a must, but type hint says Optional

elad-c · 2025-01-08T08:06:55Z

model_compression_toolkit/core/common/graph/base_node.py

+        """ Checks whether the specific weight has a configurable quantization. """
+        return self.is_weights_quantization_enabled(attr_name) and not self.is_all_weights_candidates_equal(attr_name)
+
+    def has_configurable_activation(self):


add return type hint
it has no parameters, so maybe switch to property

Added type hint.
I don't think the lack of arguments automatically qualifies as property. Property "pretends" to be a field, this is more logic than a field.

elad-c · 2025-01-08T11:26:45Z

...ssion_toolkit/core/common/mixed_precision/resource_utilization_tools/resource_utilization.py

-               ru.activation_memory <= self.activation_memory and \
-               ru.total_memory <= self.total_memory and \
-               ru.bops <= self.bops
+    def __repr__(self):


doesn't the dataclass autogenerate the repr method?

Yes, but it looks like a code (like it should be). Here it is used to print the summary, so I kept it identical.

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

elad-c · 2025-01-08T13:15:18Z

...it/core/common/mixed_precision/resource_utilization_tools/resource_utilization_calculator.py

+    def compute_weights_utilization(self,
+                                    target_criterion: TargetInclusionCriterion,
+                                    bitwidth_mode: BitwidthMode,
+                                    w_qcs: Optional[Dict[BaseNode, NodeWeightsQuantizationConfig]] = None) \


if must be provided, then why is it optional? Also, better add validation for it

It is optional. It is only used for the custom bitwidth mode. In custom mode, it must be provided for all configurable weights. For non-configurable weights it can be retrieved from the node, if not passed. I'll try to make it clearer.

elad-c · 2025-01-08T14:12:55Z

..._toolkit/core/common/mixed_precision/resource_utilization_tools/resource_utilization_data.py

+                                                 mixed_precision_enable=False,
+                                                 running_gptq=False)
+
+    ru_calculator = ResourceUtilizationCalculator(transformed_graph, fw_impl, fw_info)


Can we create a single RUCalculator, so we don't have to calculate everything twice?

In another PR. I'll add TODO

@irenaby please add this (and other RU-related open tasks we mentioned during the work on this one that we postponed to next PR) to the clickup task

tests_pytest/core/__init__.py

tests_pytest/core/common/__init__.py

tests_pytest/core/common/mixed_precision/__init__.py

tests_pytest/core/common/mixed_precision/resource_utilization_tools/__init__.py

ofirgo

Completed reviewing the code itself (still need to go over the tests but don't wait for me).
Looks very good! most of the comments are regarding documentation and usability, nothing major.

...ssion_toolkit/core/common/mixed_precision/resource_utilization_tools/resource_utilization.py

ofirgo · 2025-01-09T10:22:46Z

model_compression_toolkit/core/common/mixed_precision/resource_utilization_tools/ru_methods.py

+    NodeActivationQuantizationConfig
+
+
+# TODO take into account Virtual nodes. Are candidates defined with respect to virtual or original nodes?


regarding the first question - the candidates of a virtual node are a combination of the candidates of the nodes it is composed of, from the original graph.

regarding the second question - I think we can separate it like this. Anyway, we can dive into this when re-implementing the BOPs metric, so keep the TODO and we'll address it in the future

model_compression_toolkit/core/common/mixed_precision/resource_utilization_tools/ru_methods.py

model_compression_toolkit/core/common/mixed_precision/search_methods/linear_programming.py

model_compression_toolkit/core/runner.py

github-actions bot added auto:core auto:tests labels Jan 6, 2025

irenaby added 2 commits January 6, 2025 20:49

add resource utilization calculator

9817e53

fix output tensors size calculation

e009eac

irenaby force-pushed the ru_refactor branch from c2dcbe1 to e009eac Compare January 6, 2025 19:20

irenaby added 5 commits January 6, 2025 21:36

fix test common failures

45d22c7

topo order for total util

0954f11

return total util in topo order

872cb46

fix total ru calculation

e4babed

use same instance of ru calculator in refine final config

a3073db

irenaby requested review from ofirgo and elad-c January 7, 2025 17:01

irenaby added 5 commits January 7, 2025 19:13

update docstrong

627fef0

fix lp search test

5e7022a

dont compute cuts if no graph nodes meet the target criterion

72cadff

reduce multi head attention test runtime

1254e6f

improve coverage

6023a68

irenaby force-pushed the ru_refactor branch from 3247949 to 6023a68 Compare January 8, 2025 12:23

ofirgo reviewed Jan 8, 2025

View reviewed changes

elad-c reviewed Jan 8, 2025

View reviewed changes

ofirgo requested changes Jan 9, 2025

View reviewed changes

ofirgo reviewed Jan 9, 2025

View reviewed changes

model_compression_toolkit/core/runner.py Outdated Show resolved Hide resolved

irenaby added 5 commits January 12, 2025 09:53

update Utilization

8251863

small fixes

1036c10

fixes per cide review

ae3136b

allow custom qc with disabled quantization

a38034d

remove configurable aggregation function

488a285

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add resource utilization calculator #1315

add resource utilization calculator #1315

irenaby commented Jan 6, 2025 •

edited

Loading

ofirgo Jan 8, 2025

ofirgo Jan 8, 2025

elad-c left a comment

elad-c Jan 8, 2025

irenaby Jan 12, 2025

elad-c Jan 8, 2025

irenaby Jan 8, 2025

elad-c Jan 8, 2025

irenaby Jan 8, 2025

elad-c Jan 8, 2025

irenaby Jan 8, 2025

ofirgo Jan 9, 2025

ofirgo left a comment

ofirgo Jan 9, 2025

		NodeActivationQuantizationConfig


		# TODO take into account Virtual nodes. Are candidates defined with respect to virtual or original nodes?

add resource utilization calculator #1315

Are you sure you want to change the base?

add resource utilization calculator #1315

Conversation

irenaby commented Jan 6, 2025 • edited Loading

Pull Request Description:

Checklist before requesting a review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elad-c 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

ofirgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irenaby commented Jan 6, 2025 •

edited

Loading