zhreshold opened a new issue #11475: Amending ONNX importer/exporter #11213 URL: https://github.com/apache/incubator-mxnet/issues/11475 This thread tracks additional concerns of PR #11213 #### Naming of modules in python/mxnet/contrib/onnx: - onnx2mx/mx2onnx, from the perspective of inside mxnet python package, it is more reasonable to use name from_onnx/to_onnx. - 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. - 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)` #### Testing It's a minor issue compared to naming. - onnx unittest is special because it relies on onnx.tests - In near future I don't expect other modules to use pytest Based on these, I think `python-pytest` is not a good place to go, and `onnx` deserves a separate test folder due to it's special dependency.
---------------------------------------------------------------- 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
