@bh/model-validation - #1324
Conversation
- Add DynamicDim type and Dynamic() constructor for dimensions that require companion methods
- Add Static(), Constant() constructors and tagged union types (StaticDim, DynamicDim, ConstantDim)
- Update SymbolicTensor to convert plain numbers/strings to typed dimensions internally
- Add companion method validation in validateModelSchema (get_dynamic_dims_*, get_enumerated_dims_*)
- Extract parseDynamicInputShapes and parseEnumeratedInputShapes to model_companion_parser.h/cpp
- Use Dynamic('L') in textEmbedding and Dynamic('frames') in fsmnVoiceActivityDetection
- Clean up index.ts exports to use export * for core modules
|
I guess that cross-tensor validation is important and ultimately we want it, but for now it's ok to skip it. Just make sure we have some follow-up issue for this kind of check. |
| if (maxDim > inputMeta.sizes()[axis]) { | ||
| throw std::runtime_error(std::format("{}output[{}], axis {} max dimension ({}) " | ||
| "exceeds model metadata upper limit ({})", | ||
| ctx, tensorIndex, axis, maxDim, inputMeta.sizes()[axis])); | ||
| } |
There was a problem hiding this comment.
How about the analogical check for minDim?
| using executorch::aten::ScalarType; | ||
|
|
||
| const auto getEnumeratedShapesMethodName = std::format("get_enumerated_dims_{}", methodName); | ||
| const auto ctx = getEnumeratedShapesMethodName + ": "; | ||
|
|
||
| auto methodNames = unwrap(ctx + "failed to get method names", module.method_names()); | ||
| if (methodName == getEnumeratedShapesMethodName || | ||
| !methodNames.contains(getEnumeratedShapesMethodName)) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| auto methodMeta = unwrap(std::format("{}failed to get meta for method '{}'", ctx, methodName), | ||
| module.method_meta(methodName)); | ||
|
|
||
| size_t expectedTensorInputs = 0; | ||
| for (size_t i = 0; i < methodMeta.num_inputs(); ++i) { | ||
| auto tag = unwrap(std::format("{}failed to get tag for input [{}]", ctx, i), | ||
| methodMeta.input_tag(i)); | ||
| if (tag == executorch::runtime::Tag::Tensor) { | ||
| expectedTensorInputs++; | ||
| } | ||
| } | ||
|
|
||
| auto result = unwrap(ctx + "failed to execute", module.execute(getEnumeratedShapesMethodName)); | ||
| if (result.size() != expectedTensorInputs) { | ||
| throw std::runtime_error(std::format("{}number of outputs returned ({}) does not match the number of " | ||
| "tensor inputs declared by method '{}' ({})", | ||
| ctx, result.size(), methodName, expectedTensorInputs)); | ||
| } |
There was a problem hiding this comment.
Maybe we should abstract common part of these two into function in anonymous helper?
| for (size_t i = 0; i < expectedShape.size(); ++i) { | ||
| const auto &dim = expectedShape.at(i); | ||
|
|
||
| if (std::holds_alternative<int32_t>(dim)) { |
There was a problem hiding this comment.
Maybe use std::visit here instead of if else checks?
| inline std::shared_ptr<TensorHostObject> | ||
| fromJs(jsi::Runtime &rt, const std::string &name, const jsi::Value &value, | ||
| std::optional<DType> expectedDtype, const ShapeConstraint &shapeConstraint) { | ||
| if (std::holds_alternative<SymbolicShape>(shapeConstraint)) { |
There was a problem hiding this comment.
Consider std::visit here, definitely not as useful as in tge previous case
|
@msluszniak I'm going to close this PR because I came to a conclusion (based on exhaustive experiments with different API designs) that this is not an ideal way of solving the validation problem. I think we should actually export static method in .pte files which would return JSON string with full info about model schema. I would like to discuss the details whenever you're free. |
|
Let's discuss it today! :)) |
Description
Summary Proof of Concept
Introduces a typed dimension model (
Static/Dynamic/Constant) on the JS side and a companion-method parser on the C++ side so models can declare runtime-enforced dynamic shape constraints (min/max/step ranges or enumerated shape lists) that ExecuTorch's.ptemetadata does not otherwise expose.Changes
modelSchema.ts— Replaces loosenumber | stringshape dims with a tagged union (StaticDim | DynamicDim | ConstantDim) andStatic()/Dynamic()/Constant()constructors.SymbolicTensornormalizes plain numbers/strings to the typed form internally, so existing call sites keep working.Dynamic('L')additionally asserts the.pteexports a matching companion method.model_companion_parser.{h,cpp}— ExtractsparseDynamicInputShapes(formerly inlined inmodel.cpp) and addsparseEnumeratedInputShapesfor theget_enumerated_dims_<method>variant. Same tensor-shape contract ([rank, 3]int32 for ranges;[num_shapes, rank]int32 for enums) — kept identical to avoid a breaking change to the Python export convention.tensor_helpers.h— IntroducesShapeConstraint = variant<SymbolicShape, vector<SymbolicShape>>and a routingfromJsoverload so a single constraints map can hold either range or enum shapes without leaking the distinction intoModel.model.{h,cpp}— RenamesdynamicInputShapes_→inputShapeConstraints_; rejects methods that define both companion flavors as mutually exclusive.textEmbeddingandfsmnVoiceActivityDetectionadoptDynamic(...)to declare the symbolic dims that already required companion methods in practice.Design rationale
toTypedShapekeeps the public API source-compatible..ptemetadata) is the only way to recover min/max/step ranges today — ExecuTorch serializes only the static upper bound. Reusing it for enumerated shapes keeps a single extension mechanism.ShapeConstraintvariant + routing overload centralizes range/enum dispatch intensor_helpersrather thanmodel.cpp, soModelstays agnostic of the constraint flavor and future constraint types stay pluggable.get_dynamic_dims_*andget_enumerated_dims_*) at load time rather than at validation time.Load-time (JS) vs. runtime (C++)
This split is intentional because the
.ptemetadata only serializes the static upper bounds of dynamic dimensions — the actual[min, max, step]ranges and enumerated shape lists are only retrievable by executing the companion method, and the C++ layer has to run them anyway.validateModelSchema): pure metadata check run once when a schema is bound to a model inloadModel. Verifies tag counts, dtype, static shape signature, and — new in this branch — asserts that any input shape usingDynamic(...)has a corresponding companion method present inmodel.getMethodNames(). Surfaces misconfigured schemas (Dynamicwithout a companion, or non-matching shapes) as a hard error before any forward call instead of silently degrading to static-upper-bound-only behavior.Modelctor + per-calltensor::fromJs): theModelconstructor executes each companion method once, caches the parsedShapeConstraints intoinputShapeConstraints_, and rejects methods that declare bothget_dynamic_dims_*andget_enumerated_dims_*. On everyforward,fromJsresolves the per-input constraint and validates the concrete incoming tensor against the actual dynamic range or enumerated list — this is where the real numeric bounds get enforced, not in JS.Why validation isn't duplicated at load-time
The companion methods could be executed from JS at validate-time (the module is loaded by then, and the cost is negligible), but doing so duplicates work the runtime already does:
validateModelSchemais intentionally a pure function of(modelMetadata, schema).Dynamic(...)is a promise the schema makes about the model export ("this dim is genuinely dynamic and a companion exists"), not a specification of numeric values the schema enforces. The schema layer only needs to confirm the promise is honored — which amethodNames.includes(...)check already does — and let the runtime enforce the actual numbers.Modelmust parse the companion output intoinputShapeConstraints_regardless, sincefromJsenforces per-input bounds on every forward. Executing and parsing the same companions again from JS would establish a second representation of the same constraint with no mechanism to keep them in sync across rebinds/reloads.Known limitation: cross-tensor constraints
Per-tensor runtime validation is not complete —
fromJsvalidates each input tensor against its ownShapeConstraintindependently and cannot enforce consistency of a shared symbolic dim across multiple inputs. Concretely, passingtokensandmaskof mismatchedLtotextEmbeddingbypasses our validators and surfaces as an opaque ExecuTorch internal error deep in the forward.Introduces a breaking change?
Type of change
Tested on
Testing instructions
Screenshots
Related issues
#1323
Checklist
Additional notes