Collision avoidence for op registration. - #12505
Conversation
What was being set with the 'extra |
|
|
||
| // Call once to trigger schema static registration. | ||
| // Note: This is not transactional and can still collide with concurrent calls from other libs. | ||
| if (!OpSchemaRegistry::Schema("Add")) { |
There was a problem hiding this comment.
Is __ONNX_DISABLE_STATIC_REGISTRATION set by default so that this call doesn't trigger registration of all schemas?
What's the reason we need to do this at the ORT level vs. ONNX gracefully handling multiple calls to schema registration? The check for an semi-arbitrary operator name feels a little hacky.
There was a problem hiding this comment.
Is
__ONNX_DISABLE_STATIC_REGISTRATIONset by default so that this call doesn't trigger registration of all schemas?
ONNX_DISABLE_STATIC_REGISTRATION is off by default, AFAIK it's just a backdoor for whoever need to load a subset of opset manually, for memory or collision avoidence.
What's the reason we need to do this at the ORT level vs. ONNX gracefully handling multiple calls to schema registration?
You'll need to ask them (i.e. onnx and protobuf) for sure.
But clearly the static/shared protobuf discussion didn't move much forward in the past few years.
And google themselves is always doing that monorepo + everything-static thing, so I'm not expecting this to end well in a foreseeable future.
The check for an semi-arbitrary operator name feels a little hacky.
Yes, so I picked the very basic Add.
There was a problem hiding this comment.
If you specify ONNX_DISABLE_STATIC_REGISTRATION when creating the shared onnx library you're linking against, is the change in ORT required?
I'm trying to understand why we need to add this hack to ORT given it's not necessary in the typical usage of ORT.
There was a problem hiding this comment.
That would be a breaking change to onnx behavior as no op would be registered, making onnx itself unusable in normal cases.
|
When linked with shared onnx libs, its static registration already did the work, and ort should not re-reg again. Instead of hard-coding who's responsible for the job, now we check and reg if missing. Call to `map()` inside `Schema()` will ensure init is done exactly once and is before probing. If not all domains are there for whatever reason, ort will still cover that up. This change also removes the need to disable that with extra `-D` in ort build and avoid unnecessary divergence.
|
ping on this one |
|
This is an issue for vcpkg users, preventing a "clean" workflow, see here: microsoft/vcpkg#36850. Users of onnxruntime have to create a custom workaround (called "custom triplet") to build onnx with |
3 participants
When linked with shared onnx libs, its static registration already did the work, and ort should not re-reg again.
Instead of hard-coding who's responsible for the job, now we check and reg if missing.
Call to
map()insideSchema()will ensure init is done exactly once and is before probing.If not all domains are there for whatever reason, ort will still cover that up.
This change also removes the need to disable that with extra
-Din ort build and avoid unnecessary divergence.