Make NNCF common accuracy aware training code pass mypy checks #2637

anzr299 · 2024-04-17T02:36:15Z

Changes:

mypy checks for accuracy_aware_training pass.

type: ignore is used in some places due to one of the following reasons:

Using Generic type causes a mismatch between some instances and their assignments which causes mypy to throw an error. Bounding does not solve the problem either.
Other issues related to the use of TypeVar such as bounding OptimizerType, LRSchedulerType, and TensorboardWriterType.
Error related to named arguments not being defined in self._train_epoch_fn
attribute not available

Extra Changes:

The argument type and return type of configure_accuracy_aware_paths were changed to include pathlib.Path.
**kwargs was removed from abstract method initialize_training_loop_fns due to mypy bug regarding a mismatch of the function type.

Reasons for changes:

Passing mypy checks in issue #2492

Related tickets:

Closes #2492

codecov · 2024-04-17T02:51:22Z

Codecov Report

Attention: Patch coverage is 96.77419% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 77.96%. Comparing base (fa1a4ce) to head (43ec062).
Report is 4 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #2637       +/-   ##
============================================
- Coverage    89.84%   77.96%   -11.89%     
============================================
  Files          494      493        -1     
  Lines        45374    45468       +94     
============================================
- Hits         40768    35450     -5318     
- Misses        4606    10018     +5412

Files	Coverage Δ
nncf/common/accuracy_aware_training/runner.py	`98.70% <100.00%> (+0.04%)`	⬆️
...f/common/accuracy_aware_training/runner_factory.py	`94.00% <100.00%> (ø)`
nncf/common/accuracy_aware_training/statistics.py	`100.00% <100.00%> (ø)`
nncf/common/utils/helpers.py	`96.00% <100.00%> (+0.16%)`	⬆️
...cf/common/accuracy_aware_training/training_loop.py	`88.93% <90.69%> (+0.57%)`	⬆️

... and 109 files with indirect coverage changes

Flag	Coverage Δ
ONNX	`?`
OPENVINO	`?`
TENSORFLOW	`30.11% <93.54%> (-0.02%)`	⬇️
TORCH	`66.00% <96.77%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
common	`88.31% <96.77%> (-1.23%)`	⬇️
torch	`93.58% <ø> (+0.09%)`	⬆️
tensorflow	`93.74% <ø> (-0.01%)`	⬇️
onnx	`0.00% <ø> (-93.08%)`	⬇️
openvino	`25.67% <ø> (-68.50%)`	⬇️
ptq	`53.03% <ø> (-32.17%)`	⬇️

MaximProshin · 2024-04-18T07:01:46Z

@alexsu52 fyi

vshampor

I would recommend against using explicit cast(...) calls. For cases where you get(...) something from a Dict and assign it to a variable, it should be sufficient to add an inline type hint via var_name : type_name = ....get(...) syntax, since a plain Dict would be implicitly a Dict[Any, Any], and we have an implicit cast that mypy won't complain about.

Also, completely decoupling the new Image typevar from PIL.Image may not be the best solution, see if you can implement one of the variants from this suggestion instead:
https://stackoverflow.com/questions/61384752/how-to-type-hint-with-an-optional-import

anzr299 · 2024-04-18T13:27:23Z

I would recommend against using explicit cast(...) calls. For cases where you get(...) something from a Dict and assign it to a variable, it should be sufficient to add an inline type hint via var_name : type_name = ....get(...) syntax, since a plain Dict would be implicitly a Dict[Any, Any], and we have an implicit cast that mypy won't complain about.

Also, completely decoupling the new Image typevar from PIL.Image may not be the best solution, see if you can implement one of the variants from this suggestion instead: https://stackoverflow.com/questions/61384752/how-to-type-hint-with-an-optional-import

Sure, I will get on it and try to couple the Image TypeVar from the optional import. I will also iterate through every cast instance and adjust it as suggested.

anzr299 · 2024-04-18T15:48:07Z

@vshampor @alexsu52 I just realised that I had used cast for values returned from accuracy_aware_training_params since I was experimenting with an added object type to its initialization. But, forgot to remove them. These are removed now.
As for the accuracy_aware_training_params dictionary, it is of the type Dict[str, Union[float, int]] and so this causes an error in
for epoch in range(1, self.runner.maximal_total_epochs + 1): loop which expects only an integer but the maximal_total_epochs in runner.py is of type int|float since it is a value of the accuracy_aware_training_params dictionary. When trying to use an inline maximal_total_epochs: int = ... it gives an error regarding Incompatible types in the assignment since an int | float is expected.
I have created another variable right before this loop and casted self.runner.maximal_total_epochs before assigning the value to ensure the use of cast is limited.

As for the remaining use of cast, it was necessary due to the values being
In line 294 of runner.py: statistics = cast(NNCFStatistics, compression_controller.statistics())
cast is used because compression_controller.statistics() returns of the type statistics whereas,
prepare_for_tensorboard(statistics) expects of the type NNCFStatistics.

For the Image Type Variable, I have searched online for optional imports and there seems to be no proper way to do it or they don't work. For this, I have initialized an Image type with Any before the imports which was later used to bound the ImageType

…lue` method.

… history dictionary keys and values.

anzr299 · 2024-04-19T19:56:46Z

@alexsu52 @vshampor This is ready for review. All the mypy checks passed and I have made the required changes.

vshampor · 2024-04-23T14:36:30Z

Looks like some of the backend-specific unit tests are failing:

PT:

Test Result (13 failures / +13)
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop[max_accuracy_degradation1-0.92-0.0]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop[max_accuracy_degradation0-0.745-0.998181]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop[max_accuracy_degradation1-0.92-0.0]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop[max_accuracy_degradation2-0.82-0.9151]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_with_no_training[max_accuracy_degradation0-1-0.01]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_failing[max_accuracy_degradation0-4-0.1]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_failing[max_accuracy_degradation1-4-0.2]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_failing[max_accuracy_degradation2-10-0.3]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_failing[max_accuracy_degradation3-4-0.1]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_failing[max_accuracy_degradation4-4-0.3]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_too_high_pruning_flops[0.05-0.1]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_too_high_pruning_flops[0.1-0.2]
accuracy_aware_training.test_training_loop.test_adaptive_compression_training_loop_too_high_pruning_flops[0.4-0.6]
accuracy_aware_training.test_training_loop.test_early_exit_with_mock_validation_and_no_improvement[max_accuracy_degradation0]

TF

accuracy_aware_training.test_keras_api.test_adaptive_compression_training_loop[max_accuracy_degradation0--0.0923-0.04942]
accuracy_aware_training.test_keras_api.test_adaptive_compression_training_loop[max_accuracy_degradation1--0.0923-0.04942]
accuracy_aware_training.test_keras_api.test_adaptive_compression_training_loop[max_accuracy_degradation2--0.0923-0.04942]
accuracy_aware_training.test_keras_api.test_adaptive_compression_training_loop[max_accuracy_degradation3-0.2077-0.09749]
test_sanity_sample.test_model_accuracy_aware_train[_accuracy_aware_config0]

Please run the associated tests locally (pytest tests/torch -k accuracy_aware and pytest tests/tensorflow -k accuracy_aware from repo root for PT and TF parts respectively) and verify that you can reproduce the failures above. It seems like all of the failures are either:
AttributeError: 'TFAdaptiveCompressionLevelTrainingRunner' object has no attribute '_compression_rate_target' or AttributeError: 'PTAdaptiveCompressionLevelTrainingRunner' object has no attribute '_compression_rate_target' when the following field accesses take place:

 @property
 def compression_rate_target(self) -> float:
      if self._compression_rate_target is None:
            ...

anzr299 · 2024-04-23T15:30:11Z

Sure, I will look into it and get back to you. Hopefully by Saturday, since I am busy with my end of semester examinations.
I think the issue is mainly because of not initializing some variables like it was before. I will check them individually and re run the tests locally.

anzr299 · 2024-04-24T17:55:56Z

@vshampor I believe the pre-commit issues have been resolved. Can you please run the mypy tests and review it?

anzr299 and others added 2 commits April 17, 2024 06:19

Commit

d2478b4

Merge branch 'openvinotoolkit:develop' into develop

d243701

anzr299 requested a review from a team as a code owner April 17, 2024 02:36

github-actions bot added the NNCF Common Pull request that updates NNCF Common label Apr 17, 2024

anzr299 mentioned this pull request Apr 17, 2024

[Good First Issue] [NNCF] Make NNCF common accuracy aware training code pass mypy checks #2492

Closed

Test to check Union

80f91e7

openvino-nncf-ci added the API Public API-impacting changes label Apr 17, 2024

Changed all '|' to use Union[]

cf1bc87

anzr299 added 5 commits April 17, 2024 06:52

minor change

fcd80bc

Pre-commit checks pass

468f262

black and isort reformatted

15e0f05

test commit

940a603

checked pre commit again

b2c2945

anzr299 added 2 commits April 18, 2024 17:00

changing tuple to typing.Tuple

c97e402

Ignore matplotlib import error

6254b7f

vshampor reviewed Apr 18, 2024

View reviewed changes

change tuple to typing.Tuple

f74b857

Remove some casts, Add support for optional PIL Image Import

74fd9de

anzr299 requested a review from vshampor April 19, 2024 08:35

anzr299 mentioned this pull request Apr 19, 2024

[Good First Issue] [NNCF] Make NNCF common utils code pass mypy checks #2491

Closed

anzr299 and others added 4 commits April 19, 2024 14:31

Merge branch 'openvinotoolkit:develop' into develop

2d8d42e

Add keyword arguments support in `_determine_compression_rate_step_va…

e19b153

…lue` method.

Merge branch 'openvinotoolkit:develop' into develop

6bc4c7e

Change list() to use cast(List[]) when converting compressed training…

2d04ee0

… history dictionary keys and values.

Used cast(List[]) instead of list()

27a0175

Merge branch 'openvinotoolkit:develop' into develop

ea4e4f6

Fix issues with compression_rate_target

c2eb185

anzr299 added 2 commits April 24, 2024 19:37

Pytorch tests pass

8fef98e

Fix mypy errors regarding initialization

43ec062

vshampor approved these changes Apr 25, 2024

View reviewed changes

vshampor merged commit 17a5b65 into openvinotoolkit:develop Apr 25, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make NNCF common accuracy aware training code pass mypy checks #2637

Make NNCF common accuracy aware training code pass mypy checks #2637

anzr299 commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

MaximProshin commented Apr 18, 2024

vshampor left a comment

anzr299 commented Apr 18, 2024

anzr299 commented Apr 18, 2024 •

edited

Loading

anzr299 commented Apr 19, 2024 •

edited

Loading

vshampor commented Apr 23, 2024

anzr299 commented Apr 23, 2024 •

edited

Loading

anzr299 commented Apr 24, 2024

Make NNCF common accuracy aware training code pass mypy checks #2637

Make NNCF common accuracy aware training code pass mypy checks #2637

Conversation

anzr299 commented Apr 17, 2024 • edited Loading

Changes:

Reasons for changes:

Related tickets:

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

MaximProshin commented Apr 18, 2024

vshampor left a comment

Choose a reason for hiding this comment

anzr299 commented Apr 18, 2024

anzr299 commented Apr 18, 2024 • edited Loading

anzr299 commented Apr 19, 2024 • edited Loading

vshampor commented Apr 23, 2024

anzr299 commented Apr 23, 2024 • edited Loading

anzr299 commented Apr 24, 2024

anzr299 commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

anzr299 commented Apr 18, 2024 •

edited

Loading

anzr299 commented Apr 19, 2024 •

edited

Loading

anzr299 commented Apr 23, 2024 •

edited

Loading