nncf icon indicating copy to clipboard operation
nncf copied to clipboard

[Good First Issue] [NNCF] Make NNCF common graph code pass mypy checks

Open vshampor opened this issue 1 year ago • 22 comments

Context

NNCF code is mostly typed with conventional type hints, but historically had started out and grown without a CI-enforced mypy conformance process in place. Since typing is paramount for software systems of any appreciable complexity, NNCF needs to take steps to attain, ideally, full mypy coverage of at least its non-test code base. The scope of this exact task, however, will be necessarily to start out small, as recommended by the mypy devs.

Task

Make the files under nncf/common/graph, which define the classes and logic for NNCF's main control flow graph abstraction, pass mypy without failures. Use the following .mypy.ini file:

[mypy]
files = nncf/common/graph
follow_imports = silent
strict = True

# should be removed later
# mypy recommends the following tool as an autofix:
# https://github.com/hauntsaninja/no_implicit_optional
implicit_optional = True

Create this file with the contents above in the repository root, install NNCF in editable mode by running pip install -e . from the repository root and then run mypy in command line.

This should produce the output similar to the following:

nncf/common/graph/utils.py:108: error: Argument 1 to "get_nodes_by_metatypes" of "NNCFGraph" has incompatible type "List[OperatorMetatype]"; expected "List[Type[OperatorMetatype]]"  [arg-type]
nncf/common/graph/utils.py:112: error: Non-overlapping container check (element type: "Type[OperatorMetatype]", container item type: "OperatorMetatype")  [comparison-overlap]
nncf/common/graph/patterns/manager.py:40: error: Returning Any from function declared to return "Dict[HWFusedPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:44: error: Returning Any from function declared to return "Dict[HWFusedPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:48: error: Returning Any from function declared to return "Dict[HWFusedPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:64: error: Returning Any from function declared to return "Dict[IgnoredPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:68: error: Returning Any from function declared to return "Dict[IgnoredPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:72: error: Returning Any from function declared to return "Dict[IgnoredPatternNames, Callable[[], GraphPattern]]"  [no-any-return]
nncf/common/graph/patterns/manager.py:112: error: Call to untyped function "Patterns" in typed context  [no-untyped-call]
nncf/common/graph/patterns/manager.py:131: error: Argument 1 to "_get_full_pattern_graph" of "PatternsManager" has incompatible type "Dict[HWFusedPatternNames, Callable[[], GraphPattern]]"; expected "Dict[Union[IgnoredPatternNames, HWFusedPatternNames], Callable[[], GraphPattern]]"  [arg-type]
nncf/common/graph/patterns/manager.py:131: error: Argument 3 to "_get_full_pattern_graph" of "PatternsManager" has incompatible type "Optional[ModelType]"; expected "ModelType"  [arg-type]
nncf/common/graph/patterns/manager.py:147: error: Argument 1 to "_get_full_pattern_graph" of "PatternsManager" has incompatible type "Dict[IgnoredPatternNames, Callable[[], GraphPattern]]"; expected "Dict[Union[IgnoredPatternNames, HWFusedPatternNames], Callable[[], GraphPattern]]"  [arg-type]
nncf/common/graph/patterns/manager.py:147: error: Argument 3 to "_get_full_pattern_graph" of "PatternsManager" has incompatible type "Optional[ModelType]"; expected "ModelType"  [arg-type]
Found 95 errors in 10 files (checked 15 source files)

Walk through the relevant code lines that produced each error and correct them by setting up missing type hints or correcting the existing ones. It should be possible to infer the required type hints from the function bodies and/or usages in most cases; resort to inline-disabling mypy for certain lines by # type:ignore only if setting up correct type hints requires massive refactoring.

Refer to https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html for basic type hinting information. Use type hints compatible with Python 3.8, e.g. List from the typing module instead of list etc.

vshampor avatar Jan 16 '24 20:01 vshampor

I wish to request to allocation for this issue.

It is in preperation for GSoC 2024 eligibility.

Grimoors avatar Jan 17 '24 16:01 Grimoors

.take

Grimoors avatar Jan 17 '24 16:01 Grimoors

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

github-actions[bot] avatar Jan 17 '24 16:01 github-actions[bot]

Hi @vshampor @rkazants @mlukasze !

I'm excited to address the mypy checks for the NNCF common graph code. Here's my plan:

Install Mypy: I'll ensure that Mypy is installed in our development environment to perform thorough type checking.

Configuration File: I'll create/update the mypy.ini configuration file in the project's root directory to tailor Mypy's settings to our specific needs.

Type Annotations: I'll review and enhance the NNCF common graph code by incorporating comprehensive type annotations. This includes annotating function signatures, variables, and return types.

Run Mypy: Once annotations are in place, I'll execute Mypy on the NNCF common graph code to identify any existing issues.

Addressing Issues: I'll methodically address any issues identified by Mypy, making necessary adjustments to the codebase to ensure consistency with the type annotations.

Iterative Testing: I'll perform iterative testing, running Mypy at each stage of development to catch and resolve issues promptly.

Feel free to provide additional insights or specific preferences you may have. I'm committed to delivering a clean and well-typed NNCF common graph code. Looking forward to your feedback. I am eagerly waiting to solve this ,could you please assign me?

prajak002 avatar Jan 18 '24 20:01 prajak002

@prajak002 this exact issue seems to have already been taken. I have created openvinotoolkit/nncf#2494 which is basically the same task, but for a different, mostly non-overlapping part of code - you can take that if you want.

vshampor avatar Jan 19 '24 10:01 vshampor

Thank you Sir! I'll take on openvinotoolkit/nncf#2494 for the non-overlapping part. Let me know if there are any specifics I should be aware of. Appreciate your collaboration.

prajak002 avatar Jan 19 '24 17:01 prajak002

Implementation Plan for NNCF Common Graph Code Mypy Compliance

Subject: Expedited Plan for Mypy Compliance in NNCF Common Graph Code

Introduction

I am enthusiastic about contributing to the NNCF project and am keen on participating in GSoC 2024 with your organization. Given my limited availability, I propose an expedited implementation plan to address the mypy compliance issue (#22196) within a week, starting January 20th.

Timezone and Availability

My timezone is IST (GMT+5:30). I can dedicate 1.5 hours daily to this project.

Objective

To make all files under nncf/common/graph compliant with mypy checks, in line with the .mypy.ini configuration. This task involves correcting type hint issues and ensuring Python 3.8 type hinting standards.

Expedited Implementation Steps and Timeline

  1. Setup and Initial Analysis (21st January)

Morning: Set up the local development environment. Evening: Install NNCF in editable mode and run initial mypy checks.

  1. Error Categorization and Prioritization (22nd January)

Categorize mypy errors and prioritize files based on error complexity and impact.

  1. Resolving Simple Type Hint Issues (23rd January)

Address simpler type hint errors across multiple files.

  1. Addressing Complex Type Hint Issues (24th and 25th January)

Tackle more complex type hint issues. Begin necessary refactoring for clearer type hints and code quality.

  1. Continued Refactoring and Code Quality (26th January)

Continue refactoring for improved type hint clarity. Ensure alignment with NNCF's coding standards.

  1. Documentation Update and Final Testing (27th January)

Morning: Update documentation and comments to reflect changes. Evening: Run final mypy checks and unit testing.

  1. Review and Pull Request Preparation (28th January)

Prepare the code for final review. Create and submit the pull request with a detailed description of changes.

  1. Buffer Period (29th - 30th January)

Use this time for any final adjustments or to address immediate feedback from the project maintainers.

Conclusion

This expedited plan is designed to efficiently address the mypy compliance issue in the NNCF common graph code, maintaining high standards of code quality within the constrained timeframe. I look forward to contributing to this project and gaining valuable experience for GSoC 2024.

For further discussion or clarifications regarding this implementation plan, please feel free to reach out.

Best regards, Vivek [email protected]

Grimoors avatar Jan 19 '24 21:01 Grimoors

@prajak002 sorry, looks like the openvinotoolkit/nncf#2494 was taken before you could react. I created openvinotoolkit/nncf#2497 and asked @rkazants to assign you directly this time, this is the same stuff but yet for another code path.

vshampor avatar Jan 22 '24 12:01 vshampor

@Grimoors feel free to take openvinotoolkit/nncf#2493 that I've just created for you.

vshampor avatar Jan 22 '24 12:01 vshampor

@Grimoors feel free to take openvinotoolkit/nncf#2493 that I've just created for you.

@vshampor ; As I am already halfway done with my implementation plan for the current issue openvinotoolkit/nncf#2495 ; may I just put my pull request and then move on to the next issue after that ? It appears some other person is assigned to openvinotoolkit/nncf#2493 regardless;

Please let me continue with work on the current issue.

Grimoors avatar Jan 24 '24 07:01 Grimoors

@Grimoors sorry, did not pay enough attention and mistaken you for someone else who wanted a part of this task. Sure, please continue with this one, we are eagerly waiting for the PR.

vshampor avatar Jan 24 '24 16:01 vshampor

Quick update; I am still working on it and I am behind schedule due to unexpected increase of bugs after install of types-networkx on local to get rid of an error.

Requesting extension till 5th Feb , 23:55 hrs IST.

Grimoors avatar Jan 30 '24 19:01 Grimoors

Hello @Grimoors, are you still working on this issue or can we reassign it?

p-wysocki avatar Feb 19 '24 13:02 p-wysocki

I am sorry for delay, still on it.

Grimoors avatar Feb 22 '24 08:02 Grimoors

I'm reopening the issue due to current assignee's inactivity. If you're still working on this you can repick the task.

p-wysocki avatar Mar 12 '24 09:03 p-wysocki

@vshampor could you please unassign @Grimoors?

p-wysocki avatar Mar 12 '24 09:03 p-wysocki

@p-wysocki I apologise for my inactivity.

Grimoors avatar Mar 13 '24 15:03 Grimoors

There's no need to apologize, we're just making sure all assigned issues are actually in progress. :) If you wish to continue working on this task or picking up another feel free to do so, you're always welcome to contribute.

p-wysocki avatar Mar 13 '24 15:03 p-wysocki

If @Grimoors is no longer working on this I'd like to pick it up. :)

DaniAffCH avatar Mar 13 '24 16:03 DaniAffCH

It's okay with me, but sadly I currently do not have proper permissions to assign/unassign people to issues created in the NNCF repository. @vshampor could you please help here?

p-wysocki avatar Mar 13 '24 16:03 p-wysocki

@DaniAffCH I am keen to continue work on this.

Grimoors avatar Mar 13 '24 16:03 Grimoors

Ok no problem :)

DaniAffCH avatar Mar 13 '24 16:03 DaniAffCH

The assignee was unassigned due to the lack of activity.

andrey-churkin avatar Mar 21 '24 17:03 andrey-churkin

.take

DaniAffCH avatar Mar 21 '24 17:03 DaniAffCH

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

github-actions[bot] avatar Mar 21 '24 17:03 github-actions[bot]

Hi, I just opened a PR for this

DaniAffCH avatar Mar 26 '24 11:03 DaniAffCH