Repository navigation
fix(engine): report unimplemented workflow capabilities instead of refusing to load - #1790
Merged
Merged
Conversation
…fusing to load The canonical package describes a workflow, and the capabilities it declares (workflow_ssa, serving_service_contract, bounded_state_recurrence, typed_emit, emit_valid_length, linear_effects, loop_induction_values, nested_control_flow) describe that manifest -- not extra computation the bare-decoder path performs. That path derives its IO from the graph and never interprets the manifest, so failing admission over a capability the runtime has not implemented rejects packages that decode correctly. Measured: a mobius-exported package declaring all eight capabilities was rejected at admission; with the gate relaxed it loads, builds a native CUDA decode session, and produces a coherent token stream whose first token matches the same model exported without a workflow manifest. Structural defects stay fatal -- a document that does not describe a runnable model still fails closed, and that case keeps dedicated coverage. Closes #1767 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1790 +/- ##
==========================================
+ Coverage 80.19% 80.28% +0.09%
==========================================
Files 411 411
Lines 199146 200160 +1014
Branches 199146 200160 +1014
==========================================
+ Hits 159706 160702 +996
+ Misses 33996 33994 -2
- Partials 5444 5464 +20
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
… warn Review of the first commit found a real defect: relaxing the whole capability set also relaxed `parameter_adapters` / `heterogeneous_adapter_batching`, which `metadata_only_required_capabilities` derives whenever `metadata.adapters` is present. The bare-decode path never reads `metadata.adapters`, so an adapter package would have loaded and silently emitted BASE-MODEL output instead of the adapted output it describes -- wrong output, not degraded output, with nothing for the caller to notice. Split the set on one question: does the capability describe the document, or prescribe computation this path must perform? - Descriptive (workflow manifest: workflow_ssa, serving_service_contract, bounded_state_recurrence, typed_emit, emit_valid_length, linear_effects, loop_induction_values, nested_control_flow) -> warn and continue. These are derived from the workflow spec, and this path derives its IO from the graph and never interprets that manifest. - Prescriptive (parameter_adapters, heterogeneous_adapter_batching) -> fatal. These are a concrete weight overlay, not a description. Adds a regression test using a structurally VALID adapter service, so the adapter guard is provably what rejects it rather than an unrelated structural defect, and tightens the structural test to assert the specific unknown-component defect instead of only the generic prefix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Review pointed out that nothing in the suite distinguished the explicit PRESCRIPTIVE capability list from a `metadata.adapters.is_some()` check, even though that distinction is the whole reason the list is keyed on capability names. `workflow_required_capabilities` also derives `parameter_adapters` (from a component whose adapter ABI or contract id is `onnx-genai.parameter-overlay`) and `heterogeneous_adapter_batching` (from adapter-segment/count/scale input roles) with `metadata.adapters` unset. Keying off the service would let those workflow-declared overlays through with only a warning, which carries the identical silent base-model-output risk. Adds a fixture that declares such an overlay with no adapter service, asserts `metadata.adapters.is_none()` so it provably exercises the workflow-derived path, and asserts the bail names `parameter_adapters`. Expands the code comment to record this rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
admit_inference_metadatafailed the load whenever the package declared a capability this runtime has not implemented. In practice that rejected every newly-exported mobius package, which declares eight workflow capabilities:The rule this PR applies
Not "capabilities are advisory" — that would be wrong, and review caught exactly why. The split is on one question:
workflow_ssa,serving_service_contract,bounded_state_recurrence,typed_emit,emit_valid_length,linear_effects,loop_induction_values,nested_control_flowparameter_adapters,heterogeneous_adapter_batchingmetadata.adaptersor the workflow specStructural defects remain fatal throughout.
Evidence for the descriptive half
I relaxed the gate locally and ran a previously-rejected package end-to-end on the native CUDA backend:
It loads, builds a native decode session, executes, and produces a coherent stream whose first token (
13847) matches the same model exported without a workflow manifest. The gate was refusing work that runs.Review then confirmed against the code that all eight derive from workflow spec shape (
validation.rs:157-330), and thatbounded_state_recurrencein particular comes fromworkflow.state[..].recurrence— not from the model's KV/attention geometry, which the decode path takes frommetadata.decoder_io()/ graph derivation without ever readingworkflow.state.Why the prescriptive half stays fatal
The two adapter capabilities are a concrete weight overlay, not a description. The only consumers of
.adaptersarepipeline/adapters.rsandpipeline/mod.rs— the separatePipelineRuntime, not this path. A well-formed LoRA package would otherwise have loaded here and silently produced base-model output: wrong output, not degraded output, with nothing for the caller to notice.They are keyed on capability name, not on
metadata.adapters.is_some(), becauseworkflow_required_capabilitiesderives the same two capabilities from the workflow spec with no adapter service present (validation.rs:201for adapter-segment/count/scale input roles,validation.rs:232for anonnx-genai.parameter-overlaycomponent). Anis_some()check would let those workflow-declared overlays through with only a warning — the exact bug this guards against. That case now has its own test.Scope
Both
admit_inference_metadatacallers are bare-decode (nativeload.rs:370, ORTload.rs:1188). The workflow/serving runtime (PipelineRuntime) does not call it, so this does not weaken a serving path.Complementary to #1723 (native backend for the universal
pipeline.workflowruntime): as that lands these capabilities become genuinely implemented and the warnings stop firing on their own. No file overlap — this touches onlyengine/load.rs.Per maintainer decision, no
kind:discriminator is added: the workflow shape is the direction for all packages, so there is nothing to discriminate between.Tests
cargo test -p onnx-genai-engine --lib→ 435 passed, 0 failed.unsupported_declared_capability_is_reported_but_does_not_block_decodeworkflow_manifest_capabilities_do_not_block_the_bare_decoderdeclared_adapters_fail_closed_because_this_path_cannot_apply_them— structurally valid adapter service (real target manifest, selection contract, artifact)workflow_declared_parameter_overlay_also_fails_closed_without_an_adapter_service— assertsmetadata.adapters.is_none(), pinning the case that makes the explicit capability list correctstructural_defect_still_fails_closed— asserts the specific unknown-component defect, not just the generic prefixThe two adapter tests each assert the error is not a structural one before asserting the guard message. That guard was not cosmetic: four successive drafts of those fixtures were structurally invalid (wrong field names,
component: decoderinstead ofmodel, empty artifacts, artifact paths outsideadapters/{name}/, unpinned adapter ABI) and would otherwise have "passed" for the wrong reason.Closes #1767