[Fixed]: Issues resolved raised by mypy for issue Make #22313 #2438
Conversation
…tensor_statistics"
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop openvinotoolkit/openvino#2438 +/- ##
===========================================
- Coverage 90.55% 3.88% -86.68%
===========================================
Files 493 498 +5
Lines 44994 45519 +525
===========================================
- Hits 40746 1767 -38979
- Misses 4248 43752 +39504
... and 441 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -140,6 +140,7 @@ def __init__(self, target_type: TargetType): | |||
| :param target_type: Type of the target point. | |||
| """ | |||
| self._target_type = target_type | |||
| self.target_node_name: str = "" | |||
There was a problem hiding this comment.
target_node_name parameter is valid for all inherited classes except TFTargetPoint. I see three possible solutions here:
- Do not inheret TFTargetPoint from the TargetPoint class and implement
target_node_nameattribute as a required init attribute. - Make an interface for a target points classes with
target_node_nameattribute and use it in correspondent typehints - Rename
layer_nameandop_nametotarget_node_namein the TFTargetPoints.
I suggest to implement the third solution
CC: @alexsu52
| @@ -50,21 +49,15 @@ def collect_statistics(self, model: TModel, graph: NNCFGraph) -> None: | |||
|
|
|||
| merged_statistics = self._get_merged_statistic_points(self.statistic_points, model, graph) | |||
| transformation_layout = self._get_transformation_layout_extra_outputs(merged_statistics) | |||
| model_with_outputs = model_transformer.transform(transformation_layout) | |||
| model_with_outputs: ModelTransformer = model_transformer.transform(transformation_layout) | |||
There was a problem hiding this comment.
| model_with_outputs: ModelTransformer = model_transformer.transform(transformation_layout) | |
| model_with_outputs: TModel = model_transformer.transform(transformation_layout) |
| for ( | ||
| _, | ||
| tensor_collectors, | ||
| ) in _statistic_point.algorithm_to_tensor_collectors.items(): |
There was a problem hiding this comment.
| for ( | |
| _, | |
| tensor_collectors, | |
| ) in _statistic_point.algorithm_to_tensor_collectors.items(): | |
| for tensor_collectors in _statistic_point.algorithm_to_tensor_collectors.values(): |
| dataset_length = self.dataset.get_length() | ||
| total = ( | ||
| min(dataset_length or self.stat_subset_size, self.stat_subset_size) | ||
| if self.stat_subset_size is not None | ||
| else None | ||
| ) |
There was a problem hiding this comment.
There was logic before: if self.stat_subset_size is None then subset size is equal to the length of the whole dataset. As default value is 0 for self.stat_subset_size and this check is gone, I don't see the place where this logic is implemented. Could you please explain where is it?
There was a problem hiding this comment.
The logic to set self.stat_subset_size to the length of the whole dataset if it is None is not explicitly present. However, there are steps taken during the registration of statistic points that indirectly handle the calculation of self.stat_subset_size.
In this part of the code, it iterates over the registered statistic points and their associated tensor collectors. For each tensor collector, it checks if self.stat_subset_size is None. If it is None, it sets self.stat_subset_size to the num_samples attribute of the tensor collector.
If self.stat_subset_size is not None, it compares it with the num_samples attribute of the tensor collector and updates self.stat_subset_size to be the maximum of the two values.
This logic effectively sets self.stat_subset_size based on the maximum number of samples encountered among all tensor collectors associated with the registered statistic points. While it doesn't directly set it to the length of the whole dataset if self.stat_subset_size is None, it achieves a similar effect by considering the maximum number of samples encountered during registration.
There was a problem hiding this comment.
Got you. But despite the fact that this should work in every case, I prefer explicit double check
|
|
||
|
|
||
| class TensorStatisticCollectorBase(ABC): | ||
| """Collector estimate statistics at the quantization point based on the provided reduction shape.""" | ||
|
|
||
| def __init__(self, reduction_shape: Optional[ReductionAxes] = None, num_samples: Optional[int] = None): | ||
| def __init__(self, reduction_shape: ReductionAxes = None, num_samples: Optional[int] = None): |
There was a problem hiding this comment.
Why Optional gone?
| total=total, | ||
| description="Statistics collection", | ||
| ): | ||
| data_iterable = iter([self.dataset.get_inference_data()]) |
There was a problem hiding this comment.
Please keep track call over here
| for input_data in data_iterable: | ||
| outputs = engine.infer(input_data) | ||
| processed_outputs = self._process_outputs(outputs) | ||
| self._register_statistics(processed_outputs, merged_statistics) |
| for tensor_collectors in _statistic_point.algorithm_to_tensor_collectors.values(): | ||
| for tensor_collector in tensor_collectors: |
There was a problem hiding this comment.
Is this a real issue?
| return self._num_samples | ||
|
|
||
| def register_input(self, x: TensorType) -> TensorType: | ||
| def register_input(self, x: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
torch.Tensor in common code, common code should use only common types
| @@ -266,7 +298,7 @@ def squeeze(x: NNCFTensor, dim: Optional[Union[int, Tuple[int, ...]]] = None) -> | |||
|
|
|||
| @staticmethod | |||
| @abstractmethod | |||
| def sum(tensor: NNCFTensor) -> TensorElementsType: | |||
| def sum(tensor: TensorElementsType) -> TensorElementsType: | |||
There was a problem hiding this comment.
| def sum(tensor: TensorElementsType) -> TensorElementsType: | |
| def sum(tensor: NNCFTensor) -> TensorElementsType: |
| def _register_input_common(self, x: NNCFTensor) -> None: | ||
| if self._tensor_processor is not None: | ||
| min_reduced: None = self._tensor_processor.reduce_min(x, self._reduction_shape) | ||
| if self._use_abs_max and self._tensor_processor is not None: | ||
| x = self._tensor_processor.abs(x) | ||
| max_reduced = self._tensor_processor.reduce_max(x, self._reduction_shape) | ||
| if self._tensor_processor is not None: | ||
| max_reduced: None = self._tensor_processor.reduce_max(x, self._reduction_shape) |
There was a problem hiding this comment.
Please refactor it. And please remove None typehint
| def _register_input_common(self, x: NNCFTensor) -> None: | ||
| if self._tensor_processor is not None: | ||
| min_reduced: int = self._tensor_processor.reduce_min(x, self._reduction_shape) | ||
| if self._use_abs_max and self._tensor_processor is not None: | ||
| x = self._tensor_processor.abs(x) | ||
| max_reduced = self._tensor_processor.reduce_max(x, self._reduction_shape) | ||
| if self._tensor_processor is not None: | ||
| max_reduced: int = self._tensor_processor.reduce_max(x, self._reduction_shape) | ||
|
|
||
| if self._use_per_sample_stats: | ||
| if self._use_per_sample_stats and self._tensor_processor is not None: |
There was a problem hiding this comment.
Please refactor it. And please remove None typehint
| if self._tensor_processor is not None: | ||
| stacked_min = self._tensor_processor.stack(self._all_min_values) | ||
| if self._use_means_of_mins and self._tensor_processor is not None: | ||
| return self._tensor_processor.mean(stacked_min, axis=0) | ||
| return self._tensor_processor.reduce_min(stacked_min, axis=0) | ||
| if self._tensor_processor is not None: | ||
| return self._tensor_processor.reduce_min(stacked_min, axis=0) | ||
|
|
||
| def _max_aggregate(self): | ||
| stacked_max = self._tensor_processor.stack(self._all_max_values) | ||
| if self._use_means_of_maxs: | ||
| def _max_aggregate(self) -> None: | ||
| if self._tensor_processor is not None: | ||
| stacked_max = self._tensor_processor.stack(self._all_max_values) | ||
| if self._use_means_of_maxs and self._tensor_processor is not None: | ||
| return self._tensor_processor.mean(stacked_max, axis=0) | ||
| return self._tensor_processor.reduce_max(stacked_max, axis=0) | ||
| if self._tensor_processor is not None: | ||
| return self._tensor_processor.reduce_max(stacked_max, axis=0) |
There was a problem hiding this comment.
Please refactor it. And please remove None typehint
| if self._tensor_processor is not None: | ||
| stacked_min = self._tensor_processor.stack(self._all_min_values) | ||
| if self._tensor_processor is not None: | ||
| return self._tensor_processor.mean(stacked_min, axis=0) |
There was a problem hiding this comment.
I think this if self._tensor_processor is not None: should be done in a different place
|
hey @AtharvaPore01 |
|
@mlukasze Hi, Sorry for the delay from my side. I appreciate your patience. I'm currently tied up with some urgent tasks but will prioritize completing the PR as soon as possible. Your understanding is greatly appreciated. |
|
@AtharvaPore01 , do you plan to continue to work on this PR? |
Changes
Missing Type Annotations:
Incorrect Function Signatures:
List and Array Operations:
Missing Return Statements:
Missing Type Annotations
Function is missing a type annotation for one or more arguments [no-untyped-def].Incorrect or Missing Implementation of
__eq__Method:__eq__method in different classes. The method's signature must accept an argument of typeobjectand then check if the passed object is of the expected type.Type Argument Issues in Generic Types:
Missing type parameters for generic typesuggest that you are using generic types (likendarray,deque,List) without specifying their type parameters.Incompatible Types and Assignments:
Mypy-specific Issues and Recommendations:
Miscellaneous Errors:
-> Noneshould be used), the use of a function as a type that is invalid, and a method in a subclass that does not return a value, contradicting the superclass's method signature.Other Issues:
Reason for changes
As stated in issue #22313, the main reason behind solving the errors above as they were raised by running the mypy.ini file.
Ticket Number:
#22313