fix: make same-seed generations reproducible (LM sampling + reference-audio cropping) - #1283
fix: make same-seed generations reproducible (LM sampling + reference-audio cropping)#1283tsondo wants to merge 3 commits into
Conversation
The generation seed was only applied to the DiT diffusion stage; the 5Hz LM planning stage sampled unseeded, so two runs with the same seed produced entirely different songs (different CoT metadata and audio codes) whenever thinking/CoT was enabled. Gaps closed: - _run_vllm accepted `seeds` but never used it. nano-vllm's sampler draws from the global torch generator (exponential_ in sampler.py), which was never seeded. Now seeds torch/CUDA RNG before generate. nano-vllm samples all prompts in one stream, so the batch is seeded once with seeds[0]. - _run_pt and _run_mlx seeded per-item in batch mode but not in single mode (the batch_size=1 default). Now both single paths seed too. - Phase 1 (CoT) and single-mode Phase 2 (codes) went through generate_from_formatted_prompt, which had no seeds plumbing at all, on every backend. Seeds are now passed via the cfg dict and forwarded to all three backends. Behavior is unchanged when no seeds are provided. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ility Fork commit 4063a4c plumbs the generation seed into the 5Hz LM planning stage (nano-vllm/pt/mlx backends, CoT and codes phases). Previously only the DiT diffusion stage was seeded, so same-seed generations produced entirely different songs whenever Planning intelligence was enabled. Submitted upstream as ace-step/ACE-Step-1.5#1283. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOptional seeds now pass through formatted-prompt generation, CoT, code generation, and backend dispatch. vLLM, PyTorch, and MLX paths initialize RNG state from the first seed. Reference-audio segment sampling also uses the resolved first seed. ChangesSeed reproducibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant generate_from_formatted_prompt
participant Backend
participant RNG
participant ReferenceAudio
Caller->>generate_from_formatted_prompt: provide optional seed list
generate_from_formatted_prompt->>Backend: forward seed list
Backend->>RNG: initialize backend RNG from first seed
RNG-->>Backend: seeded sampling state
Caller->>ReferenceAudio: provide resolved first seed
ReferenceAudio->>ReferenceAudio: select reference segments with local RNG
Backend-->>Caller: generate output
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@acestep/llm_inference.py`:
- Line 2439: Split the oversized llm_inference module by moving backend-specific
generation and related dispatch/configuration logic, including the seeds
extraction flow, into responsibility-specific modules while preserving the
existing facade imports and behavior; if this cannot be completed here, add a
concrete follow-up plan in the PR notes describing the split.
- Line 1417: Update the `generate_with_stop_condition` docstring and the
corresponding second `seeds` documentation to accurately describe that seeds are
forwarded for CoT generation and single-mode code generation, explain their
batch behavior, and document the vLLM limitation; remove the outdated “only when
batch_size > 1” and “not used yet” wording.
- Around line 4096-4098: Update the hybrid fallback setup around _sample_tokens
to seed PyTorch with seeds[0] when seeds are provided, in addition to the
existing MLX seed. Apply the same PyTorch seeding before the sequential batch
path invokes _run_mlx_single, preserving the current behavior when no seeds are
supplied.
- Around line 910-918: Update the vLLM batch path in
generate_with_stop_condition to preserve per-item seed semantics instead of
seeding the entire batch with seeds[0]. Either generate each item with its
corresponding seeds[i], or explicitly introduce a single-seed-per-batch contract
and reject normalized per-item seed lists before generation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b0d7eb1-9785-49de-b634-58dcfee9df86
📒 Files selected for processing (1)
acestep/llm_inference.py
| # Seed torch RNG for reproducibility. nano-vllm's sampler draws from the | ||
| # global torch generator; it samples all prompts in a single stream, so | ||
| # per-item seeding is not possible — seeding once with the first seed | ||
| # still makes same-seed runs reproducible. | ||
| if seeds: | ||
| torch.manual_seed(seeds[0]) | ||
| if torch.cuda.is_available(): | ||
| torch.cuda.manual_seed_all(seeds[0]) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'actual_batch_size|seeds\[0\]|seeds\[i\]|_run_vllm|_run_pt|_run_mlx' \
acestep/llm_inference.pyRepository: ace-step/ACE-Step-1.5
Length of output: 21089
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the normalized inputs around the vLLM API so the seed-list semantics are clear.
sed -n '840,1010p' acestep/llm_inference.py
printf '\n--- generate_with_stop_condition signature and seed normalization ---\n'
sed -n '1320,1390p' acestep/llm_inference.py
# Locate the public entry point names and any seed length assumptions.
python3 - <<'PY'
import ast
from pathlib import Path
p = Path("acestep/llm_inference.py")
tree = ast.parse(p.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "generate_with_stop_condition":
print("function", node.name, "line", node.lineno)
print("args", [arg.arg for arg in node.args.args])
print("seed normalizations:")
for sub in ast.walk(node):
if isinstance(sub, ast.If):
src = ast.get_source_segment(p.read_text(), sub)
if src and "seeds" in src:
print(src[:700])
PYRepository: ace-step/ACE-Step-1.5
Length of output: 15189
Preserve per-item seed semantics in the vLLM batch path.
generate_with_stop_condition normalizes seeds to one value per batch item, and the PyTorch/MLX batch paths use seeds[i]. This path seeds all vLLM output with seeds[0], so changing seeds[1:] cannot change the corresponding outputs. Implement per-item generation semantics for vLLM or define a separate single-seed-per-batch API and reject per-item seed lists.
🤖 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 `@acestep/llm_inference.py` around lines 910 - 918, Update the vLLM batch path
in generate_with_stop_condition to preserve per-item seed semantics instead of
seeding the entire batch with seeds[0]. Either generate each item with its
corresponding seeds[i], or explicitly introduce a single-seed-per-batch contract
and reject normalized per-item seed lists before generation.
| caption = cfg.get("caption", "") | ||
| lyrics = cfg.get("lyrics", "") | ||
| cot_text = cfg.get("cot_text", "") | ||
| seeds = cfg.get("seeds") # Optional list of seeds for reproducible sampling |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split the oversized module or add the required follow-up plan.
acestep/llm_inference.py is over 4,000 lines. This exceeds the 200-LOC hard cap. The new seed extraction and backend dispatch further couple configuration logic to three backend implementations.
Move backend-specific generation into responsibility-specific modules. Preserve stable facade imports. If the split cannot happen in this PR, add a concrete follow-up split plan to the PR notes.
As per coding guidelines, Python modules have a target size of 150 LOC and a hard cap of 200 LOC. Based on learnings, module-size findings apply when a file exceeds 200 LOC; this file exceeds that threshold.
🤖 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 `@acestep/llm_inference.py` at line 2439, Split the oversized llm_inference
module by moving backend-specific generation and related dispatch/configuration
logic, including the seeds extraction flow, into responsibility-specific modules
while preserving the existing facade imports and behavior; if this cannot be
completed here, add a concrete follow-up plan in the PR notes describing the
split.
Sources: Coding guidelines, Learnings
process_reference_audio() samples three random 10s windows from the reference track with unseeded random.randint, so two generations with identical parameters and seed conditioned on different slices of the reference audio — producing audibly different songs whenever a reference track was supplied. The generation seed (actual_seed_list[0]) is now threaded into the segment sampling via a local random.Random instance, so same-seed runs condition on the same reference slices. Behavior is unchanged when no seed is provided, and the global RNG is never touched. Verified end-to-end: two API generations with the same seed and a reference audio file now produce bit-identical output; a different seed produces different output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fork commit d7c3e69: process_reference_audio() cropped three random 10s windows from the reference track with unseeded RNG, so same-seed generations conditioned on different reference slices — the remaining source of run-to-run variation after the LM seeding fix. Verified two same-seed API generations with reference audio are now bit-identical. Added to upstream PR ace-step/ACE-Step-1.5#1283. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tsondo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
acestep/core/generation/handler/io_audio.py (1)
161-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for deterministic segment selection.
Use an audio fixture with distinct front, middle, and back regions. Call
process_reference_audiotwice with the same seed and assert that the returned tensors are equal. This protects the same-seed reproducibility contract.Based on the PR objective, same-seed reference-audio conditioning must be reproducible.
🤖 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 `@acestep/core/generation/handler/io_audio.py` around lines 161 - 169, Add a regression test for process_reference_audio using an audio fixture with distinguishable front, middle, and back regions; invoke it twice with the same seed and assert the returned tensors are equal, preserving deterministic reference-audio segment selection.acestep/core/generation/handler/generate_music_request.py (2)
109-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSeparate reference-audio and source-audio preparation.
_prepare_reference_and_source_audioprocesses two independent inputs, applies task-specific validation, and builds error payloads. Extract reference and source helpers. Keep this method as a small coordinator so the new seed flow remains local to reference-audio processing.As per coding guidelines, functions must have one responsibility and explicit side effects.
🤖 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 `@acestep/core/generation/handler/generate_music_request.py` around lines 109 - 114, Refactor _prepare_reference_and_source_audio into a small coordinator by extracting reference-audio preparation and source-audio preparation into separate helpers. Move each input’s processing, task-specific validation, and error-payload construction into its corresponding helper, while keeping seed handling local to the reference-audio helper and preserving the coordinator’s existing return behavior.Source: Coding guidelines
109-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
seedcontract.The modified helper adds
seed, but its docstring does not describe the input or return tuple. AddArgsandReturnssections. State thatseedcontrols deterministic reference-audio segment selection.Proposed docstring update
- """Prepare reference/source audio tensors and return early error payload when invalid.""" + """Prepare reference/source audio tensors and return errors for invalid input. + + Args: + seed: Optional seed for deterministic reference-audio segment selection. + + Returns: + Reference tensors, processed source audio, and an optional error payload. + """As per coding guidelines, modified Python functions must document purpose, key inputs, outputs, and relevant exceptions.
🤖 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 `@acestep/core/generation/handler/generate_music_request.py` around lines 109 - 114, Update the docstring of the modified helper containing the seed parameter to add Args and Returns sections: document seed as controlling deterministic reference-audio segment selection, describe the relevant inputs, and specify the returned three-element tuple containing processed tensors or an error payload. Include any relevant exceptions only if this helper explicitly raises them.Source: Coding guidelines
🤖 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 `@acestep/core/generation/handler/generate_music.py`:
- Line 334: Reduce both modules below the 200-line hard cap while preserving
their stable facade imports: in
acestep/core/generation/handler/generate_music.py (lines 334-334), extract
orchestration or reference/source preparation into a focused module; in
acestep/core/generation/handler/generate_music_request.py (lines 109-114),
extract request normalization or audio preparation into a focused module. Update
internal imports and retain the existing public entry points; alternatively,
include a concrete follow-up split plan in the PR.
---
Nitpick comments:
In `@acestep/core/generation/handler/generate_music_request.py`:
- Around line 109-114: Refactor _prepare_reference_and_source_audio into a small
coordinator by extracting reference-audio preparation and source-audio
preparation into separate helpers. Move each input’s processing, task-specific
validation, and error-payload construction into its corresponding helper, while
keeping seed handling local to the reference-audio helper and preserving the
coordinator’s existing return behavior.
- Around line 109-114: Update the docstring of the modified helper containing
the seed parameter to add Args and Returns sections: document seed as
controlling deterministic reference-audio segment selection, describe the
relevant inputs, and specify the returned three-element tuple containing
processed tensors or an error payload. Include any relevant exceptions only if
this helper explicitly raises them.
In `@acestep/core/generation/handler/io_audio.py`:
- Around line 161-169: Add a regression test for process_reference_audio using
an audio fixture with distinguishable front, middle, and back regions; invoke it
twice with the same seed and assert the returned tensors are equal, preserving
deterministic reference-audio segment selection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 216aedb1-9cf4-4b6e-8b77-114d4faaeb22
📒 Files selected for processing (3)
acestep/core/generation/handler/generate_music.pyacestep/core/generation/handler/generate_music_request.pyacestep/core/generation/handler/io_audio.py
| actual_batch_size=actual_batch_size, | ||
| task_type=task_type, | ||
| flow_edit_morph=flow_edit_morph, | ||
| seed=actual_seed_list[0] if actual_seed_list else None, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Bring both generation modules under the 200-line hard cap.
Both modules exceed the hard cap. Split responsibilities into focused modules, preserve stable facade imports, or add a concrete follow-up split plan.
acestep/core/generation/handler/generate_music.py#L334-L334: extract orchestration or reference/source preparation responsibilities.acestep/core/generation/handler/generate_music_request.py#L109-L114: extract request normalization or audio preparation responsibilities.
As per coding guidelines, Python modules have a 200-line hard cap unless the PR includes a concrete split plan. Based on learnings, module-size findings apply only above 200 LOC.
📍 Affects 2 files
acestep/core/generation/handler/generate_music.py#L334-L334(this comment)acestep/core/generation/handler/generate_music_request.py#L109-L114
🤖 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 `@acestep/core/generation/handler/generate_music.py` at line 334, Reduce both
modules below the 200-line hard cap while preserving their stable facade
imports: in acestep/core/generation/handler/generate_music.py (lines 334-334),
extract orchestration or reference/source preparation into a focused module; in
acestep/core/generation/handler/generate_music_request.py (lines 109-114),
extract request normalization or audio preparation into a focused module. Update
internal imports and retain the existing public entry points; alternatively,
include a concrete follow-up split plan in the PR.
Sources: Coding guidelines, Learnings
- MLX single and sequential paths now also seed torch: the hybrid
fallback samples via torch.multinomial (_sample_tokens), which
mx.random.seed does not cover.
- _run_vllm logs when per-item seeds are provided, since the vllm
backend can only seed the whole batch with seeds[0].
- Correct the stale generate_with_stop_condition seeds docstring
("TODO: not used yet") and document the per-backend contract.
- Document the seed parameter of _prepare_reference_and_source_audio.
- Add a regression test asserting process_reference_audio returns
identical tensors for the same seed and different tensors for
different seeds.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in the latest commit:
Skipped as out of scope for this fix: splitting 🤖 Generated with Claude Code |
1 participant
Problem
Two runs with the same seed and identical settings produce entirely different songs. The generation seed was only applied to the DiT diffusion stage; two other stochastic stages sampled unseeded randomness:
thinkingor CoT is enabled (the default).process_reference_audio()samples three random 10s windows (front/middle/back) from the reference track with unseededrandom.randint, so every run conditions on different slices of the reference audio.Root causes and fixes
Commit 1: LM sampling
_run_vllmacceptsseedsbut never uses it. nano-vllm's sampler draws from the global torch generator (probs.div_(torch.empty_like(probs).exponential_(1))innanovllm/layers/sampler.py), which nothing seeds. Now seeds torch/CUDA RNG beforellm.generate. nano-vllm samples all prompts in a single stream, so per-item seeding isn't possible — the batch is seeded once withseeds[0], which still makes same-seed runs reproducible._run_ptand_run_mlxonly seed in batch mode. The single-mode paths (thebatch_size=1default) skip seeding entirely. Now both single paths seed withseeds[0], mirroring each backend's existing batch-path pattern.generate_from_formatted_prompthad no seeds plumbing at all, and both Phase 1 (CoT) and single-mode Phase 2 (codes) go through it — so those phases were unseeded on every backend. Seeds are now passed via thecfgdict and forwarded to all three backends.Commit 2: reference-audio cropping
The generation seed (
actual_seed_list[0]) is threaded intoprocess_reference_audio()via a localrandom.Randominstance, so same-seed runs condition on the same reference slices. The global RNG is never touched.Verification
Tested end-to-end against a running API server (Linux, single RTX 5090, nano-vllm backend): two
/release_taskgenerations with the same seed — including a reference audio file and CoT metadata generation — now produce bit-identical audio (same SHA-256); a different seed produces different audio.Behavior is unchanged when no seeds are provided (all new code is behind
if seeds:/seed is not None).Caveat: bit-exact reproducibility still assumes the usual determinism constraints (same hardware, same batch size); these changes make the sampling RNG deterministic, which is the dominant source of run-to-run variation.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests