Skip to content

chore: Perf/wheel pipeline and runtime defaults#198

Merged
goodroot merged 3 commits into
mainfrom
perf/wheel-pipeline-and-runtime-defaults
Jun 24, 2026
Merged

chore: Perf/wheel pipeline and runtime defaults#198
goodroot merged 3 commits into
mainfrom
perf/wheel-pipeline-and-runtime-defaults

Conversation

@goodroot

Copy link
Copy Markdown
Owner

Some tidy around setup and process, as well as some performance improvements and defaults.

goodroot added 2 commits June 24, 2026 09:56
…ift guard

The installer requested pywhispercpp 1.4.1 wheels but the workflow built 1.4.0, so every download 404'd and silently fell back to a slow source build (NVIDIA) or PyPI (CPU). Make backend_installer.py the single source of truth: the workflow derives version, CUDA variant, and expected filename by importing it. Build one cuda12 wheel per CUDA major, drop self-hosted CPU wheels (PyPI already ships them), and gate publish on a verify-wheels job that fails on any name/version drift. Point WHEEL_BASE_URL at wheels-v2 and remove dead _parallel_deps_and_wheel.
…rride

threads now defaults to min(8, os.cpu_count() or 4) instead of a hardcoded 4. Seed a default word override 'hyper whisper' -> 'hyprwhspr' for the product name; users with their own overrides are never clobbered (config load is a top-level dict replace, so a user's word_overrides fully wins). Update schema (drop pinned numeric threads default) and docs (omit-for-auto note, language auto-detect latency tip) to match.
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Continue PR Review

Automated review via Continue CLI run through opub.

Summary

The PR is generally sound, but it introduces one likely packaging/install regression for CPU installs and one workflow gap that can let broken release wheels ship despite the new verification job.

Findings

  • install_pywhispercpp_cpu() now verifies installation with import pywhispercpp immediately after pip install (lib/src/backend_installer.py). That import can fail even when the package was installed correctly if pywhispercpp imports optional/native runtime dependencies at import time. This changes success criteria from “package installed” to “package fully importable in this CI/runtime environment,” which is stricter than before and can cause false-negative setup failures for CPU users. Consider verifying with pip show pywhispercpp / metadata inspection, or importing a lighter module if available.
  • The release workflow no longer validates the exact wheel artifact that users download from GitHub. In verify-wheels, the job copies dist/${EXPECTED_WHEEL} to a pip-compatible filename and installs the renamed file, but it never checks that the original +cuda12 wheel itself is accepted by pip or that the release asset name is usable as published (.github/workflows/build-wheels.yml). Since download_pywhispercpp_wheel() depends on downloading the suffixed asset and renaming it locally, a filename/packaging mismatch could still break end users while CI stays green. Add a test that downloads/installs the artifact exactly as published, then performs the local rename path used by backend_installer.py.

Suggested follow-ups

  • Add a unit test for install_pywhispercpp_cpu() covering the “pip succeeds but import fails” path, to confirm the intended behavior and avoid accidental regressions.
  • Add a test for _detect_venv_python_version() where python --version writes to stderr, since that was one of the behavior changes in this PR.

Summary

The diff is generally solid, but there are a couple of merge-blocking regressions around configuration defaults and wheel-install coverage that should be addressed before release.

Findings

  • threads is now implicitly enabled for all users instead of being optional/auto-only. In lib/src/config_manager.py, the default config now sets 'threads': min(8, os.cpu_count() or 4), while the docs/schema say users should omit threads for auto behavior. This changes persisted config semantics: any code that distinguishes “unset” from “user explicitly set a value” will now always see a concrete integer. That can break existing auto-tuning/reload behavior and makes the documentation inaccurate. Either keep threads absent/None by default and resolve auto at use time, or update all consumers/schema/docs consistently.
  • CPU installation path no longer has test coverage for the new --only-binary=pywhispercpp behavior and import verification. install_pywhispercpp_cpu() was materially changed in lib/src/backend_installer.py to stop using self-hosted wheels, require binary wheels from PyPI, and fail if import pywhispercpp does not work after pip succeeds. Those are good changes, but the added tests only cover helper functions and config defaults. Missing tests here create release risk for a user-facing install path, especially across Python 3.13/3.14 and filtered requirements scenarios.

Suggested follow-ups

  • Add/adjust tests for install_pywhispercpp_cpu() covering:
    • successful install with --only-binary=pywhispercpp
    • pip success but import pywhispercpp failure
    • requirements filtering when PyGObject is skipped
  • Reconcile threads behavior across config_manager.py, share/config.schema.json, and docs:
    • either restore “unset means auto” in code
    • or document/schema it as an always-materialized default and verify consumers handle that correctly

@goodroot goodroot force-pushed the perf/wheel-pipeline-and-runtime-defaults branch from f44d4c7 to fe09812 Compare June 24, 2026 17:10
- build-cuda: apt-get update before installing libcublas (fresh-runner flakiness)
- CPU install: --only-binary=pywhispercpp so a missing PyPI wheel fails clearly instead of silently attempting a source build
- CPU install: post-install 'import pywhispercpp' check (using the pip_bin-paired interpreter) so a pip success that didn't install the backend fails cleanly
- _detect_venv_python_version: delegate to _get_python_version so a stderr-printing interpreter no longer falls back to the wrong (system) Python and picks a wrong wheel filename
- verify-wheels: ldd smoke check fails on any non-CUDA unresolved lib (a real import can't run on GPU-less CI)
- tests: _get_wheel_variant / _get_wheel_filename (CUDA 11/12/13/none, py3.10-3.14) and config defaults (threads, word_overrides incl. existing-user preservation)
- docs: note the product word override ships by default
@goodroot goodroot force-pushed the perf/wheel-pipeline-and-runtime-defaults branch from fe09812 to 875e087 Compare June 24, 2026 17:18
@goodroot

Copy link
Copy Markdown
Owner Author

Cool, yep, the rest is noise.

@goodroot goodroot merged commit eebdccd into main Jun 24, 2026
2 checks passed
@goodroot goodroot deleted the perf/wheel-pipeline-and-runtime-defaults branch June 24, 2026 17:21
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.

1 participant