Amending ONNX importer/exporter #11213

[zhreshold]

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.

[zhreshold]

@anirudhacharya

[anirudhacharya]

@spidydev @Roshrini

[anirudhacharya]

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 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 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.

[marcoabreu]

Sorry, but I feel very strongly about the pytest folder naming and I would like it to stay as it is. The reason is that you're otherwise not able to distinguish nosetests tests from pytest tests. That's why we have one big parent folder which contains all pytest tests. By our convention, all Python tests are nosetests by default. The special folder name clearly shows that this convention does not apply there and makes it more transparent. please leave it as it is.

[frankfliu]

Thank you for submitting the issue! We are labeling it so MXNet community members can help resolve it.

[zhreshold]

@marcoabreu If you think a parent folder for all pytest is necessary and reserved for future, I can take that. But isn’t pytest itself indicating python tests? IMO pytest is better than python-pytest as a parent folder name in this case.

I always insist that a good naming and reasonable structure is critical to a project’ capability of maintenance and allow external contributors to better understand what has been done without asking “what is it?”. Therefore every decision had to be made with cautious otherwise it’s so hard to change later.

I understand your situation that some testing facility may already depend on this, if you insist, I can hold the change as it’s a minor issue. But I am afraid there will never be a good chance to fix it in the future

[marcoabreu]

We currently have the following structure:

tests/python-pytest
tests/python
tests/cpp

The current structure is basically tests/{LANGUAGE} and I think it's quite user-friendly to keep it that way (we have some exceptions already). tests/python is nosetests and basically every python test is nose by default if not specified otherwise; the same counts for tests/cpp being for gtest. Having a folder called python-pytest on the same level as the regular python tests covered by nose makes it easily distinguishable that there is an exception from our standard. I rather prefer to have explicit naming opposed to having implicit ones (like python being implied by pytest). From my experience, this "verbose" style makes it more structured and easier for newcomers to understand.

[zhreshold]

Resolved offline.

Things left:

• Merging files
• Leave some changes intact for consistency.