fix(memory): use slim Hindsight stack on Intel macOS instead of bare hindsight-all - #81530
fix(memory): use slim Hindsight stack on Intel macOS instead of bare hindsight-all#81530Enough1122 wants to merge 8 commits into
Conversation
68d91ef to
429932c
Compare
|
Fix follow-up: the original fix only covered This follow-up:
|
9df12e2 to
964ee66
Compare
|
Update — third commit added (964ee66): smoke-check the slim runtime, per @ijevin's superseded #81559. I've integrated the post-install smoke validation that @ijevin's #81559 (closed in favor of this PR) flagged as its unique value, so nothing needs cherry-picking from that branch:
About the failing CI check: the only red is New commits: |
|
Thanks for integrating the smoke check + Quick observation from looking at your three-commit evolution: forcing the smoke check on both the install path AND the non-force early-return refresh path is a stronger design than what I had (mine was install-only). The non-force path is exactly where stale runtimes hide — your fix catches the failure mode I'd have left behind. On the Docker arm64 timeout: that's a known GitHub Actions infrastructure flake on this repo, not your code. If it persists after a rebase-retry, it's worth pinging a maintainer — they sometimes have a workaround for the heavier Docker build path. (Not blocking the merge in any case.) Closing out from my side. 🫡 |
…1530) Slice 5/12 (one of 12 parallel test buckets containing 220 unrelated test files) failed in the previous run. The PR only touches `hermes_cli/memory_setup.py`, `hermes_cli/web_server.py`, `plugins/memory/hindsight/__init__.py`, and a new `tests/hermes_cli/test_memory_setup_intel_hindsight.py` (15/15 passing locally). Triggering fresh CI to clear the flaky bucket.
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
|
Follow-up commit pushed on What changed: the post-install smoke check now lives inside the Behavior preserved: the early-return refresh path (
|
c1b34f4 to
d554efa
Compare
|
Thanks for the detailed verification and for flagging the walkthrough — and yes, the observation about ordering is exactly right: the smoke check now runs ONLY after a confirmed successful install (commit 9dad6fce4), with failure propagation out of the install try/except (79ed32370), so a failed/blocked install can never produce a RuntimeError that claims pip succeeded. The refresh path (deps already importable) still smoke-checks the configured runtime, since that is the #81421 failure mode the check exists for. Branch rebased onto current main; tests/hermes_cli/test_memory_setup_intel_hindsight.py 21/21 pass. |
d554efa to
9327f3a
Compare
|
Rebased onto current main to pick up #85970 (cost-guard fixture fix that landed 29 minutes after the previous CI run). The slice 2/12 failures were pre-existing on main at the time (test_model_cost_guard.py, untouched by this PR), fixed upstream by #85970. Verified locally on the rebased head: 35 passed (memory setup + cost guard). |
…hindsight-all _provider_pip_dependencies appends a bare `hindsight-all` spec for every local/local_embedded install (added by NousResearch#72363 to heal Hindsight after a venv rebuild). On Intel macOS the full local-ML dependency set pulls MLX packages that ship no x86_64 wheels; the resolver backtracks instead of failing, installing ancient hindsight-all / hindsight-api releases whose overlapping hindsight_api files override the working slim API. The daemon then crashes with 'Unknown embeddings provider: onnx' and the web UI reports HTTP 500/502 (NousResearch#81421). Fix: detect Darwin + x86_64 in _provider_pip_dependencies and install the thin slim stack (hindsight-all-slim + hindsight-api-slim[local-onnx]) for local/local_embedded mode there. Apple Silicon, Linux, and all other platforms keep the full bundle — the NousResearch#70636 heal path is unchanged. Non-local modes and missing config are unaffected. Adds 6 regression tests: Intel-macOS detection, arm64/linux negatives, the issue's exact scenario (bare hindsight-all must not appear on Intel macOS), non-Intel keeps the full bundle, non-local modes unchanged, and missing-config fallback. The issue scenario is RED on pre-fix code (old code appends bare hindsight-all unconditionally).
…ost_setup (NousResearch#81421) The earlier fix only covered hermes_cli.memory_setup._provider_pip_dependencies; the Hindsight plugin's interactive post_setup wizard still hardcoded local_dep = 'hindsight-all' and bypassed the guard. The hermes memory setup flow now installs the slim stack first and then overwrites it with the bare full bundle, undoing the fix on Intel macOS. - Plugin post_setup now imports _is_intel_macos from hermes_cli.memory_setup and selects the slim specs on Intel macOS. - _IMPORT_NAMES (memory_setup.py) and _MEMORY_PROVIDER_IMPORT_NAMES (web_server.py) now cover hindsight-all-slim / hindsight-api-slim so the availability recheck short-circuits on already-installed slim packages instead of reinstalling them every run. - Adds regression tests for the plugin path's Intel-macOS dep selection.
…tall (NousResearch#81421) The NousResearch#81421 failure mode is the resolver backtracking to an ancient hindsight_api release that is still importable but no longer exposes LocalSTEmbeddings — pip reports success while the daemon crashes with "Unknown embeddings provider: onnx" and the update claims success. After a Hindsight install/refresh on Intel macOS + local mode, run a smoke probe in a clean subprocess that imports hindsight_api and hindsight_embed and confirms LocalSTEmbeddings resolves. Failures print an actionable banner and raise RuntimeError so ``hermes update`` cannot report a healed install that is still broken. The probe also runs on the non-force early-return refresh path (all slim deps already importable) to catch pre-existing stale runtimes. Gated to Intel macOS + local mode; every other path is untouched. Also corrects the slim-package import-name mapping (hindsight-all-slim / hindsight-api-slim map to the hindsight_api module they ship, not a top-level hindsight_all_slim package) in memory_setup.py and web_server.py, and adds hindsight-embed to the slim stack spec in both _provider_pip_dependencies and the plugin post_setup wizard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1530) Slice 5/12 (one of 12 parallel test buckets containing 220 unrelated test files) failed in the previous run. The PR only touches `hermes_cli/memory_setup.py`, `hermes_cli/web_server.py`, `plugins/memory/hindsight/__init__.py`, and a new `tests/hermes_cli/test_memory_setup_intel_hindsight.py` (15/15 passing locally). Triggering fresh CI to clear the flaky bucket.
…esearch#81530) @spfcraze's triage review caught the contradiction in the previous flow: on Intel macOS the smoke check sat *after* the install try/except block, so a failed or blocked install still ran it and raised a RuntimeError whose message ("pip (or an existing importable release) reports success but the configured local runtime is not usable") directly contradicted the "Run manually:" guidance the same branch just printed. Move the smoke check into the install-success branch only. The early- return refresh path (line ~210) keeps its own pre-existing call — it already covers the stale-runtime case for installs where every slim dep is already importable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ept (NousResearch#81530) The follow-up fix moved the smoke check inside `if outcome.ok:` but left it inside the install `try:` block — its RuntimeError was swallowed by the outer `except Exception`, printed as "⚠ Install failed", and the `pytest.raises(RuntimeError)` contract in test_install_failure_raises_and_prints (and the wizard's failure surface) never saw the exception. Hoist the check out of the try/except via an `install_succeeded` flag: a failed or blocked install prints "Run manually:" and returns normally (no contradictory smoke error), while a successful install that fails the smoke probe raises — the exact contract the tests assert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… mirror copy (NousResearch#81421) Extract the local-embedded spec choice into a shared helper, memory_setup._hindsight_local_embedded_deps(), used by BOTH the refresh/heal path (_provider_pip_dependencies) and the Hindsight plugin's HindsightMemoryProvider.post_setup wizard. This removes the second inline copy of the Intel-macOS slim-stack list that the PR's earlier commits had to keep in sync. The TestHindsightPluginPostSetupGuard tests now drive the real shared helper (which is exactly what the plugin calls) instead of re-implementing the Intel/non-Intel branch and asserting against their own mirror, and add a wiring assertion that the REAL HindsightMemoryProvider.post_setup source actually invokes the helper. The guard can now only break when real plugin behavior changes, not when the test's clone of it drifts out of date. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rch#81421) The interactive HindsightMemoryProvider.post_setup wizard installed the Intel-macOS slim stack but never ran the post-install smoke check that the update/heal path runs. On Intel macOS + local_embedded, pip can report ok while the resolver backtracks the slim stack to an ancient hindsight_api that no longer exposes LocalSTEmbeddings, and the daemon then crashes with "Unknown embeddings provider: onnx" — a fresh interactive `hermes memory setup` could slip through undetected. Call the shared `_maybe_run_intel_macos_local_embedded_smoke_check` after the selected mode is persisted to config.json (Step 4), gated on install success only, mirroring the update path's outcome.ok-only rule. Placing it after persistence matters: the helper gates on the on-disk config.json mode, and on a fresh setup there is no config at install time, so calling it right after install would silently no-op. Its RuntimeError propagates so the wizard cannot claim a configured-but- broken runtime; failed/blocked installs never reach it. Adds tests driving the real wizard end-to-end (pickers/stdin/install mocked) asserting the smoke probe fires on Intel macOS after a successful local_embedded install and does not fire on non-Intel or on failed/blocked installs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9327f3a to
dc3ec37
Compare
|
Rebased onto The previous rebase onto latest main picked up This PR's own changes are unaffected: memory-setup + cost-guard + profile-home tests all pass (36/36) on the rebased head. One commit behind latest main (56a4171 only) — happy to rebase again once the main-side test is fixed upstream. |
Fixes #81421
Root cause
hermes_cli/memory_setup.py::_provider_pip_dependenciesappends a barehindsight-allspec for every Hindsightlocal/local_embeddedinstall. This was added by #72363 (commit5645169c8f1) to restore local Hindsight after a venv rebuild strippedhindsight-embed(#70636).On Intel macOS, the current full local-ML dependency set includes MLX packages that have no x86_64 wheels. The resolver backtracks instead of failing: it installs ancient
hindsight-all/hindsight-apireleases whose overlappinghindsight_apifiles override the working slim API. The daemon then crashes with:while the Hindsight web UI stays up and reports HTTP 500/502 — exactly the mixed-stack evidence in the issue (
hindsight-all 0.4.17+hindsight-api 0.3.0installed over slim0.8.4).Fix
_provider_pip_dependenciesnow detectsDarwin + x86_64(_is_intel_macos()) and installs the thin slim stack for local/local_embedded mode on that platform:hindsight-all-slimhindsight-api-slim[local-onnx]This matches the known-working Intel stack from the issue (slim 0.8.4 +
hindsight-allinstalled--no-depsas a thin wrapper; the explicit slim specs are the portable declaration). Apple Silicon, Linux, and every other OS keep the fullhindsight-allbundle — the #70636 heal path is unchanged. Non-local modes and missing config fall back to the declared bridge deps exactly as before.Regression tests
6 tests in
tests/hermes_cli/test_memory_setup_intel_hindsight.py:test_detects_intel_macos— Darwin + x86_64 detection.test_arm64_macos_is_not_intel— Apple Silicon keeps the full bundle.test_linux_is_not_intel_macos— Linux x86_64 keeps the full bundle.test_intel_macos_uses_slim_stack_not_bare_bundle— the issue's exact scenario: barehindsight-allmust NOT be in the dependency plan on Intel macOS.test_non_local_modes_unaffected— remote-mode config never gets local deps.test_missing_config_falls_back_to_declared— no config.json → declared deps only.The issue scenario is RED on pre-fix code (verified: old code appends bare
hindsight-allunconditionally for local_embedded) and GREEN after. Existingtest_memory_setup.py(4 tests) continues to pass.Scope
Two small additions to
memory_setup.py(_is_intel_macoshelper + one branch in_provider_pip_dependencies) plus tests. No change to the installer, the resolver, or non-Hindsight providers.