Skip to content

fix(wake): declare pypinyin for the sherpa wake-word engine (#74719) - #74733

Open
chelsealong wants to merge 2 commits into
NousResearch:mainfrom
chelsealong:fix/wake-sherpa-pypinyin-dep
Open

chelsealong wants to merge 2 commits into
NousResearch:mainfrom
chelsealong:fix/wake-sherpa-pypinyin-dep

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

What does this PR do?

The sherpa wake-word engine never arms when a custom phrase is set — the desktop app shows Wake word: "…" — off — No module named 'pypinyin'.

sherpa_onnx.text2token() imports both sentencepiece and pypinyin unconditionally at call time (before any tokens_type branching), even for the English BPE phrases the wake word tokenizes — yet sherpa-onnx declares neither as a dependency. sentencepiece was already carried by the [wake] extra and LAZY_DEPS["wake.sherpa"], but pypinyin was missing, so tools/wake_word.py's first text2token() call raised ModuleNotFoundError: No module named 'pypinyin' and the listener silently failed to arm. On CLI-only installs the miss is worse: lazy_deps.ensure("wake.sherpa") reported the feature satisfied (sherpa-onnx present) and never installed pypinyin.

Fix: add pypinyin alongside sentencepiece in the two places that declare the sherpa engine's runtime deps, so it ships on eager desktop installs and is lazy-installed on CLI installs — exactly the same treatment sentencepiece already gets.

Related Issue

Fixes #74719

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • pyproject.toml — add pypinyin==0.55.0 to the [wake] extra (eager desktop install path).
  • tools/lazy_deps.py — add pypinyin==0.55.0 to LAZY_DEPS["wake.sherpa"] (CLI lazy-install path).
  • uv.lock — regenerated with uv lock (adds pypinyin only; no other resolution churn).
  • tests/test_project_metadata.py — new test_wake_sherpa_declares_text2token_runtime_deps asserting both surfaces carry text2token's undeclared runtime deps (sentencepiece, pypinyin).

How to Test

  1. git stash the pyproject.toml + tools/lazy_deps.py changes and run scripts/run_tests.sh tests/test_project_metadata.py -k test_wake_sherpa_declares_text2token_runtime_deps → fails with missing ['pypinyin'].
  2. Restore the changes and re-run → passes (all 8 tests in the file green).
  3. scripts/run_tests.sh tests/tools/test_wake_word.py → 22 passed.
  4. End-to-end: with a sherpa wake phrase configured (wake_word: {enabled: true, provider: sherpa, phrase: "yo iris"}), the engine now tokenizes the phrase instead of raising No module named 'pypinyin'.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(wake):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected test files via scripts/run_tests.sh and they pass (see How to Test); I did not run the full suite
  • I've added a regression test (metadata test that fails without the fix)
  • I've tested on my platform: macOS arm64

Documentation & Housekeeping

  • Docs — N/A (dependency-only fix; rationale captured in code comments)
  • cli-config.yaml.example — N/A (no config keys changed)
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform impact — pypinyin is a pure-Python py2.py3 wheel, no platform constraints
  • Tool descriptions/schemas — N/A

AI assistance disclosure

This change was prepared with AI assistance (Claude). The root cause (text2token's unconditional pypinyin import in sherpa-onnx 1.13.4), the fix, the regression test, and the lockfile regeneration were all verified locally as described above.

@chelsealong
chelsealong requested a review from a team July 30, 2026 09:22
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets tool/tts Text-to-speech and transcription labels Jul 30, 2026

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

Thanks for tracing this to sherpa-onnx's runtime imports. The core fix is valid: current main calls text2token() after ensure("wake.sherpa") (tools/wake_word.py:571-600), while current dependency declarations omit pypinyin (pyproject.toml:190-203, tools/lazy_deps.py:171-179). Upstream sherpa-onnx v1.13.4 imports pypinyin before its BPE branch (sherpa-onnx/python/sherpa_onnx/utils.py:40-55).

Problems

  • uv.lock:3995 also removes an existing scipy NumPy marker. The diff similarly changes scipy 1.18.0 and vercel-workers markers, none of which are related to pypinyin.

Suggested changes

  • Regenerate the lock with CI's pinned uv 0.9.28 (.github/workflows/uv-lockfile-check.yml:72-77) or restore the unrelated marker edits, retaining only the pypinyin lock additions.

Automated hermes-sweeper review.

Comment thread uv.lock Outdated
@@ -3981,7 +3992,7 @@ resolution-markers = [
"python_full_version < '3.12'",
]
dependencies = [
{ name = "numpy", marker = "python_full_version < '3.12'" },
{ name = "numpy" },

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.

This removes the existing Python-version marker from scipy's NumPy dependency, unrelated to adding pypinyin. Please regenerate with the repository's CI-pinned uv version or restore this and the other unrelated marker changes so the lockfile diff stays scoped to the new dependency.

@chelsealong

Copy link
Copy Markdown
Contributor Author

Restored the scipy numpy marker and vercel-workers python-version markers you flagged on uv.lock:3995 — confirmed via a clean uv lock (CI-pinned 0.9.28) starting from a pypinyin-only pyproject.toml diff that they weren't caused by this change, then manually restored them so the lockfile diff is scoped to the pypinyin addition only. uv lock --check and all 8 tests in tests/test_project_metadata.py pass.

…arch#74719)

sherpa_onnx.text2token imports both sentencepiece and pypinyin
unconditionally at call time — before any tokens_type branching — even
for the English BPE phrases the wake word tokenizes, yet sherpa-onnx
declares neither. sentencepiece was already carried by the [wake] extra
and LAZY_DEPS["wake.sherpa"]; pypinyin was missing, so the sherpa engine
died on its first text2token() call with `No module named 'pypinyin'`
and the listener never armed (the desktop app showed "off — No module
named 'pypinyin'").

Add pypinyin==0.55.0 to the [wake] extra (eager desktop install) and to
LAZY_DEPS["wake.sherpa"] (CLI lazy install), and regenerate uv.lock. Add
a packaging-metadata regression test asserting both surfaces carry
text2token's undeclared runtime deps.
Restore the scipy numpy marker and vercel-workers python-version
markers that a prior lock regeneration incidentally stripped —
unrelated to the pypinyin dependency this PR adds. Verified a clean
`uv lock` run (CI's pinned 0.9.28) starting from a pypinyin-only
pyproject.toml change reproduces no such marker drift, confirming it
wasn't caused by this change.
@chelsealong
chelsealong force-pushed the fix/wake-sherpa-pypinyin-dep branch from 0d14f24 to a3be51d Compare August 1, 2026 01:20
@chelsealong

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to clear the conflict, and regenerated uv.lock from upstream's version so the lockfile diff is now only the pypinyin addition (+11 lines, nothing removed) — which is what you flagged earlier. uv lock --check is clean and tests/test_project_metadata.py passes (8 passed).

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Three PRs address this issue complex. #74733 and #74768 both add the missing pypinyin dependency to the eager and lazy Sherpa paths, while #75321 also corrects the wrong-interpreter installation guidance and adds installation-order coverage for the Hermes environment.

Related pull requests

Duplicates

#74768 is a substantive duplicate of #74733 for #74719. #75321 overlaps both on the pypinyin dependency declarations but is not a full duplicate because it additionally addresses #75241's wrong-environment guidance and tests the actual lazy-install path.

Suggested consolidation

Keep #74733 open with a salvage path as the scoped dependency fix, retaining its eager/lazy declarations, metadata regression, and clean pypinyin-only lockfile diff; close #74768 as duplicate of #74733. For #75321, author action: rebase onto main with a clean lockfile or split out the distinct Hermes-interpreter remediation and ensure-before-text2token tests, removing the duplicated dependency work and unrelated lockfile changes.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I74719(["issue #74719 (open)"])
    I75241(["issue #75241 (open)"])
    subgraph Dup74733 ["PRs duplicating each other"]
        P74733["PR #74733 (open)"]
        P74768["PR #74768 (open)"]
    end
    P74733 -->|best fix| I74719
    P74733 -->|fixes| I75241
    class I74719 open
    class I75241 open
    class P74733 open
    class P74768 open
    class P74733 best
    class P74733 target
    click I74719 "https://github.com/NousResearch/hermes-agent/issues/74719"
    click I75241 "https://github.com/NousResearch/hermes-agent/issues/75241"
    click P74733 "https://github.com/NousResearch/hermes-agent/pull/74733"
    click P74768 "https://github.com/NousResearch/hermes-agent/pull/74768"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 7 kB of issue/PR text, 5 kB of discussion (11 comments), 8 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

AnalogHubris pushed a commit to AnalogHubris/hermes-agent that referenced this pull request Aug 13, 2026
…rm64

sherpa-onnx 1.13.x is a broken wheel on macOS arm64: KeywordSpotter builds,
streams, and reports no error, but produces ZERO detections — even against
sherpa's own bundled model and test wavs. The wake listener "arms" (log shows
listening for the phrase) but the phrase never triggers. Verified locally on
2026-07-31 with a synthetic "hey robot" clip through the engine: 1.13.4 = no
detections, 1.12.40 = clean detection.

The pypinyin half of the sherpa runtime dep is already covered by NousResearch#74733 /
NousResearch#74768 / NousResearch#75321; this PR only pins the broken wheel back to the last version
that actually fires, and adds the matching sherpa-onnx-core pin so the native
closure resolves (adjacent to NousResearch#77947, which locks 1.13.4-core for a different
dlopen issue but does not fix detection).

- tools/lazy_deps.py: wake.sherpa 1.13.4 → 1.12.40 (+ core pin)
- pyproject.toml: wake extra same pins

Fixes the silent never-fires wake word on Apple Silicon.

This branch has not been deployed

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

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/tts Text-to-speech and transcription type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sherpa wake word fails to arm: No module named 'pypinyin'

4 participants