Skip to content

Conversation

gramalingam
Copy link
Contributor

Many onnx models used in practice use tensor names that are not valid C identifiers (as required by the ONNX standard). This PR extends the printer and parser to support such identifiers.

(It may be useful to relax the ONNX standard's requirements in this regard, but that's a separate issue, requiring some community discussion.)

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam requested a review from a team as a code owner September 4, 2024 20:56
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.23%. Comparing base (0c7132a) to head (0ff1b25).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6346   +/-   ##
=======================================
  Coverage   57.23%   57.23%           
=======================================
  Files         507      507           
  Lines       31407    31407           
  Branches     4690     4690           
=======================================
  Hits        17977    17977           
  Misses      12575    12575           
  Partials      855      855           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Do we need to update the syntax document as well?

@justinchuby justinchuby added module: parser onnx parser release notes Important changes to call out in release notes labels Sep 4, 2024
@justinchuby justinchuby added this to the 1.18 milestone Sep 4, 2024
Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam
Copy link
Contributor Author

Do we need to update the syntax document as well?

Done, thanks.

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
@gramalingam gramalingam added this pull request to the merge queue Sep 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 5, 2024
@gramalingam gramalingam added this pull request to the merge queue Sep 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 5, 2024
@justinchuby justinchuby added this pull request to the merge queue Sep 5, 2024
Merged via the queue into onnx:main with commit 85ae976 Sep 6, 2024
39 checks passed
linshokaku pushed a commit to linshokaku/onnx that referenced this pull request Oct 2, 2024
Many onnx models used in practice use tensor names that are not valid C
identifiers (as required by the ONNX standard). This PR extends the
printer and parser to support such identifiers.

(It may be useful to relax the ONNX standard's requirements in this
regard, but that's a separate issue, requiring some community
discussion.)

---------

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Signed-off-by: Linsho Kaku <linsho@preferred.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: parser onnx parser release notes Important changes to call out in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants