anirudhacharya commented on issue #11475: Amending ONNX importer/exporter #11213 URL: https://github.com/apache/incubator-mxnet/issues/11475#issuecomment-401211603 Hi @zhreshold thanks for the feedback - I will make a PR to change ``python-pytest`` to ``onnx``. But the other changes could be hard to accommodate - - onnx2mx/mx2onnx, from the perspective of inside mxnet python package, it is more reasonable to use name from_onnx/to_onnx - This change was made in the recent [PR](https://github.com/apache/incubator-mxnet/pull/11213#discussion_r195585421) and I think it makes things more clear and unambiguous because ``onnx2mx`` says a lot more about what the module does compared to ``from_onnx``. But I am still open to discussion here. - Please be considerate about modules and their purposes before spreading to many files. For example, https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/contrib/onnx/mx2onnx/_export_helper.py don't necessarily need to be a separate module, a private function in export_onnx.py is more appropriate. - import_model/import_onnx/import_to_gluon.py are basically doing similar things, just put them in import_onnx.py. Same story for export_model/export_onnx. What is the advantage we derive by merging all these into one file now. Lets discuss this over a call/in-person - Name of highest level APIs, e.g., import_model, get_model_metadata, import_to_gluon, export_model are totally dummy and missing valid info. I suggest to use import_onnx, get_onnx_metadata, export_onnx, import_to_gluon can be deleted and merged as an option for example import_onnx(to_gluon=True) This is API breakage. the ``import_model`` API was already shipped as part of v1.2 breaking it now would not be very customer friendly. There are blog posts and tutorials published based on this. Also as an aside, I think all this is very valuable feedback and I thank you for that but my perception is that it just came a bit late. It is hard to incorporate them after actually shipping the product. We had sent out design proposal for this module on the mxnet dev list in early March and then again we asked for one more round of feedback in the month of April. We created confluence wiki with [API Design Document](https://cwiki.apache.org/confluence/display/MXNET/ONNX-MXNet+API+Design) and sent it out on the dev list for review. We also solicited reviews on all the PRs. Going ahead it would be great if we could get the feedback during design and review phase rather than after merging and shipping the code.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
