Skip to content

fix(cli): honor lowercase --sd_switch values - #221

Open
Minh3132 wants to merge 2 commits into
modelscope:mainfrom
Minh3132:fix/cli-sd-switch-case-54
Open

Minh3132 wants to merge 2 commits into
modelscope:mainfrom
Minh3132:fix/cli-sd-switch-case-54

Conversation

@Minh3132

@Minh3132 Minh3132 commented Sep 14, 2026

Copy link
Copy Markdown

Summary

Fix CLI speaker diarization switch handling so that the documented lowercase value:

--sd_switch yes

actually enables speaker diarization.

Previously, the CLI accepted lowercase yes/no, while the runtime logic only treated "Yes" as enabled. As a result, using --sd_switch yes could silently leave speaker diarization disabled.

Changes

  • Make sd_switch handling case-insensitive.
  • Ensure CLI --sd_switch yes enables speaker diarization.
  • Ensure --sd_switch no keeps it disabled.
  • Preserve compatibility with the existing UI Yes/No values.
  • Add regression tests covering CLI and UI variants.

Validation

Regression coverage includes:

  • lowercase CLI yes
  • lowercase CLI no
  • default disabled behavior
  • existing Yes/No UI compatibility

The focused validation reported 10/10 relevant tests passing (3 new regression tests + 7 existing compatibility tests).

This validation is scoped to the switch-handling regression and does not claim full end-to-end diarization, browser, or video export validation.

Fixes #54

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked exact head ad11daa against base 9e72061. The three submitted CLI-switch tests plus seven existing recognition-result compatibility tests pass (10 total). Running the same three new tests on the unchanged base gives one expected assertion failure: parsed --sd_switch yes requests no speaker results; the no/default and existing Yes/No controls pass.

The tested candidate now forwards return_spk_res=True for lowercase yes, retains False for no/default, and preserves the existing UI Yes/No behavior. I loaded the hash-verified candidate module under both videoclipper import names with unchanged base helpers and real CPU numerical dependencies. The ASR boundary is a recorder stand-in: this verifies parser-to-recog option routing and state behavior, not model diarization accuracy, a full CLI subprocess, or browser/video export.

No new functional blocker was found in this bounded check. Please replace the TODO PR template with the issue54 scope and these validation steps before merging. This review does not close issue54 or establish full workflow acceptance.

@Minh3132 Minh3132 changed the title Fix/cli sd switch case 54 fix(cli): honor lowercase --sd_switch values Sep 16, 2026
@Minh3132

Copy link
Copy Markdown
Author

Hi maintainers, the GitHub Actions workflow for this PR is currently awaiting maintainer approval, so no CI jobs have run yet. Could you please approve the workflow when convenient? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli模式--sd_switch参数错误

2 participants