This pull request introduces 1 alert when merging 7396ccb into 1cbd549 - view on LGTM.com

new alerts:

• 1 for Constant return type on member

All updated. Thank you @askhade for all the reviews

Hi @jcwchen , thank you very much for introducing this change.

Per my understanding, this allows a Runtime to register ONLY the specific opset schema. However, turning on ONNX_DISABLE_STATIC_REGISTRATION does NOT imply a "dynamic" schema registration, because the schema is not automatically "loaded" when required and "offloaded" when not in use. Registered schema holds on allocated memory until program ends. Is this correct?

If so, I want to get your thought on introducing an ONNX API to "offload" schema at runtime. The goal is to reduce MaxRSS in Runtime framework - granted that a Runtime may not need ONNX schema after model loading stage, but still needs to reserve aside RSS for it.

Do you think there's obvious reason against adding such a feature? If not, i'm happy to contribute and would appreciate any pointer. So far the "registration" looks like it's inserting pairs into the static OpName_Domain_Version_Schema_Map. What complexities to be considered for removing elements from it at runtime? Thank you very much!

Hi @q-ycong-p,
Good question. I did consider similar feature you mentioned (offload schema) when I was implementing this PR. I think it is good to have in ONNX-level and don't see any obvious concern. However, if you want to further apply this feature in downstream runtime, it might be more complex. For instance, by default onnxruntime registers all schemas in the beginning and it will need much code change to enable dynamic schema registration (add/offload dynamically). If your final goal is enabling this feature in onnxruntime, I would suggest you confirm this feature request is good to them (like raise a feature request in their repo) before you start. Still, I think such a feature is fairly good-to-have in ONNX. Thank you for your interest!

Thank you @jcwchen for the detailed reply. We have a runtime inference framework that depends on ONNX as one of the supported model formats. However unlike ONNXRuntime, we do not rely on ONNX graph representation for inference, hence this will be a beneficial ONNX-level feature for us, and for other similar downstream runtimes I think.

I will experiment on my side and update further. If easy I'll open a PR, if tricky for me, I may seek community help as a feature request.

Hi @jcwchen, I'd like to ask for your thoughts on a few items:

• Current issue: A runtime may want to register opset schema version VA when loading model A, then register opset schema version VB when loading model B, without terminating the program. From a runtime's perspective, I think it'd expect the registered ONNX schemas to be (1) bumped or downgraded, so that it's equivalent to only VB is ever registered, or (2) all schema versions in U(A, B) are registered. However, the current behavior is to throw errors due to duplicated schema registration. I think this is problematic because a runtime is given the handle to specify schema version to register, but cannot {up,down}grade that version, nor reset it to empty.

Moving forward, to expose the appropriate handles to runtime, I'm thinking (naively), in order of importance:

1. Add func ONNX_NAMESPACE::DeregisterOnnxOperatorSetSchema in operator_sets.h which empties the static OpName_Domain_Version_Schema_Map map, and set opset_version_to_load=0.
2. (good to have but I might be missing important things here) Add funcs to allow runtime to bump/downgrade opset schema verison to achieve case (1) in the above example.

Will this seem to make sense? Any advise against pitfall is appreciated here!

Update: #1 above is added in #5221. From a runtime perspective, to achieve #2, it can deregister all ONNX opset schemas first, then register new version as needed, regardless of the version(s) registered prior to the clear.

1. bumped or downgraded, so that it's equivalent to only VB is ever registered, or (2) all schema versions in U(A, B) are registered.

Personally I vote for 2) in ONNX-level. And we can have additional API to remove registered schema (all or specified). In principle, I prefer keeping ONNX simple and providing runtime sufficient onload/offload API for complex scenario. That is to say, we can still use multiple functions from ONNX to achieve 1) in runtime as well.

However, the current behavior is to throw errors due to duplicated schema registration.

Yes, I think we might need another standalone registration function without that error (or enabling an option to ignore that error in the same function if feasible).

I will take a closer look at your PR and we can continue the discussion there.

I agree that the principle is to keep ONNX simple and provide runtime sufficient onload/offload APIs. Concretely I think this means ONNX to provide APIs for runtime to:

• Register schema (all or specified): API already exists. But need to fix the duplicate schema registration error as you said, so that when a runtime register(VA) and then register(VB), ONNX should have U(VA, VB) loaded without error.
• De-register schema (all or specific): pull#5221 introduces API to naively de-register all. We can add logic to handle specific version, by implementing the opposite of the register functions.

pull#5221 simply de-registers all, for a quick turn-around to measure runtime-side memory delta on my side. I will try iterate in the PR there to expand. Also would really appreciate your input there! Ty!