fix: honor MSST MDXC inference defaults - #299
Open
HAGerox wants to merge 2 commits into
Open
Conversation
WalkthroughMDXC overlap and batch size now default to model inference metadata, with fallbacks of 8 and 1. Remote interfaces omit unset values, explicit overrides remain supported, and RoFormer chunk steps use the overlap divisor. ChangesMDXC defaults and chunking
Remote parameter propagation
CLI and documentation defaults
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RemoteAPI
participant MDXCSeparator
participant ModelConfig
Caller->>RemoteAPI: submit optional overlap and batch_size
RemoteAPI->>MDXCSeparator: pass omitted or explicit values
MDXCSeparator->>ModelConfig: read inference settings
ModelConfig-->>MDXCSeparator: return configured values or fallbacks
MDXCSeparator-->>Caller: process audio with resolved settings
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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 `@audio_separator/separator/architectures/mdxc_separator.py`:
- Around line 96-102: Validate the resolved values in the MDXC initialization
flow before assigning self.overlap and self.batch_size: use the inference
defaults for YAML null values, and reject any non-positive explicit or fallback
result before storing it. Apply the same validation to both settings and add
tests covering zero, negative, and null YAML values.
In `@audio_separator/separator/separator.py`:
- Line 128: Update Separator.__init__ so mdxc_params, mdx_params, vr_params, and
demucs_params default to None rather than shared dictionary objects, then
instantiate a fresh default dictionary for each parameter when its value is
omitted while preserving caller-provided dictionaries.
🪄 Autofix
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: 6eb653e8-54d2-401e-b9a5-5f59e8640e60
📒 Files selected for processing (12)
README.mdaudio_separator/remote/api_client.pyaudio_separator/remote/cli.pyaudio_separator/remote/deploy_cloudrun.pyaudio_separator/remote/deploy_modal.pyaudio_separator/separator/architectures/mdxc_separator.pyaudio_separator/separator/separator.pyaudio_separator/utils/cli.pytests/unit/test_cli.pytests/unit/test_mdxc_config.pytests/unit/test_mdxc_roformer_chunking.pytests/unit/test_remote_api_client.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_mdxc_config.py (1)
58-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test name with the covered configuration sources.
Lines 61-68 validate both architecture and model inference values, but
test_mdxc_rejects_non_positive_inference_valuesnames only inference values. Rename it totest_mdxc_rejects_non_positive_config_values, or split the cases into source-specific tests.Proposed rename
-def test_mdxc_rejects_non_positive_inference_values(arch_config, inference_config, message): +def test_mdxc_rejects_non_positive_config_values(arch_config, inference_config, message):🤖 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/unit/test_mdxc_config.py` around lines 58 - 73, Rename test_mdxc_rejects_non_positive_inference_values to test_mdxc_rejects_non_positive_config_values so the name accurately covers invalid values from both architecture and inference configurations; leave the parameterized cases unchanged.
🤖 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.
Nitpick comments:
In `@tests/unit/test_mdxc_config.py`:
- Around line 58-73: Rename test_mdxc_rejects_non_positive_inference_values to
test_mdxc_rejects_non_positive_config_values so the name accurately covers
invalid values from both architecture and inference configurations; leave the
parameterized cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04234348-0047-4fca-8f0b-2b8028fadf68
📒 Files selected for processing (2)
audio_separator/separator/architectures/mdxc_separator.pytests/unit/test_mdxc_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- audio_separator/separator/architectures/mdxc_separator.py
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
PAS currently ignores the MDXC/RoFormer inference defaults in a model's YAML unless the user happens to supply the same values manually. It also interprets RoFormer overlap in different units from the model configuration and MSST.
This PR makes PAS use the model YAML's
inference.num_overlapandinference.batch_sizevalues by default, while preserving explicit user overrides and retaining PAS's existing fallback values for older YAML files that do not contain them.What was wrong
PAS was using the right-looking setting in the wrong way.
An MDXC model YAML can contain:
In MSST,
num_overlapdescribes how many overlapping prediction windows fit into a chunk. The distance between chunk starts is therefore calculated as:For an 11-second chunk and
num_overlap: 2, the next chunk starts 5.5 seconds later. That gives the intended 50% overlap.PAS did two different things instead:
mdxc_overlap=8, so the model's YAML value was not used.For the same 11-second chunk, PAS therefore interpreted
8as an 8-second step. Only 3 seconds were shared between adjacent chunks, or about 27.3% overlap.The numerical values were especially misleading because identical numbers meant very different schedules:
28This meant PAS could appear substantially faster while not performing the inference schedule requested by the model author. It was doing less overlapping model work, which can also change separation quality.
Performance impact
Correcting the calculation significantly reduces the apparent throughput compared with the previous, non-equivalent PAS behavior.
For a 126.71-second test input using an 11-second RoFormer chunk:
The previous schedule performed 16 model forwards, while the intended overlap-2 schedule performed 25. That is 56.25% more model inference work before accounting for device throughput, memory movement, or benchmark noise.
The two corrected runs varied enough that 207–225 seconds should be treated as an approximate range rather than a definitive stable benchmark. However, the direction of the performance change is expected: the old 88-second result was faster because PAS was not applying the requested overlap, not because it was processing an equivalent workload more efficiently.
Changes
inference.num_overlap/inference.batch_size);8/1) when the YAML does not specify a value.Scope
The incorrect seconds-based overlap calculation was in the RoFormer branch of the MDXC separator. The configuration precedence change applies to MDXC models so their existing YAML inference values are honored. Other architecture implementations are unchanged.
Testing
293 passed, 7 skipped, 1 deselectedgit diff --checkpassed.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation