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

Make NNCF common utils code pass mypy checks #2780

Merged
merged 31 commits into from
Jul 5, 2024
Merged

Make NNCF common utils code pass mypy checks #2780

merged 31 commits into from
Jul 5, 2024

Conversation

anzr299
Copy link
Contributor

@anzr299 anzr299 commented Jul 2, 2024

Changes

Made all the mypy checks pass for nncf/common/utils

Related tickets

Closes Issue #2491

@anzr299 anzr299 requested a review from a team as a code owner July 2, 2024 13:57
@github-actions github-actions bot added NNCF TF Pull requests that updates NNCF TensorFlow NNCF Common Pull request that updates NNCF Common labels Jul 2, 2024
@daniil-lyakhov
Copy link
Collaborator

Mypy and precommit jobs are red, please deal with it

@anzr299
Copy link
Contributor Author

anzr299 commented Jul 3, 2024

I have fixed the issues, it seemed to be import related and I used tuple instead of Typing.tuple by mistake

@anzr299
Copy link
Contributor Author

anzr299 commented Jul 3, 2024

I will explore the failed tests and what went wrong and try to fix it.

@anzr299
Copy link
Contributor Author

anzr299 commented Jul 3, 2024

I believe the failing test is due to the fact that I added an extra line in nncf/common/utils/timer.py. This causes a failed test tests/common/test_timer.py because of the line assert "nncf:timer.py:28 Elapsed Time: 00:00:01" in nncf_caplog.text which expects it to be on line 28 but due to addition of an extra import line in nncf/common/utils/timer.py moves it to 29 as seen in the error output FAILED tests/common/utils/test_timer.py::test_timer - AssertionError: assert 'nncf:timer.py:28 Elapsed Time: 00:00:01' in 'INFO nncf:timer.py:29 Elapsed Time: 00:00:01\n'. Would you suggest I change the test to check on 29 instead of 28?

@daniil-lyakhov
Copy link
Collaborator

I believe the failing test is due to the fact that I added an extra line in nncf/common/utils/timer.py. This causes a failed test tests/common/test_timer.py because of the line assert "nncf:timer.py:28 Elapsed Time: 00:00:01" in nncf_caplog.text which expects it to be on line 28 but due to addition of an extra import line in nncf/common/utils/timer.py moves it to 29 as seen in the error output FAILED tests/common/utils/test_timer.py::test_timer - AssertionError: assert 'nncf:timer.py:28 Elapsed Time: 00:00:01' in 'INFO nncf:timer.py:29 Elapsed Time: 00:00:01\n'. Would you suggest I change the test to check on 29 instead of 28?

Please update the reference here. It is OK practice to update reference files when the new reference is checked manually and is confirmed to be correct

@anzr299
Copy link
Contributor Author

anzr299 commented Jul 4, 2024

Hi @daniil-lyakhov, can you please review the changes. I believe all the checks have passed.

Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

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

Please use typing.Any instead of object in typehints. Besides that the pr is ready to go

@anzr299
Copy link
Contributor Author

anzr299 commented Jul 4, 2024

Alright, I will replace all the object type hints to typing.Any? I mainly did that to avoid using typing.Any since I thought it would be too vague.

@daniil-lyakhov
Copy link
Collaborator

@AlexanderDokuchaev, please merge

@alexsu52 alexsu52 merged commit 28d99a0 into openvinotoolkit:develop Jul 5, 2024
12 checks passed
@anzr299 anzr299 deleted the mypy-utils branch July 5, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NNCF Common Pull request that updates NNCF Common NNCF TF Pull requests that updates NNCF TensorFlow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants