Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle name conflict (attribute parameter) when converting back to python #1166

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

gramalingam
Copy link
Collaborator

Attribute parameters and normal values (like tensors) use the same namespace in onnxscript/python. This can cause a conflict when converting ONNX proto back to onnxscript (since these are different namespaces in ONNX). Examples of where this happens shown in test-case below: eg., when an attribute-parameter "yyy" is used as a value, the onnxscript translator introduces a value called "yyy" which is bound to the attribute-value "yyy".

Fix this in the exporter.

Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
onnxscript/backend/onnx_export_test.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export_test.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
onnxscript/backend/onnx_export.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (10f9a1f) 78.55% compared to head (632d44b) 78.52%.

Files Patch % Lines
onnxscript/backend/onnx_export.py 84.28% 8 Missing and 3 partials ⚠️
onnxscript/backend/onnx_export_test.py 50.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1166      +/-   ##
==========================================
- Coverage   78.55%   78.52%   -0.03%     
==========================================
  Files         118      118              
  Lines       15154    15231      +77     
  Branches     1620     1635      +15     
==========================================
+ Hits        11904    11960      +56     
- Misses       2872     2891      +19     
- Partials      378      380       +2     

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

Copy link

github-actions bot commented Nov 18, 2023

Test Results

         18 files  ±0         18 suites  ±0   1h 9m 40s ⏱️ + 11m 35s
  11 188 tests +1    8 343 ✔️ +1      2 792 💤 +1       53  - 1 
160 011 runs  +8  36 686 ✔️ +9  121 282 💤 ±0  2 043  - 1 

For more details on these failures, see this check.

Results for commit 632d44b. ± Comparison against base commit 10f9a1f.

This pull request skips 1 test.
onnxscript.tests.function_libs.torch_lib.ops_test.TestOutputConsistencyFullGraphCPU ‑ test_output_match_opinfo__nn_functional_max_pool3d_cpu_float32

♻️ This comment has been updated with latest results.

@gramalingam gramalingam merged commit e75da82 into main Nov 20, 2023
27 of 31 checks passed
@gramalingam gramalingam deleted the rama/to-os branch November 20, 2023 18:00
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.

2 participants