[IREE EP][JIT, OnnxImporter] Synchronize C++ and Python importers. #7
Conversation
| // TODO: Consider pulling in initializers on demand since there can be so | ||
| // much unused crap. | ||
| for (auto it : graph_info_.initializer_map()) { | ||
| if (failed(ImportInitializer(it.second))) | ||
| return failure(); | ||
| } | ||
| for (auto it : graph_info_.graph_proto().node()) { | ||
| if (failed(ImportNode(it))) | ||
| if (failed(ImportNode(it, gv))) |
There was a problem hiding this comment.
Is there a way to report what is failing for easier debugging?
There was a problem hiding this comment.
The errors should propagate through the function call stack. Thus the errors are usually reported at the last point of failure. If we report errors at each function in the error path, there will be too many messages and the actual problem might get lost
Your thoughts?
| @@ -712,7 +740,7 @@ Status NodeImporter::ImportGeneralNode(const onnx::NodeProto &node) { | |||
| std::vector<MlirType> output_types; | |||
| for (auto &output_name : node.output()) { | |||
| const onnx::TypeProto *type_proto = | |||
| graph_info_.FindTypeProtoForName(output_name); | |||
| gv.GetNodeArg(output_name)->TypeAsProto(); | |||
There was a problem hiding this comment.
Is this the main reason for passing the graph view? I'd like to revisit the need to use graph viewer, and whether one of the structures, such as GraphInfo should just store the graph viewer instead of passing it around as an an argument in these importer methods.
Is the problem that FindTypeProtoForName isn't finding the associated type proto for initializers?
There was a problem hiding this comment.
Actually yes, this is the main reason. I could very well store the whole graph viewer, but I was wary of the cost it would have in terms of memory. That could very well be premature optimisation, now that you mention this.
The problem as I figure it out is that it could be possible (this is just an observation, and I might be wrong here, but my grounds for this belief are rather solid) that the GraphProto that we get from graph viewer does not reflect all the nodes that live in the graph.
It would of course have been different if we were operating on an unmodified onnx protobuf obtained directly from the model, as we have in the case of the python importer.
Do you think that passing graph viewer as an arg is bad style? I think this would warrant a change then. I will push that along with the next commit.
There was a problem hiding this comment.
This type info issue doesn't just happen for initializers, but a good number of other values as well. I wanted to be sure about this, so I inserted a check to inform if the name was present in initializer_map. For a significant number of such failures, it wasn't.
Of course, if I am able to find out that something else is going on, I will be revising this. This does appear to be the most complete solution at the moment, though.
There was a problem hiding this comment.
Let me see if I understand this correctly. When an inference session is initialized for a model.onnx file (with whatever session options and EP), does ORT load the onnx model, then pass off just a graph view to the EP for further processing?
I'm guessing this is the case, since we are getting the graph from a graph view rather than a onnx::ModelProto.
If a graph view is our starting point for ORT, would it make more sense to just store the graph view and not the graph in the GraphInfo structure? Does graph view have all the same capabilities of a graph (e.g., the ability to read nodes and value info)?
There was a problem hiding this comment.
does ORT load the onnx model, then pass off just a graph view to the EP for further processing?
Yes, that is correct. This is the essence of the matter.
Does graph view have all the same capabilities of a graph (e.g., the ability to read nodes and value info)?
It actually does, and more. The graph that we operate on currently is obtained from graph viewer, and thus we can think of graph viewer as a super view of the actual onnx graph.
If a graph view is our starting point for ORT, would it make more sense to just store the graph view and not the graph in the GraphInfo structure?
This, unfortunately, requires some thought. All the processing that we do outside of internal classes like GraphInfo that works on graph viewer would now require handling inside these class and structs.
Also, I am doubtful about the ergonomics of storing the whole graph viewer, especially unless we heavily use it. Extracting values from it is something we'll still have to do.
In conclusion, storing graph viewer is actually a very viable option, though it will have to be unwrapped somewhere later down the execution path.
WIP PR aiming to patch up the C++ ONNX importer for functional equivalence with Torch-MLIR's python ONNX importer.
Still requires a good amount of work, and also iterative reviews side-by-side to ensure that the ported logic is correct.
The code might deviate from the original Python importer, mainly because we deal with OnnxRT as an intermediary, rather than directly operate on unmodified ONNX protobufs, and secondarily because C++ has its differences from Python. Therefore, it should not be expected to have bug-to-bug compatibility with the Python importer.