Fix IsOnnxStaticRegistrationDisabled() inline function breaking schema registration in external modules#7409
Merged
justinchuby merged 7 commits intomainfrom Oct 27, 2025
Conversation
…issue Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
…mentation Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix IsOnnxStaticRegistrationDisabled function for proper schema registration
Fix IsOnnxStaticRegistrationDisabled() inline function breaking schema registration in external modules
Oct 18, 2025
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7409 +/- ##
=======================================
Coverage 54.48% 54.48%
=======================================
Files 512 512
Lines 31929 31929
Branches 2868 2868
=======================================
Hits 17397 17397
Misses 13749 13749
Partials 783 783 ☔ View full report in Codecov by Sentry. |
Member
|
@eKevinHoang does this look ok to you? Thanks |
cyyever
approved these changes
Oct 20, 2025
titaiwangms
approved these changes
Oct 22, 2025
cyyever
approved these changes
Oct 24, 2025
justinchuby
approved these changes
Oct 24, 2025
Member
|
@gramalingam for approval. Thanks |
gramalingam
reviewed
Oct 24, 2025
Contributor
|
@copilot can you address the feedback/comment? |
Clarify the responsibilities of the linking module when static registration is disabled vs enabled, as requested in code review feedback. Co-authored-by: gramalingam <10075881+gramalingam@users.noreply.github.com>
auto-merge was automatically disabled
October 24, 2025 20:56
Head branch was pushed to by a user without write access
gramalingam
approved these changes
Oct 24, 2025
gramalingam
approved these changes
Oct 25, 2025
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix IsOnnxStaticRegistrationDisabled() inline function breaking schema registration in external modules
Issue Summary:
The
IsOnnxStaticRegistrationDisabled()function was defined as an inline function inonnx/defs/operator_sets.hwith behavior based on compile-time macro__ONNX_DISABLE_STATIC_REGISTRATION. This breaks encapsulation when external projects (like ONNX Runtime) use prebuilt ONNX libraries, as the function's return value depends on the caller's compile-time macro rather than ONNX's actual build configuration.Root Cause:
Inline functions with preprocessor-dependent behavior are expanded at the call site, causing the function to use the caller's compile-time configuration instead of the library's build configuration.
Solution:
Changes Made:
Documentation Added:
Added detailed comments explaining:
RegisterOnnxOperatorSetSchema()for ai.onnx domainRegisterOnnxMLOperatorSetSchema()for ai.onnx.ml domainRegisterOnnxTrainingOperatorSetSchema()for ai.onnx.training domainRegisterOnnxPreviewOperatorSetSchema()for ai.onnx.preview domainTesting Results:
SchemaRegistrationTest.DisabledOnnxStaticRegistrationAPICallvalidates correct behaviorImpact:
IsOnnxStaticRegistrationDisabled()always reflects ONNX library's actual build configurationFixes #7399
Original prompt
Fixes #7399
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.