feat: expose MOSS valid-frame metadata in stable C ABI (#344) - #345
Closed
jajmangold wants to merge 2 commits into
Closed
feat: expose MOSS valid-frame metadata in stable C ABI (#344)#345jajmangold wants to merge 2 commits into
jajmangold wants to merge 2 commits into
Conversation
Additive MOSS-Audio/MOSS-Music encoder/tap/adapter metadata for the downstream MOSS-Music-8B-Thinking feature pipeline. All existing moss_audio_* symbols and behaviour are preserved. - moss_audio_plan_chunks(): pure per-chunk valid-frame bookkeeping (3x stride-2 conv over 400-frame chunks); the single source of valid counts shared by both encoder entrypoints, so metadata can never drift from the chunk loop. - moss_audio_compute_mel_meta(): reports the observed PRE-PAD mel length alongside the same padded tensor as compute_mel(); the pre-pad value is never inferred from padded zeros or a floor. - moss_audio_run_encoder_meta(): executes and trims using exactly T_mel_actual (validated 1..T_mel, fail-closed otherwise), returns only real-content 1280-D encoder/tap frames, and fills caller- allocated per-chunk valid counts with out_total_valid == out_T_enc == sum(valid_counts). Byte-identical to run_encoder() when no pad. - Load-time fail-closed check: llm.hidden_size must equal the adapter down_proj output-row shape; adapter out dim reported from the weight (ne[1]), not a hardcoded 2560. - Header docs: fixed stale 2560-only comment (4096 for MOSS-Music), ownership, synchronous copies, single-inflight/non-reentrancy, cached-encoder-graph invalidation. - Tests: hermetic CPU test (sum(valid)==total, sub-30s pad exclusion) plus a live differential test that skips without CRISPASR_MODEL_MOSS_AUDIO. Validation: full Release build green; hermetic test 208 assertions pass; clang-format-18 clean; trailmark entrypoints 17->20 (+3) with run_encoder complexity 38->4 (logic moved to shared impl).
…Strobe#344) B1 from adversarial review of 872ce72: moss_audio_run_encoder_impl frees tap buffers on its graph-alloc/compute/missing-tensor failure paths but left the caller-facing ds_results slots pointing at freed memory; both run_encoder wrappers then unconditionally published those slots into *ds_tap_0/1/2 even when returning NULL, handing callers dangling non-NULL pointers on failure. - Wrappers now write the tap out-pointers only on success (r != nullptr), restoring the pre-refactor contract: on failure *ds_tap_x are not modified, so they can never dangle. - Impl failure paths also null their ds_results slots alongside the frees as defense-in-depth. - Header docs state the failure contract for both entrypoints. - Hermetic regression test pins the wrapper invariant at the observable boundary (any NULL-returning call leaves caller tap pointers untouched); deep alloc/compute failures cannot be forced without a loaded model. Verified red against the pre-fix source, green post-fix. Validation: Release build green; hermetic test 220 assertions / 4 cases pass; trailmark structural diff shows only the two wrapper guards (complexity +1 each) and impl line spans, no call-graph or ABI changes.
Owner
|
Superseded by #349, which includes this PR plus the validation fixes and tests. |
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
2 participants
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.
Summary
Adds an additive, backward-compatible C ABI for truthful MOSS audio valid-frame metadata:
moss_audio_compute_mel_metareports the observed pre-padding mel-frame count while preserving the existing padded mel-buffer contract.moss_audio_plan_chunksexposes exact three-stage stride-2 chunk planning and per-chunk valid encoder-frame counts.moss_audio_run_encoder_metaexcludes synthetic pad frames, returns exact valid counts, and preserves byte-identical behavior whenT_mel_actual == T_mel.moss_audio_compute_mel,moss_audio_run_encoder, andmoss_audio_run_adaptersignatures remain unchanged.llm.hidden_sizeat load time.ds_tap_*output slots remain untouched; internal failure paths cannot publish freed tap buffers.Validation
c38739dand pass after it.CRISPASR_MODEL_MOSS_AUDIO; it should run with the governed MOSS audio model before final merge.A repository-wide CTest run also exposed unrelated environment/live-test failures (missing optional models,
/mnt/storagepermissions, absentpythonshim, and live server/model tests); the focused changed surface is green.Risk and rollback
Risk is limited to the additive MOSS audio ABI and its shared encoder implementation. The legacy entrypoints retain their signatures and success behavior. The strict adapter-dimension check intentionally refuses mislabeled/inconsistent GGUFs. Failure paths are safer because tap outputs are not published on NULL return.
Rollback is the two commits in this branch; no data migration or model artifact change is involved.
Closes #344