Introduce internal interface for ValuePropBackend
#201
Conversation
Signed-off-by: Atanas Dimitrov <[email protected]>
Signed-off-by: Atanas Dimitrov <[email protected]>
neNasko1
requested review from
adityagoel4512,
cbourjau and
jbachurski
as code owners
| ) | ||
| # TODO: Implement function propagation |
There was a problem hiding this comment.
Is this a TODO for this PR or does the TODO reflect pre-existing behaviour?
There was a problem hiding this comment.
This TODO reflect a previous bug that got introduced in #189.
It was uncovered after restricting the PropDict to be dict[str, PropValue] instead of dict[str, PropValueType] and has to do with the into_vars which gets called on the output _VarInfos-s with the input propagated values.
There was a problem hiding this comment.
There are quite a lot of moving parts in this PR. Is this change absolutely necessary to introducing the BaseValuePropBackend? It would be helpful to have one PR for fixing that bug and then a PR that just handles the value propagation interface.
| setattr(self.session_options, opt, val) | ||
|
|
||
| def wrap_feed(self, value: PropValue) -> ORTValue: | ||
| return value.to_ort_value() |
There was a problem hiding this comment.
It's not clear to me why you are leaving the PropValue -> ORTValue conversion in the PropValue. This is the only conceivable call site right? Same applies for RefValue and the opposite translation. I would just inline that here.
There was a problem hiding this comment.
Since the PropValue-s are generally accessible from the public API and those are public functions of them I guess this is not the only call site. I am not sure if this is true in practice, i.e. I don't think any users are using that functionality.
The better point in keeping them this way is that the to_ref_value and to_ort_value are intertwined so it doesn't make sense to split them apart.
| if not isinstance(backend, BaseValuePropBackend): | ||
| warnings.warn( | ||
| "using '_future.value_prop_backend' with a 'spox._future.ValuePropBackend' is deprecated and will be removed in the future", | ||
| DeprecationWarning, | ||
| ) | ||
|
|
||
| new_backend: spox._value_prop_backend.BaseValuePropBackend | None = None | ||
| if backend == spox._value_prop.ValuePropBackend.REFERENCE: | ||
| new_backend = spox._value_prop_backend.ReferenceValuePropBackend() | ||
| elif backend == spox._value_prop.ValuePropBackend.ONNXRUNTIME: | ||
| new_backend = spox._value_prop_backend.OnnxruntimeValuePropBackend() | ||
| elif isinstance(backend, BaseValuePropBackend): | ||
| new_backend = backend |
There was a problem hiding this comment.
Why are you not using set_value_prop_backend here?
There was a problem hiding this comment.
This is currently a minimum effort way to keep the old interface working for the time being. In this case it doesn't make sense to use set_value_prop_backend since this is a context manager and it's better to lend the work to the more suited new context manager.
|
@adityagoel4512 I have left the open conversations here so you could review them on #202 |
Since we currently "bless" the reference and onnxruntime implementations as the only way to propagate value internally, this PR refactors a bit the internals to allow a better interface for "custom" execution providers(i.e. @cbourjau rust runtime, onnx-mlir...).
Consult the tests for a POC which could also be used to register custom opsets as well.
An additional internal change is related to the
PropDicttype as previously it was too wide and didn't capture the correct semantics of the returned values from onnxruntime. As per the definition of from_ref_value we can see that the returned feed is of typedict[str, PropValue]which makes more sense to model around.This exposed a bug in the function value inference which was unnoticed and is now fixed.
Checklist
CHANGELOG.rstentry