Skip to content

fix(python-tests): UnboundLocalError instead of the intended ValueError in test_fast_clustering - #3944

Merged
csukuangfj merged 1 commit into
k2-fsa:masterfrom
Anai-Guo:fix-test-fast-clustering-unbound-config
Sep 10, 2026
Merged

csukuangfj merged 1 commit into
k2-fsa:masterfrom
Anai-Guo:fix-test-fast-clustering-unbound-config

Conversation

@Anai-Guo

@Anai-Guo Anai-Guo commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

The bug

TestFastClustering.test_cluster_speaker_embeddings names its extractor config extractor_config, because plain config is reused ~20 lines further down for the FastClusteringConfig. The validation error, however, still interpolates {config}:

extractor_config = sherpa_onnx.SpeakerEmbeddingExtractorConfig(
    model=str(model_file), num_threads=1, debug=0,
)
if not extractor_config.validate():
    raise ValueError(f"Invalid extractor config. {config}")   # line 131
...
config = sherpa_onnx.FastClusteringConfig(num_clusters=3)     # line 150 -- first binding

config is a local of this method (bound at line 150), so at line 131 it is unbound. When the extractor config fails to validate — a corrupt or incompatible .onnx, which is exactly what this branch exists to report — the test dies with

UnboundLocalError: cannot access local variable 'config' where it is not associated with a value

instead of the intended message, and the actual config never gets printed. python -m pyflakes on master flags it:

sherpa-onnx/python/tests/test_fast_clustering.py:131:59: undefined name 'config'

The fix

Interpolate extractor_config. This is the only place in the Python tree where the validated local and the interpolated name disagree — the six sibling call sites all name the local config and print that same local:

file line
sherpa-onnx/python/tests/test_speaker_recognition.py 54
python-api-examples/speaker-identification.py 122
python-api-examples/speaker-identification-with-vad.py 137
python-api-examples/speaker-identification-with-vad-dynamic.py 106
python-api-examples/speaker-identification-with-vad-non-streaming-asr.py 350
python-api-examples/speaker-identification-with-vad-non-streaming-asr-alsa.py 350

So this looks like the copy that renamed the local but not the message.

Verification

The test's real method was extracted with ast and run against a stub whose SpeakerEmbeddingExtractorConfig.validate() returns False (no sherpa_onnx build needed), with the wave/model fixture files created so the early-return guards don't fire:

condition: extractor_config.validate() returns False

upstream  -> UnboundLocalError: cannot access local variable 'config' where it is not associated with a value
patched   -> ValueError: Invalid extractor config. SpeakerEmbeddingExtractorConfig({'model': '...3dspeaker_speech_eres2net_base_sv_zh-cn_3dspeaker_16k.onnx', 'num_threads': 1, 'debug': 0})

The happy path is untouched — this only changes which name the error message reads.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the invalid extractor configuration error message to reference the appropriate configuration setting.

test_cluster_speaker_embeddings names its extractor config
`extractor_config`, because plain `config` is reused further down for the
FastClusteringConfig. The validation error still interpolates `{config}`,
which at that point is a local that has not been assigned yet, so a model
file that fails to validate raises

    UnboundLocalError: cannot access local variable 'config' where it is
    not associated with a value

instead of the intended message. Interpolate `extractor_config`, matching
the six sibling call sites (test_speaker_recognition.py and the
speaker-identification examples), which all name the local `config`.

Signed-off-by: Tai An <antai12232931@outlook.com>
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 89276ecc-99a4-42d3-8236-42d4ae25ec11

📥 Commits

Reviewing files that changed from the base of the PR and between b0899d9 and 437625c.

📒 Files selected for processing (1)
  • sherpa-onnx/python/tests/test_fast_clustering.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change corrects the invalid extractor-config error message in the fast clustering test. The message now references extractor_config instead of config.

Changes

Extractor validation

Layer / File(s) Summary
Correct extractor configuration reference
sherpa-onnx/python/tests/test_fast_clustering.py
The validation error message now reports the extractor_config object.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 43762

This test-only fix reports the invalid extractor configuration correctly and restores the intended ValueError behavior. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the test fix and the incorrect UnboundLocalError behavior. It accurately reflects the main change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@csukuangfj csukuangfj 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.

Thank you for your contribution!

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.

2 participants