Skip to content
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.

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

[NNCF] NNCF common accuracy aware training code pass mypy checks #2521

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

siddhant-0707
Copy link

Changes

Added type annotation to files in nncf/common/accuracy_aware_training.
Some checks still not passing mainly due to problem with class annotations.

Related tickets

#2492

Tests

mypy

@siddhant-0707 siddhant-0707 requested a review from a team as a code owner February 26, 2024 17:25
@github-actions github-actions bot added the NNCF Common Pull request that updates NNCF Common label Feb 26, 2024
@openvino-nncf-ci openvino-nncf-ci added the API Public API-impacting changes label Feb 26, 2024
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.22%. Comparing base (6b0ba7a) to head (e497a96).
Report is 19 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2521      +/-   ##
===========================================
- Coverage    90.91%   84.22%   -6.70%     
===========================================
  Files          498      498              
  Lines        45526    45540      +14     
===========================================
- Hits         41390    38354    -3036     
- Misses        4136     7186    +3050     
Files Coverage Δ
nncf/common/accuracy_aware_training/runner.py 81.77% <100.00%> (-16.89%) ⬇️
nncf/common/accuracy_aware_training/statistics.py 100.00% <100.00%> (ø)
...cf/common/accuracy_aware_training/training_loop.py 61.35% <100.00%> (-27.00%) ⬇️

... and 72 files with indirect coverage changes

Flag Coverage Δ
COMMON 42.72% <0.00%> (-0.01%) ⬇️
ONNX 34.78% <73.07%> (+<0.01%) ⬆️
OPENVINO ?
TENSORFLOW 29.21% <100.00%> (-0.46%) ⬇️
TORCH 65.71% <100.00%> (-0.28%) ⬇️

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

Components Coverage Δ
common 91.68% <100.00%> (-2.09%) ⬇️
torch 93.33% <ø> (-0.09%) ⬇️
tensorflow 92.87% <ø> (-1.09%) ⬇️
onnx 93.05% <ø> (ø)
openvino 25.58% <ø> (-68.48%) ⬇️
ptq 67.30% <ø> (-20.40%) ⬇️

@alexsu52 alexsu52 requested a review from vshampor February 27, 2024 03:42
@vshampor
Copy link
Contributor

@siddhant-0707 please extend the .mypy.ini file in the repo root in this PR to include the code path that you enabled with mypy so that our mypy precommit check can confirm that the files you changed indeed pass.

@@ -1,5 +1,5 @@
[mypy]
files = nncf/common/sparsity
files = nncf/common/accuracy_aware_training
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't replace the path, you should extend it.

@@ -127,8 +127,8 @@ def initialize_training_loop_fns(
validate_fn: Callable[[TModel, Optional[int]], float],
configure_optimizers_fn: Callable[[], Tuple[OptimizerType, LRSchedulerType]],
dump_checkpoint_fn: Callable[[TModel, CompressionAlgorithmController, "TrainingRunner", str], None],
**kwargs,
):
**kwargs: Any,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type hint is incorrect, review your general Python knowledge as to what type the **kwargs parameter is constrained to.

@mlukasze
Copy link

hey @siddhant-0707 will you have time to respond for changes request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Public API-impacting changes NNCF Common Pull request that updates NNCF Common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants