Skip to content

fix: discover Piper beside the running Python interpreter - #894

Open
Mohammad-Maraqa wants to merge 3 commits into
OpenMontageInc:mainfrom
Mohammad-Maraqa:fix/piper-venv-discovery
Open

Mohammad-Maraqa wants to merge 3 commits into
OpenMontageInc:mainfrom
Mohammad-Maraqa:fix/piper-venv-discovery

Conversation

@Mohammad-Maraqa

Copy link
Copy Markdown

Summary

Launching a project venv interpreter directly does not add its launcher directory to PATH. Piper can therefore be installed beside that interpreter while OpenMontage reports it unavailable. Resolve the executable from PATH first, then beside the running interpreter, and use that resolver for both status and subprocess argv.

Related issue

Closes #888.

Prior work: #680 addressed the same discovery gap and was closed unmerged by its author. This is an independently implemented fix with executable-resolution regressions. Existing encoding, model-directory and explicit-model-routing PRs address separate concerns and are not included.

Changes

  • Keep PATH precedence and the venv interpreter's directory without resolving its symlink to system Python.
  • Use an explicit interpreter-adjacent candidate through shutil.which, preserving platform executable checks and Windows suffix handling.
  • Cover registry/selector availability, selected argv, missing/non-executable launchers, symlinks, timeout, exit and missing-output behavior. Explain discovery in README.

Testing

  • Windows / Python 3.14: Piper plus phase3 contracts, 96 passed, 2 POSIX-only skips; compile smoke and dependency checks passed. Independent Piper run: 12 passed, 2 skips.
  • Linux / Python 3.11: focused Piper/phase3, 97 passed, 1 Windows-only skip; make lint and make test passed: 2241 passed, 12 skipped, 3 xfailed, 1 subtest passed.
  • Public Linux validation. Production/test/docs files match this PR head; the separate validation commit adds only its workflow.
  • Windows full-suite collection was blocked by missing FFmpeg; Linux installed the CI prerequisite and ran the full suite. A separate Windows contracts run was interrupted before completion; no pass is claimed for it.
  • No Piper voice/model download or native synthesis was performed. Availability remains executable discovery, not proof that a voice model exists. The inherited generic check_dependencies remains PATH-only; inspected Piper status consumers use get_status.

Checklist

  • The change is focused on a single logical concern.
  • Relevant tests ran locally; the full suite ran on Linux CI as recorded above.
  • README updated for the changed discovery behavior.
  • No build artifacts, local config or validation workflow in the PR diff.

AI-assisted implementation and independent review using Codex. Validation results and limitations are recorded above.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Interpreter-adjacent discovery depends on version-sensitive Windows shutil.which/PATHEXT behavior that remains unverified on supported Python 3.10/3.11 configurations.

No open issues 🎉

What changed in this PR

Fixes Piper discovery when a virtual environment’s Python interpreter is launched without activating the environment, addressing #888 while preserving PATH precedence.

Changes:

  • Shares executable resolution between availability checks and subprocess execution, with an interpreter-adjacent fallback.
  • Adds offline regression coverage for discovery, launcher selection, and generation failures.
  • Documents discovery behavior and the separate voice-model requirement.
File Description
tools/​audio/​piper_tts.py Resolves Piper from PATH, then beside the interpreter, for status and execution.
tests/​tools/​test_piper_tts.py Adds discovery, platform-specific launcher, and failure-handling regressions.
tests/​contracts/​test_phase3_contracts.py Updates the unavailable-Piper mock to cover adjacent-path lookup.
README.md Explains discovery without venv activation and the voice-model requirement.

Review effort: Balanced

@calesthio calesthio 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 the focused discovery fix and the detailed cross-platform tests. I reviewed the full patch and ran the clean current-main integration: 96 passed, 2 skipped on Windows/Python 3.13.

[P2] Resolve interpreter-adjacent Windows launchers on supported Python 3.10/3.11 as well. _resolve_executable() passes an extensionless path containing a directory (.../Scripts/piper) to shutil.which. In CPython 3.11, that case returns after checking the exact path, before PATHEXT expansion, so a normal piper.exe beside the interpreter is still missed. PATHEXT expansion for directory-containing commands was added in Python 3.12; the README supports Python 3.10+.

Please explicitly resolve a valid adjacent Windows executable suffix while preserving PATH precedence and the current-directory boundary, and add a Windows 3.10/3.11 regression (or equivalent legacy-which behavior coverage). The passing 3.13 test does not cover this branch.

Primary references: https://docs.python.org/3/library/shutil.html#shutil.which and https://github.com/python/cpython/blob/3.11/Lib/shutil.py#L1364-L1370 .

@Mohammad-Maraqa

Copy link
Copy Markdown
Author

Commit 1ab414b adds an explicit interpreter-adjacent piper.exe check when the existing lookups miss on Windows. PATH keeps precedence. The legacy lookup regression failed before the change and passed afterward. The Piper suite and Piper contracts passed on Windows/Python 3.14 (17 passed, 2 POSIX-only skips). The regression emulates the 3.10/3.11 lookup behavior because Windows Application Control blocked the downloaded legacy interpreter. The broader phase 3 run hit the same policy when loading _pydantic_core (84 passed, 2 skipped, 13 failures). AI assisted with the change and validation.

@Mohammad-Maraqa

Copy link
Copy Markdown
Author

I resolved the conflict with main in c0f4ee8. The result keeps upstream’s voice routing and UTF-8 stdin changes alongside interpreter-adjacent discovery. The focused Piper tests and contracts returned 23 passed and 2 POSIX-only skips on Windows. The legacy lookup case uses a test double.

SanHsien added a commit to SanHsien/OpenMontage that referenced this pull request Nov 9, 2026
…open fixes

Apply the upstream diff (67 merged PRs plus 3 direct commits), resolving 13
conflicts in favour of the fork where it already had a fix. Port OpenMontageInc#894 (Piper
beside the interpreter), OpenMontageInc#913 (stale skill paths), OpenMontageInc#915 (explicit zero scene
threshold) and OpenMontageInc#923 (scene_plan artifact in the slideshow gate) with tests.
Replace the curl|bash hint in the MiniCut install message.

Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
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.

Piper TTS shows as unavailable unless the virtualenv is activated

3 participants