feat: add POST /score endpoint for lyric alignment scoring#95
Open
jeeftor wants to merge 11 commits into
Open
Conversation
Closes #1 Add a C++ implementation of lyric alignment scoring that captures cross-attention matrices from the DiT forward pass and computes coverage, monotonicity, and confidence metrics via DTW pathfinding. This eliminates the need for a Python sidecar — the entire scoring pipeline runs in-process with the ace-server binary. Ported from the Python ACE-Step scoring modules (pinned to ace-step/ACE-Step-1.5@82252c24): - acestep/core/scoring/_dtw.py (DTW + median filter) - acestep/core/scoring/dit_score.py (MusicLyricScorer metrics) New files: - src/dtw-score.h: Pure C++ port of DTW, median filter, token type mask generation, attention preprocessing (head selection, averaging, min-max normalization), and the full scoring pipeline (coverage, monotonicity, confidence -> lyrics_score = cov^2 * mono^2 * conf). Zero external dependencies beyond standard library headers. Each function has comments referencing the exact Python source file and line numbers for traceability. - tests/test-dtw-score.cpp: 9 unit tests (58 assertions) covering DTW pathfinding on diagonal/horizontal matrices, median filter spike removal with reflect padding, token type mask generation (bracket detection), full scoring pipeline with perfect/poor/tagged alignment, multi-head attention averaging, and DTW path monotonicity. All pass. - .github/workflows/scoring-drift-check.yml: Weekly CI check that fetches the upstream Python scoring files at the pinned commit and compares SHA256 hashes. Fails with a warning if the Python reference has drifted, signaling that the C++ port needs re-evaluation. Modified files: - src/dit-graph.h: dit_attn_f32() and dit_ggml_build_cross_attn() accept an optional capture_scores parameter that forces the f32 attention path (instead of flash_attn_ext) and marks the softmax attention weights as a graph output via ggml_set_output(). dit_ggml_build_graph() accepts score_layers[] to capture cross-attention from specific layers, naming each captured tensor "cross_attn_scores_L{layer}" for later retrieval. - src/dit-sampler.h: New dit_ggml_score_forward() function that builds a DiT graph with score capture, runs a single forward pass at t=0 (no denoising loop), and reads back the attention matrices for each captured layer. Returns [enc_S, S, Nh] per layer (batch 0 only). - src/pipeline-synth-impl.h: Added per_lyric_ids field to SynthState to persist lyric token IDs from text encoding through to scoring. - src/pipeline-synth-ops.cpp/h: ops_encode_text() now stores lyric token IDs in SynthState. New ops_score_forward() op runs the scoring forward pass, stacks attention from all captured layers into [n_layers, n_heads, enc_S, S] layout, decodes token IDs to strings via the BPE tokenizer for type mask generation, and calls calculate_lyric_score() to compute metrics. - src/pipeline-synth.h/cpp: New ace_synth_score() public API and LyricScoreResult struct (defined in dtw-score.h). Runs the same phase 1 setup as ace_synth_job_run_dit (text encoding, context build, noise init) but calls ops_score_forward() instead of the full denoising loop. Currently supports text2music task only. - tools/ace-server.cpp: New POST /score endpoint with score_worker and handle_score. Accepts the same JSON request format as /synth (plain JSON or multipart), creates an async job, and returns a JSON array of per-request scores: [{"coverage":1.0,"monotonicity":0.95,"confidence":0.24, "lyrics_score":0.21}, ...] Uses the existing job queue system (POST /score -> job ID -> GET /job?id=N -> GET /job?id=N&result=1). - tests/CMakeLists.txt: Added test-dtw-score build target (links libm, no ggml dependency). Verified end-to-end on Strix Halo (Radeon 8060S, Vulkan backend): - 58/58 unit tests pass - ace-server builds and runs with /score endpoint - Single scoring forward pass: ~106ms for 10s audio (T=250, S=125) - Cross-attention captured from 5 layers x 32 heads x [100 x 125] - JSON response correctly returned via job queue Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DTW-based lyric alignment scoring by capturing DiT cross-attention, integrating score-only synthesis APIs, exposing asynchronous ChangesLyric alignment scoring
Upstream scoring drift check
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handle_score
participant score_worker
participant ace_synth_score
participant dit_ggml_score_forward
participant calculate_lyric_score
Client->>handle_score: POST /score with request and predicted latents
handle_score->>score_worker: enqueue validated scoring job
score_worker->>ace_synth_score: process request batch
ace_synth_score->>dit_ggml_score_forward: run scoring forward passes
dit_ggml_score_forward-->>ace_synth_score: return captured attention
ace_synth_score->>calculate_lyric_score: compute DTW alignment metrics
calculate_lyric_score-->>score_worker: return lyric score comparisons
score_worker-->>Client: expose JSON result through job polling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeeftor
marked this pull request as draft
July 10, 2026 17:16
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/dtw-score.h (1)
202-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
DEFAULT_SCORE_HEADSlayer fields are absolute DiT layer numbers (2–6), but callers must index a compacted layer array.
preprocess_attentionusesconfig[c].layerdirectly as the index into the[n_layers, ...]attention buffer (Line 263-265) and drops any entry wherelayer >= n_layers. Theops_score_forwardcaller passes a buffer of onlyn_score_layers(5) slots indexed 0..4, so these absolute layer numbers do not line up. See the detailed integration comment onpipeline-synth-ops.cpp. Consider documenting here that the layer field must match the caller's layer-dim indexing convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dtw-score.h` around lines 202 - 205, Document in ScoreLayerHeadConfig and DEFAULT_SCORE_HEADS that layer values must use the compacted score-layer buffer indices supplied by ops_score_forward, not absolute DiT layer numbers; update DEFAULT_SCORE_HEADS to map the intended layers to indices 0–4 and verify preprocess_attention’s direct config[c].layer indexing remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scoring-drift-check.yml:
- Around line 19-21: Add a job-level timeout to the `check` job in the scoring
drift workflow by setting `timeout-minutes` to 5 alongside `runs-on`, ensuring
the network-based hash checks cannot consume runner time indefinitely.
- Around line 13-21: Add an explicit top-level permissions block with no granted
scopes to the workflow containing the check job, placing it alongside on and
jobs. Use permissions: {} so the workflow does not inherit broader GITHUB_TOKEN
access.
In `@src/pipeline-synth-ops.cpp`:
- Around line 1026-1097: The scoring loop in dit_ggml_score_forward reuses
batch-0 attention for every batch item; restrict this scoring path to batch_n ==
1, or update attention capture and stacking to store and select tensors per
batch item before calling calculate_lyric_score. Ensure out_scores entries for
unsupported batch sizes are handled explicitly rather than mixing batch-0
attention with other decoded_tokens.
- Around line 1059-1091: The compact attention stack uses indices 0..4 while
DEFAULT_SCORE_HEADS references absolute layer IDs, causing incorrect layer
selection and skipped heads. Before calculate_lyric_score, remap the configured
score-head layer IDs to their compact positions based on the captured layer
mapping, and ensure preprocess_attention receives and uses the remapped heads
consistently.
- Around line 1069-1086: Fix the source indexing in the attention-stacking loop
identified by layer_attentions and stacked: cross_attn_scores_L* uses [enc_S, S,
Nh, N] with batch 0, so update src_idx to reflect enc_S as the innermost
dimension rather than Nh being fastest. Keep the destination layout [n_layers,
n_heads, enc_S, S] unchanged and ensure bounds checking remains correct.
In `@tests/test-dtw-score.cpp`:
- Around line 198-226: Fix the malformed structural-tag token in
test_scoring_with_tags by replacing the standalone "[" entry in decoded with
"[Intro]" so generate_token_type_mask produces [1, 0, 1, 1, 1]. Keep the
five-token setup and existing assertions, ensuring the test actually validates
four lyric tokens and one structural tag.
In `@tools/ace-server.cpp`:
- Around line 1424-1435: Update the output-format comment above the JSON
construction in the score handler to describe a bare array of score objects
(`[{"coverage":...}, ...]`) rather than an object containing a “scores” field,
matching the `yyjson_mut_arr` root and the `handle_score` documentation.
- Around line 1509-1520: The array-element serialization in the request parsing
loop leaks memory and redundantly retries parsing. In the loop handling
array-form requests, update the logic around request_parse_json to serialize
yyjson_val obj once, retain the returned buffer, parse it, then free it on every
path; remove the fallback reserialization and second parse.
---
Nitpick comments:
In `@src/dtw-score.h`:
- Around line 202-205: Document in ScoreLayerHeadConfig and DEFAULT_SCORE_HEADS
that layer values must use the compacted score-layer buffer indices supplied by
ops_score_forward, not absolute DiT layer numbers; update DEFAULT_SCORE_HEADS to
map the intended layers to indices 0–4 and verify preprocess_attention’s direct
config[c].layer indexing remains valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3fc92c6-1c3a-4148-ade5-405e4c5cc124
📒 Files selected for processing (12)
.github/workflows/scoring-drift-check.ymlsrc/dit-graph.hsrc/dit-sampler.hsrc/dtw-score.hsrc/pipeline-synth-impl.hsrc/pipeline-synth-ops.cppsrc/pipeline-synth-ops.hsrc/pipeline-synth.cppsrc/pipeline-synth.htests/CMakeLists.txttests/test-dtw-score.cpptools/ace-server.cpp
- Remove vacuous comment about LyricScoreResult include location
- Fix misleading t=0 comment ("final denoised state" → "matches Python reference")
- Replace UB pointer sentinel (ggml_tensor*)1 with a plain bool flag
- Fix memory leak: yyjson_val_write in array-parse if-condition was never freed; collapse to a single alloc+free
- Use ::error:: instead of ::warning:: in drift-check workflow (was emitting a warning annotation on a failing job)
- Document that batch_n > 9 limit matches ace_synth_job_run
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bapse2pC9C1iJyEFFQ4svj
Require generated latents, score pure-noise and regressed states, and correct per-batch attention and layer selection. Harden model metadata, request parsing, drift checks, and CI test coverage.
jeeftor
marked this pull request as ready for review
July 10, 2026 18:56
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/model-store.cpp`:
- Around line 60-98: Update dit_meta_load_alignment_config to mark malformed
alignment metadata invalid: treat JSON parse failures, non-object roots, and a
present non-object lyric_alignment_layers_config as invalid, while retaining
architecture-default fallback only when the configuration key is absent. Ensure
yyjson_doc cleanup remains safe for all paths and prevent defaults from being
applied when invalid is true.
In `@src/pipeline-synth-ops.h`:
- Around line 83-87: Propagate the job cancellation signal through
ace_synth_score() into ops_score_forward() using the existing cancellation
state, checking it before starting and between the two DiT forward passes so
scoring exits promptly. Update the /score worker’s completion handling to return
CANCELLED instead of DONE whenever job->cancel is set, including cancellation
observed after scoring completes.
In `@tools/ace-server.cpp`:
- Around line 1450-1456: Ensure the score worker checks job->cancel after
ace_synth_score returns and before serializing results or publishing the final
status; mark the job CANCELLED and exit without emitting a result when
cancellation was requested. If supported by the scoring implementation, also
propagate cancellation through ace_synth_score for mid-score interruption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6d95169-ac72-432c-9caf-c8e603b4994d
📒 Files selected for processing (18)
.github/workflows/ci-build.yml.github/workflows/scoring-drift-check.ymlCMakeLists.txtREADME.mddocs/ARCHITECTURE.mdsrc/dit-graph.hsrc/dit-sampler.hsrc/dtw-score.hsrc/model-store.cppsrc/model-store.hsrc/pipeline-synth-impl.hsrc/pipeline-synth-ops.cppsrc/pipeline-synth-ops.hsrc/pipeline-synth.cppsrc/pipeline-synth.htests/CMakeLists.txttests/test-dtw-score.cpptools/ace-server.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/CMakeLists.txt
- src/pipeline-synth-impl.h
- src/dit-graph.h
Author
|
@coderabbitai run pre-merge checks |
Contributor
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
Author
|
@coderabbitai run pre-merge checks |
Contributor
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
Author
|
@coderabbitai Please list every C++ function currently counted as undocumented by the Docstring Coverage pre-merge check on this PR, and state the exact C++ documentation-comment syntax that this check recognizes. Do not make changes. |
Contributor
|
What I can tell you:
If you want, I can trigger a fresh pre-merge check run now in case the earlier ones are stale, or if you paste the specific Docstring Coverage section content here, I can help interpret it or reconcile it against the actual C++ functions added in this PR (e.g., (＾・ω・＾ ) ✅ Action performedReviews paused. |
Author
|
@coderabbitai resume |
Author
|
@coderabbitai full review |
Contributor
✅ Action performedReviews resumed. |
Contributor
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 20 minutes. |
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test-dtw-score.cpp (1)
67-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_dtw_horizontal_stepdoesn't verify the path sequence, allowing the DTW tie-breaking bug to go undetected.The test only checks
front()andback()of the path. With the current tie-breaking bug (see comment ondtw_cpu), the path[(0,0), (1,0), (1,1), (1,2)](cost 18) passes the same assertions as the optimal path[(0,0), (0,1), (0,2), (1,2)](cost 0).Consider asserting the full path or at least the path length to ensure the DTW selects the correct route.
♻️ Proposed enhancement
static void test_dtw_horizontal_step() { // 2x3 cost matrix: // [[0, 0, 0], // [9, 9, 0]] // The optimal path should go right along row 0, then diagonal to (1,2). float cost[] = { 0, 0, 0, 9, 9, 0 }; DTWPath path = dtw_cpu(cost, 2, 3); // Path must start at (0,0) and end at (1,2) CHECK(!path.text_idx.empty(), "horizontal DTW path non-empty"); CHECK(path.text_idx.front() == 0, "horizontal DTW starts at text=0"); CHECK(path.time_idx.front() == 0, "horizontal DTW starts at time=0"); CHECK(path.text_idx.back() == 1, "horizontal DTW ends at text=1"); CHECK(path.time_idx.back() == 2, "horizontal DTW ends at time=2"); + + // Optimal path stays on row 0 (cost 0) then drops to row 1 at the end. + // Path length should be 4: (0,0) -> (0,1) -> (0,2) -> (1,2) + CHECK(path.text_idx.size() == 4, "horizontal DTW path length == 4"); + CHECK(path.text_idx[0] == 0 && path.time_idx[0] == 0, "horizontal DTW step 0"); + CHECK(path.text_idx[1] == 0 && path.time_idx[1] == 1, "horizontal DTW step 1"); + CHECK(path.text_idx[2] == 0 && path.time_idx[2] == 2, "horizontal DTW step 2"); + CHECK(path.text_idx[3] == 1 && path.time_idx[3] == 2, "horizontal DTW step 3"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-dtw-score.cpp` around lines 67 - 81, Strengthen test_dtw_horizontal_step by asserting the complete expected path sequence, not just its endpoints: (0,0), (0,1), (0,2), then (1,2). At minimum, verify the path length and each corresponding text_idx/time_idx entry so the tie-breaking bug cannot pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scoring-drift-check.yml:
- Around line 25-34: Add a curl-level total retry duration cap to the CURL
options in the “Fetch upstream Python scoring files” step by including
--retry-max-time with a value that keeps the worst-case download duration within
the workflow’s 5-minute timeout, while retaining the existing per-attempt
timeout and retries.
In `@src/dtw-score.h`:
- Line 383: Update preprocess_attention’s layer/head validation to require both
values are non-negative as well as less than n_layers and n_heads, preventing
invalid pointer access when called directly; retain or align remap_score_heads
validation as appropriate.
- Around line 115-124: Fix predecessor selection in the DTW cost recurrence by
changing the comparisons in the shown branch to an inclusive minimum chain,
preserving the intended diagonal → up → left tie-breaking order. Update the
logic in the DTW computation in src/dtw-score.h so equal costs select a minimum
predecessor rather than falling through to c2, and add or extend a test to
verify the accumulated cost and alignment quality for tied predecessors.
---
Nitpick comments:
In `@tests/test-dtw-score.cpp`:
- Around line 67-81: Strengthen test_dtw_horizontal_step by asserting the
complete expected path sequence, not just its endpoints: (0,0), (0,1), (0,2),
then (1,2). At minimum, verify the path length and each corresponding
text_idx/time_idx entry so the tie-breaking bug cannot pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e3fea9a-ec1a-4a10-a089-dc1e43b3a24e
📒 Files selected for processing (18)
.github/workflows/ci-build.yml.github/workflows/scoring-drift-check.ymlCMakeLists.txtREADME.mddocs/ARCHITECTURE.mdsrc/dit-graph.hsrc/dit-sampler.hsrc/dtw-score.hsrc/model-store.cppsrc/model-store.hsrc/pipeline-synth-impl.hsrc/pipeline-synth-ops.cppsrc/pipeline-synth-ops.hsrc/pipeline-synth.cppsrc/pipeline-synth.htests/CMakeLists.txttests/test-dtw-score.cpptools/ace-server.cpp
Author
|
@coderabbitai run pre-merge checks |
Contributor
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
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
1 participant
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 asynchronous
POST /scoreendpoint for lyric-alignment scoring ofgenerated DiT latents. It captures selected DiT cross-attention layers and
returns LM and DiT alignment metrics (coverage, monotonicity, confidence, and
combined lyric score) through the existing job queue.
Closes jeeftor#1.
API
POST /scoreacceptsmultipart/form-data:request: oneAceRequestJSON object or an array of requests.pred_latents: raw little-endian f32 data in[batch, T, 64]layout.latentis accepted as an alias.The endpoint immediately returns a job ID. Poll
GET /job?id=<id>and fetchthe completed bare JSON array with
GET /job?id=<id>&result=1.See the README
and architecture reference
for the current request and response contract.
Implementation
fused attention path that discards those weights.
validates metadata and score-head indices, and propagates job cancellation.
check for the upstream ACE-Step scoring modules.
Reference behavior
The C++ implementation is based on ACE-Step-1.5 commit
82252c24. The driftworkflow detects changes to the pinned upstream source files. It does not claim
bit-for-bit output parity: the DTW predecessor selection intentionally corrects
a strict-comparison bug in that snapshot so tied predecessors always select a
minimum-cost path.
Validation
clang-format --dry-run --Werroron all modified C/C++ filescmake --build /private/tmp/acestep-pr95-fix-build --parallel 4ctest --test-dir /private/tmp/acestep-pr95-fix-build --output-on-failure(
test-dtw-score: 86 passed, 0 failed)End-to-end model execution remains to be validated on a Linux GPU host. The
upstream CI workflow requires maintainer approval because this PR originates
from a fork.