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

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

Closed
vshampor opened this issue Jan 16, 2024 · 26 comments
Closed
Assignees
Labels
good first issue Good for newcomers

Comments

@vshampor
Copy link
Contributor

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.

@MaximProshin MaximProshin changed the title [Good First Issue] Make NNCF common graph code pass mypy checks [Good First Issue] [NNCF] Make NNCF common graph code pass mypy checks Jan 17, 2024
@mlukasze mlukasze added the good first issue Good for newcomers label Jan 17, 2024
@Grimoors
Copy link

I wish to request to allocation for this issue.

It is in preperation for GSoC 2024 eligibility.

@Grimoors
Copy link

.take

Copy link

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

@prajak002
Copy link

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?

@vshampor
Copy link
Contributor Author

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

@prajak002
Copy link

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

@Grimoors
Copy link

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]

@vshampor
Copy link
Contributor Author

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

@vshampor
Copy link
Contributor Author

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

@Grimoors
Copy link

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

@vshampor ; As I am already halfway done with my implementation plan for the current issue #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 #2493 regardless;

Please let me continue with work on the current issue.

@vshampor
Copy link
Contributor Author

@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.

@Grimoors
Copy link

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.

@p-wysocki
Copy link
Contributor

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

@ilya-lavrenov ilya-lavrenov transferred this issue from openvinotoolkit/openvino Feb 20, 2024
@Grimoors
Copy link

I am sorry for delay, still on it.

@p-wysocki
Copy link
Contributor

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 p-wysocki moved this from Assigned to Contributors Needed in Good first issues Mar 12, 2024
@p-wysocki
Copy link
Contributor

@vshampor could you please unassign @Grimoors?

@Grimoors
Copy link

@p-wysocki I apologise for my inactivity.

@p-wysocki
Copy link
Contributor

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.

@DaniAffCH
Copy link
Contributor

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

@p-wysocki
Copy link
Contributor

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?

@Grimoors
Copy link

@DaniAffCH I am keen to continue work on this.

@DaniAffCH
Copy link
Contributor

Ok no problem :)

@p-wysocki p-wysocki moved this from Contributors Needed to Assigned in Good first issues Mar 14, 2024
@andrey-churkin
Copy link
Contributor

The assignee was unassigned due to the lack of activity.

@andrey-churkin andrey-churkin moved this from Assigned to Contributors Needed in Good first issues Mar 21, 2024
@DaniAffCH
Copy link
Contributor

.take

Copy link

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

@alexsu52 alexsu52 moved this from Contributors Needed to Assigned in Good first issues Mar 22, 2024
@DaniAffCH
Copy link
Contributor

Hi, I just opened a PR for this

@mlukasze mlukasze moved this from Assigned to In Review in Good first issues Mar 26, 2024
andrey-churkin pushed a commit that referenced this issue Mar 26, 2024
### Changes

This PR closes issue #2495 by addressing various mypy checks and
enhancing type safety in the codebase.

- Resolved specific mypy errors related to type inconsistencies.
- Utilized `# type:ignore` for cases requiring significant refactoring
due to untyped packages like `networkx`.
- Added the directory `nncf/common/graph` to `.mypy.ini` to include
additional files for type checking.

### Related Tickets

N/A

### Tests

Pytests were run to ensure that the changes did not modify the common
logic and that the codebase remained functional.
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Archived in project
Development

No branches or pull requests

7 participants