Add ONNX model hub python client

[mhamilton723]

Please see model hub proposal and onnx/models manifest PR for more info.

[lgtm-com]

This pull request introduces 1 alert when merging dae42a1 into c24cd42 - view on LGTM.com

new alerts:

• 1 for Module is imported with 'import' and 'import from'

[lgtm-com]

This pull request introduces 1 alert when merging d545a05 into c24cd42 - view on LGTM.com

new alerts:

• 1 for Unused local variable

[linkerzhang]

Good idea. However, I don't think it's a good idea to add this kind of tool into ONNX repo. Instead, it may be added into onnx/model or onnx/tools (if it's there).

[memoryz]

For lfs, the download URL should be media.githubusercontent.com/media. See "download_url" field in link for example. And "raw=true" is no longer needed for this URL.

Suggested change

	return "https://github.com/{}/{}/blob/{}/".format(repo_owner, repo_name, repo_branch)
	return "https://media.githubusercontent.com/media/{}/{}/{}/".format(repo_owner, repo_name, repo_branch)

[mhamilton723]

Tried this and unfortunately this 404s

[memoryz]

That's weird. I tried "https://media.githubusercontent.com/media/mhamilton723/models/onnx-hub/vision/classification/resnet/model/resnet101-v1-7.onnx" and it's giving me the payload. Let me play with the code a bit more.

[mhamilton723]

use ?raw=true

[lgtm-com]

This pull request introduces 1 alert when merging ef6420d into c24cd42 - view on LGTM.com

new alerts:

• 1 for Unused import

[mhamilton723]

Good idea. However, I don't think it's a good idea to add this kind of tool into ONNX repo. Instead, it may be added into onnx/model or onnx/tools (if it's there).

Thanks for the feedback @linkerzhang, some of the other folks in the community suggested we put this here because onnxmltools is just for converters. Do you have an email that I could loop into this discussion thread I just started? I can see motive for both and just want to ensure the community is aligned on what we should do.

[linkerzhang]

Good idea. However, I don't think it's a good idea to add this kind of tool into ONNX repo. Instead, it may be added into onnx/model or onnx/tools (if it's there).

Thanks for the feedback @linkerzhang, some of the other folks in the community suggested we put this here because onnxmltools is just for converters. Do you have an email that I could loop into this discussion thread I just started? I can see motive for both and just want to ensure the community is aligned on what we should do.

my email is, linkerzhang@yeah.net.

How about onnx/model then?

[askhade]

The advantage of adding this to onnx main repo is it can be part of the onnx release package and then users can simply import onnx.hub and get models... There is no release package for onnx\models ...

[mhamilton723]

Good idea. However, I don't think it's a good idea to add this kind of tool into ONNX repo. Instead, it may be added into onnx/model or onnx/tools (if it's there).

Thanks for the feedback @linkerzhang, some of the other folks in the community suggested we put this here because onnxmltools is just for converters. Do you have an email that I could loop into this discussion thread I just started? I can see motive for both and just want to ensure the community is aligned on what we should do.

my email is, linkerzhang@yeah.net.

How about onnx/model then?

We brought up this discussion in the steering committee and they mentioned onnx/onnx was the way to go to make it simple for folks to get started. onnx/model doesn't have any python package so it would be a big infra change and also less desireable from a easy user experience

[linkerzhang]

Good idea. However, I don't think it's a good idea to add this kind of tool into ONNX repo. Instead, it may be added into onnx/model or onnx/tools (if it's there).

Thanks for the feedback @linkerzhang, some of the other folks in the community suggested we put this here because onnxmltools is just for converters. Do you have an email that I could loop into this discussion thread I just started? I can see motive for both and just want to ensure the community is aligned on what we should do.

my email is, linkerzhang@yeah.net.
How about onnx/model then?

We brought up this discussion in the steering committee and they mentioned onnx/onnx was the way to go to make it simple for folks to get started. onnx/model doesn't have any python package so it would be a big infra change and also less desireable from a easy user experience

OK. Thanks for clarification!

[linkerzhang]

Interesting... 0 file changed?

[memoryz]

Interesting... 0 file changed?

Sorry my mistake - I just fixed the problem. Please check again.

[mhamilton723]

@askhade and @linkerzhang Ready for your review, thanks for your help!

[askhade]

CIs are failing because of flake8 errors:
onnx/hub.py:233:19: F523 '...'.format(...) has unused arguments at position(s): 0, 1
onnx/hub.py:239:1: W391 blank line at end of file
onnx/test/hub_test.py:79:1: W391 blank line at end of file
flake8 returned failures

[memoryz]

CIs are failing because of flake8 errors:
onnx/hub.py:233:19: F523 '...'.format(...) has unused arguments at position(s): 0, 1
onnx/hub.py:239:1: W391 blank line at end of file
onnx/test/hub_test.py:79:1: W391 blank line at end of file
flake8 returned failures

Looking...

[memoryz]

CIs are failing because of flake8 errors:
onnx/hub.py:233:19: F523 '...'.format(...) has unused arguments at position(s): 0, 1
onnx/hub.py:239:1: W391 blank line at end of file
onnx/test/hub_test.py:79:1: W391 blank line at end of file
flake8 returned failures

Fixed.

[rajeevnalawadi]

These changes look good

Ke has agreed in #3663 (comment) but has not been reachable to clear the review. Since there are a number of other approvals, dismissing the review to unblock on team.