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

mypy checks for common graph #2600

Merged
merged 13 commits into from
Mar 26, 2024
Merged

mypy checks for common graph #2600

merged 13 commits into from
Mar 26, 2024

Conversation

DaniAffCH
Copy link
Contributor

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.

@DaniAffCH DaniAffCH requested a review from a team as a code owner March 25, 2024 21:44
@github-actions github-actions bot added the NNCF Common Pull request that updates NNCF Common label Mar 25, 2024
@DaniAffCH
Copy link
Contributor Author

DaniAffCH commented Mar 25, 2024

Actually, 3 mypy errors are remaining. I wanted to discuss them with you.

@DaniAffCH
Copy link
Contributor Author

https://github.com/openvinotoolkit/nncf/blob/b7ba5ad1f8c785e11d3b2a3864bd33aa1787bb56/nncf/common/graph/patterns/manager.py#L118C1-L131C97

In this function, the parameter model_type is optional. However, the function _get_full_pattern_graph expects model_type to be of type ModelType and not optional. The case where model_type is None is not explicitly handled. Is this a problem? (I didn't modify the datatype because I wanted to discuss with you this detail before, to figure out if I should modify get_full_ignored_pattern_graph or _get_full_pattern_graph)

@DaniAffCH
Copy link
Contributor Author

https://github.com/openvinotoolkit/nncf/blob/b7ba5ad1f8c785e11d3b2a3864bd33aa1787bb56/nncf/common/graph/graph.py#L677C1-L681C30

Here nx_edge[2] is accessed. However, the variable boundary comes from the function _get_edge_boundaries that returns a list of Tuple[str, str] with only 2 elements. If so, nx_edge[2] would be out of bounds.
Is the return type of _get_edge_boundaries incorrect, or is there a more fundamental issue at play?

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

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

Project coverage is 84.91%. Comparing base (b7ba5ad) to head (069a607).
Report is 4 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2600      +/-   ##
===========================================
- Coverage    91.16%   84.91%   -6.26%     
===========================================
  Files          494      494              
  Lines        45350    45390      +40     
===========================================
- Hits         41342    38541    -2801     
- Misses        4008     6849    +2841     
Files Coverage Δ
nncf/common/graph/graph_matching.py 100.00% <100.00%> (ø)
nncf/common/graph/layer_attributes.py 96.29% <100.00%> (ø)
nncf/common/graph/model_transformer.py 87.50% <100.00%> (ø)
nncf/common/graph/operator_metatypes.py 93.97% <100.00%> (ø)
nncf/common/graph/patterns/patterns.py 96.46% <100.00%> (+0.03%) ⬆️
nncf/common/graph/transformations/commands.py 97.01% <100.00%> (ø)
nncf/common/graph/transformations/layout.py 100.00% <100.00%> (ø)
nncf/common/graph/utils.py 82.14% <100.00%> (ø)
nncf/common/utils/registry.py 90.90% <100.00%> (ø)
nncf/common/graph/graph.py 96.25% <97.50%> (-0.27%) ⬇️
... and 1 more

... and 60 files with indirect coverage changes

Flag Coverage Δ
COMMON 44.13% <69.31%> (-0.02%) ⬇️
ONNX 34.67% <78.40%> (+0.01%) ⬆️
OPENVINO ∅ <ø> (∅)
TENSORFLOW 30.10% <68.18%> (-0.02%) ⬇️
TORCH 65.92% <86.36%> (-0.03%) ⬇️

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

Components Coverage Δ
common 93.13% <94.31%> (-0.67%) ⬇️
torch 93.48% <ø> (+<0.01%) ⬆️
tensorflow 93.74% <ø> (ø)
onnx 93.09% <ø> (+0.06%) ⬆️
openvino 25.75% <ø> (-68.39%) ⬇️
ptq 69.92% <ø> (-20.20%) ⬇️

@alexsu52 alexsu52 requested a review from andrey-churkin March 26, 2024 11:53
@alexsu52
Copy link
Contributor

alexsu52 commented Mar 26, 2024

get_full_ignored_pattern_graph or _get_full_pattern_graph)

Thanks for your find. This is exactly what we expect from a mypy task. model_type can has None value

model_type: Optional[ModelType] = None,
Thus typing of _get_full_pattern_graph and _filter_patterns should be modified.

@alexsu52
Copy link
Contributor

https://github.com/openvinotoolkit/nncf/blob/b7ba5ad1f8c785e11d3b2a3864bd33aa1787bb56/nncf/common/graph/graph.py#L677C1-L681C30

Here nx_edge[2] is accessed. However, the variable boundary comes from the function _get_edge_boundaries that returns a list of Tuple[str, str] with only 2 elements. If so, nx_edge[2] would be out of bounds. Is the return type of _get_edge_boundaries incorrect, or is there a more fundamental issue at play?

It looks like the return typing of _get_edge_boundaries is wrong, nx.edge_boundary(graph, match, data=True) returns tuple from 3 elements:

out_edge_boundary = list(nx.edge_boundary(graph, match, data=True))

More information is here https://networkx.org/documentation/stable/reference/classes/generated/networkx.MultiGraph.edges.html

@DaniAffCH
Copy link
Contributor Author

DaniAffCH commented Mar 26, 2024

Thank you @alexsu52 for the clarification. All the mypy checks pass now, so the PR is ready to be reviewed.

nncf/common/graph/graph.py Outdated Show resolved Hide resolved
nncf/common/graph/graph.py Show resolved Hide resolved
nncf/common/graph/graph.py Show resolved Hide resolved
nncf/common/graph/graph.py Show resolved Hide resolved
nncf/common/graph/patterns/manager.py Outdated Show resolved Hide resolved
nncf/common/graph/patterns/manager.py Outdated Show resolved Hide resolved
Copy link
Contributor

@andrey-churkin andrey-churkin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@andrey-churkin andrey-churkin merged commit b28c3fe into openvinotoolkit:develop Mar 26, 2024
13 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants