Skip to content

Add programmatic access to version-table and extend make-model#2918

Merged
gramalingam merged 24 commits intoonnx:masterfrom
gramalingam:irversion-july-2020
Aug 20, 2020
Merged

Add programmatic access to version-table and extend make-model#2918
gramalingam merged 24 commits intoonnx:masterfrom
gramalingam:irversion-july-2020

Conversation

@gramalingam
Copy link
Copy Markdown
Contributor

The existing version of make-model uses the latest IR-VERSION as the IR version if the caller does not explicitly supply an IR version. This PR makes the version-table available for programmatic access (in Python) and provides an extended version of make-model that generates the minimum required version from the opsets specified (on a best-effort basis).

@gramalingam gramalingam requested a review from a team as a code owner July 22, 2020 20:38
Copy link
Copy Markdown
Member

@prasanthpul prasanthpul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a cross reference between this script and the versioning doc at https://github.com/onnx/onnx/blob/master/docs/Versioning.md#released-versions

Whenever one is updated, the other needs to be updated too.

@prasanthpul prasanthpul requested a review from a team July 22, 2020 22:21
Comment thread onnx/test/helper_test.py Outdated
@gramalingam
Copy link
Copy Markdown
Contributor Author

can you add a cross reference between this script and the versioning doc at https://github.com/onnx/onnx/blob/master/docs/Versioning.md#released-versions

Whenever one is updated, the other needs to be updated too.

Done, thanks.

Comment thread onnx/test/helper_test.py Outdated
@alrevuelta
Copy link
Copy Markdown

This is very useful for #2912 thanks!. Just one concern. Regarding VERSION_TABLE that comes from here, is the v1.2 entry right?

I can't find v1.2 neither in onnx PyPi nor in the GitHub Releases. The alternatives are v1.2.1 or v1.2.2.

Since @prasanthpul mentioned this:

can you add a cross reference between this script and the versioning doc at https://github.com/onnx/onnx/blob/master/docs/Versioning.md#released-versions
Whenever one is updated, the other needs to be updated too

I thought it would be appropriate to bring this up here.

@gramalingam
Copy link
Copy Markdown
Contributor Author

@alrevuelta : that's a good question. I merely copied the text document over. May be this is the history, and perhaps not all versions were published. @prasanthpul , do you know? Should we do something different for those versions?

Comment thread onnx/helper.py
@gramalingam gramalingam requested a review from a team as a code owner July 29, 2020 21:23
Comment thread onnx/defs/schema.h
Comment thread onnx/defs/schema.h
@askhade
Copy link
Copy Markdown
Contributor

askhade commented Aug 4, 2020

Circle CI is fixed now. Merging with master should resolve the failure

Comment thread docs/Versioning.md
1.6.0|6|11|2|-
1.7.0|7|12|2|1

A programmatically accessible version of the above table is available [here](../onnx/helper.py). Limited version number
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need a text here? You should update "https://github.com/onnx/onnx/blob/master/docs/OnnxReleases.md".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I updated the OnnxReleases.md file also.

Comment thread onnx/common/version.h
namespace ONNX_NAMESPACE {

// Represents the most recent release version. Updated with every release.
constexpr const char* LAST_RELEASE_VERSION = "1.7.0";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't read that file at run-time, right? The one thing we could do is to auto-generate one file from the other. But that seems non-trivial and not worth it (at this point).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Summarizing an offline discussion.) Can we make auto-generation of these files a separate task and do it in a separate PR? It will be better to decide on a solution for all the version-numbers in multiple files, instead of just for this one file.

@askhade
Copy link
Copy Markdown
Contributor

askhade commented Aug 14, 2020

@ebarsoum @gramalingam what is the status on this? The changes look good to me and I am ok with adding auto-generation as part of a separate PR.

@gramalingam
Copy link
Copy Markdown
Contributor Author

I am also fine with completing this PR.

@alrevuelta
Copy link
Copy Markdown

I mentioned this some weeks ago but it didn't got much attention. Can someone have a look? Is it right?

This is very useful for #2912 thanks!. Just one concern. Regarding VERSION_TABLE that comes from here, is the v1.2 entry right?
I can't find v1.2 neither in onnx PyPi nor in the GitHub Releases. The alternatives are v1.2.1 or v1.2.2.

@gramalingam gramalingam merged commit c443abd into onnx:master Aug 20, 2020
@gramalingam gramalingam deleted the irversion-july-2020 branch August 20, 2020 03:57
sveta-levitan pushed a commit to sveta-levitan/onnx that referenced this pull request Aug 25, 2020
…2918)

* Add programmatic access to version-table

* Add cross-links between documentation and code

* Address flake8 warnings

* add more tests

* add last release opset version map

* Add version.h

* Update Versioning.md

* add python type annotations

* fix typecheck

* add comment

* address flake8 warnings

* fix typing

* Address PR feedback

Co-authored-by: Ashwini Khade <askhade@microsoft.com>
Co-authored-by: Emad Barsoum <ebarsoum@gmail.com>
daquexian pushed a commit to daquexian/onnx that referenced this pull request Sep 19, 2020
…2918)

* Add programmatic access to version-table

* Add cross-links between documentation and code

* Address flake8 warnings

* add more tests

* add last release opset version map

* Add version.h

* Update Versioning.md

* add python type annotations

* fix typecheck

* add comment

* address flake8 warnings

* fix typing

* Address PR feedback

Co-authored-by: Ashwini Khade <askhade@microsoft.com>
Co-authored-by: Emad Barsoum <ebarsoum@gmail.com>
Signed-off-by: daquexian <daquexian566@gmail.com>
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
…2918)

* Add programmatic access to version-table

* Add cross-links between documentation and code

* Address flake8 warnings

* add more tests

* add last release opset version map

* Add version.h

* Update Versioning.md

* add python type annotations

* fix typecheck

* add comment

* address flake8 warnings

* fix typing

* Address PR feedback

Co-authored-by: Ashwini Khade <askhade@microsoft.com>
Co-authored-by: Emad Barsoum <ebarsoum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants