-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Enable GraphIterator interface by default #33400
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Maxim Vafin <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables the GraphIterator interface by default for ONNX frontend processing, adds comprehensive testing for legacy behavior in GitHub Actions, and fixes an issue in OpExtension attribute handling.
- GraphIterator is now enabled by default (when
ONNX_ITERATORis not explicitly set to disable it) - Test infrastructure updated to run both with GraphIterator enabled and disabled
- Python and C++ test helpers added to conditionally skip ONNX Editor tests when GraphIterator is active
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/frontends/onnx/tests/tests_python/test_frontend_onnx_editor.py |
Adds helper functions and xfail marks for editor tests that are incompatible with GraphIterator |
src/frontends/onnx/tests/tests_python/test_frontend_onnx.py |
Adds GraphIterator detection and xfail marker for decode/convert tests |
src/frontends/onnx/tests/onnx_utils.hpp |
Implements C++ helper function to check if GraphIterator is enabled via environment variable |
src/frontends/onnx/tests/onnx_import_org_openvino.in.cpp |
Replaces direct std::getenv calls with the new is_graph_iterator_enabled() helper |
src/frontends/onnx/tests/onnx_editor_topological_sort.cpp |
Adds skip macros to editor tests that are incompatible with GraphIterator |
src/frontends/onnx/tests/onnx_editor_test_utils.hpp |
Defines macro and skip message for GraphIterator-incompatible editor tests |
src/frontends/onnx/tests/onnx_editor.cpp |
Adds skip macros to numerous editor tests that don't work with GraphIterator |
src/frontends/onnx/frontend/src/frontend.cpp |
Changes default behavior to enable GraphIterator and refactors environment variable checking |
src/frontends/onnx/frontend/src/core/node.cpp |
Fixes attribute value retrieval for decoder-based nodes with float-to-double upcast |
.github/workflows/job_python_unit_tests.yml |
Duplicates ONNX test runs to cover both GraphIterator enabled and disabled modes |
.github/workflows/job_cxx_unit_tests.yml |
Duplicates ONNX frontend tests to cover both GraphIterator enabled and disabled modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Maxim Vafin <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
6aa0aff to
a22acce
Compare
Signed-off-by: Maxim Vafin <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
bumbosiepsak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have few questions in few places. Let's clarify and approve afterwads.
|
Add a message about deprecation of old approach in future |
Signed-off-by: Maxim Vafin <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
Signed-off-by: Maxim Vafin <[email protected]>
| } | ||
|
|
||
| // Unknown value - print error and default to enabled | ||
| OPENVINO_WARN("Unknown value for ONNX_ITERATOR environment variable: '", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it might actually be better for the user if we exit here (instead of warning and continuing).
Rationale: easy to miss a warning - especially if you don't know what to look for.
Details:
OpExtensiontestTickets: