Conversation
| auto domain_map = schema_registry->GetLatestOpsetVersions(false); | ||
| for (const auto& domain : domain_map) { | ||
| if (domain_to_version.find(domain.first) == domain_to_version.end()) { | ||
| auto iter = domain_to_version.find(domain.first); |
There was a problem hiding this comment.
If we plan to consume the latest ONNX commits before the release (and thus taking in partial opset-12 schemas) to develop opset-12 ops and unblock converter testing then the domain to version map from ONNX would return 12 for the ONNX domain. I guess we need to find a way to re-usable mechanism to clamp it to a lower version that ORT claims full support for in a release ? (In this case 11)
There was a problem hiding this comment.
It can be solved in the GetLatestOpsetVersions function.
There was a problem hiding this comment.
Yeah, this PR will disallow opset 12 models from loaded until we update the onnx commit. Once we update onnx, this won't work unless we maintain the opset version we support in a given release somewhere and check against that. While we're in the master branch, we can store opset 12 in this MAX_SUPPORTED_OPSET variable to allow testing of opset 12 op implementations by others. When we cut a release, we should use opset 11 for this variable. Would this work? Is this PR going to address this?
|
Looks like we need to revisit the #2413 PR. |
| auto domain_map = schema_registry->GetLatestOpsetVersions(false); | ||
| for (const auto& domain : domain_map) { | ||
| if (domain_to_version.find(domain.first) == domain_to_version.end()) { | ||
| auto iter = domain_to_version.find(domain.first); |
There was a problem hiding this comment.
The major differ between this change and #2413 is: here it retrieves the domain version mapping from a local schema_registry, which has better support for custom ops and custom registries.
Also, this change covers all the domains, not just onnx domains.
|
Can we add a unit test please? |
|
This change is not needed. |
3 participants
Description:
If the model has a opset version newer than onnxruntime knows, throw an exception.
Motivation and Context
From: @pranavsharma
Sent: Friday, November 8, 2019 7:32 PM
Subject: Note about opset forward compatibility in ORT.
Dwayne from the WinML team apprised me of this issue today afternoon. ORT supports opset 11 in 1.0. Let's say the resize op undergoes a change in behavior in opset 12. The schema would be the same, but the behavior would be different (possibly a spec upgrade happened causing it’s behavior to change). Now if a model that is stamped with opset 12 is inferred by ORT (that implements only opset 11), ORT will use the opset 11 implementation of the op since it registered the op as 11->INF. This doesn’t seem correct because we silently pick the wrong op implementation instead of giving an error. If we clamp our op registration with the opset version we're implementing in the release (in this case opset 11), we will (correctly) throw an error for the unimplemented opset 12 resize op.