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]: Fixing NNCFGraph export for visualization in Netron #2552

Closed
andrey-churkin opened this issue Mar 6, 2024 · 6 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@andrey-churkin
Copy link
Contributor

Context

Neural Network Compression Framework (NNCF) uses an instance of NNCFGraph class to represent a framework-specific deep learning model. An instance of NNCFGraph class is used in almost every algorithm and serves as the primary means to obtain information about deep learning model.

It is very beneficial to have a way to visualize such graph. One possible way to visualize it is by using a .dot file.
Please see visualize_graph() method:

def visualize_graph(self, path: str):
out_graph = self._get_graph_for_visualization()
write_dot_graph(out_graph, path)

The main disadvantage of this approach is that it is very difficult to analyze a .dot file for very large models. It is better to use Netron. We already have a method that allows us to export an instance of the NNCFGraph class to a format supported by Netron. Actually, we export NNCFGraph to the OpenVINO IR format. Please see OpenVINO IR format specification. We save only the topology (.XML file).

Please follow the following code snippet to understand how to save NNCFGraph for visualization in Netron

import openvino as ov
from nncf.common.factory import NNCFGraphFactory
from nncf.experimental.common.graph.netron import save_for_netron

ov_model = ov.Core().read_model("<MODEL_PATH>")
graph = NNCFGraphFactory.create(ov_model)
save_for_netron(graph, "graph.xml")

What needs to be done?

  • Currently, it is not possible to open the resulting graph.xml in Netron. The main task is to resolve this problem by fixing the export. Netron returns the following error: Error loading OpenVINO model. Unsupported precision 'undefined'
  • Add docstrings for all methods (get_graph_desc(), save_for_netron()) and classes (PortDesc, NodeDesc, EdgeDesc) inside nncf.experimental.common.graph.netron module
  • Add tests for the following methods
    • get_graph_desc()
    • PortDesc.as_xml_element()
    • NodeDesc.as_xml_element()
    • EdgeDesc.as_xml_element()

Example Pull Requests

N/A

Resources

Contact points

@andrey-churkin @alexsu52

Ticket

N/A

@andrey-churkin andrey-churkin added the good first issue Good for newcomers label Mar 6, 2024
@DaniAffCH
Copy link
Contributor

.take

Copy link

github-actions bot commented Mar 6, 2024

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

@DaniAffCH
Copy link
Contributor

DaniAffCH commented Mar 6, 2024

Hi @andrey-churkin,
I dug deeper into the issue and found a potential solution, but I want to discuss the details with you.

The dtype should be retrieved from the instances of the class NNCFGraphEdge. However, the class contains only the nncf dtype that could be either FLOAT or INTEGER. This is because the original ov dtype is squished into one of these two classes

https://github.com/openvinotoolkit/nncf/blob/481ce9d9624c4b699878aa337d2e9b78dc25a517/nncf/openvino/graph/nncf_graph_builder.py#L32C1-L63C1

The Netron OpenVino frontend supports only the native OpenVino datatypes

https://github.com/lutzroeder/netron/blob/49e3a04a3210cf655e0abb04016e36b2a3bfcebf/source/openvino.js#L748C1-L772C111

So we can choose to save the original ov dtype into the class NNCFGraphEdge to display the correct layer dtype in Netron.

An alternative solution would be to map the nncf dtype FLOAT into f32 and INTEGER into i32.

I think the first solution would be the correct one as it will allow to display of the correct data type in Netron, but this requires modifying all the NNCFGraphEdge instantiations. Or maybe I could set the parameter as optional.

Let me know your thoughts on this, so I can proceed with the implementation

@andrey-churkin
Copy link
Contributor Author

andrey-churkin commented Mar 8, 2024

@DaniAffCH Hi
I think we can map Dtype.FLOAT to "fp32" and DType.INTEGER to "i32". It allows us to avoid significant validation.

@DaniAffCH
Copy link
Contributor

I've just submitted the PR #2567 solving this issue

andrey-churkin pushed a commit that referenced this issue Mar 13, 2024
### Changes

This PR solves #2552 by fixing the Netron visualization. The function
`save_for_netron` now produces an XML file that can be correctly opened
by Netron. To achieve this, a dummy `dtype` conversion has been
introduced, as discussed in #2552. This conversion maps the nncf dtype
`Dtype.FLOAT` to `f32` and `DType.INTEGER` to `i32`.

The `precision` parameter of the class `PortDesc` now is no longer
optional as it's always available and it's necessary to produce a
working XML file.

In addition, I added a docstring for all the functions/classes and
implemented tests for the following methods:

- `get_graph_desc()`
- `PortDesc.as_xml_element()`
- `NodeDesc.as_xml_element()`
- `EdgeDesc.as_xml_element()`
 

### Reason for changes

<!--- Why should the change be applied -->
It was not possible to open XML files produced by the function
`save_for_netron` due to the error: `Error loading OpenVINO model.
Unsupported precision 'undefined'`

### Related tickets

N/A

### Tests

To validate the accuracy of the modifications, I created Netron XML
files from multiple ONNX models. The visualization of these models in
Netron was successful, confirming the effectiveness of the changes.
@andrey-churkin
Copy link
Contributor Author

@DaniAffCH Thanks for the contribution!

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

2 participants