[GitHub] zhreshold opened a new issue #11475: Amending ONNX importer/exporter #11213

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org


With regards,
Apache Git Services