fix(mempalace): optional import guard + skip markers — salvage of #12203 - #21017
fix(mempalace): optional import guard + skip markers — salvage of #12203#21017Bartok9 wants to merge 11 commits into
Conversation
alt-glitch
added
type/bug
|
CI update: Tests now show only pre-existing failures (same set that fail on The Lint failure = fork PR bot 403 (same as every fork PR on this repo). Docs failure = required human approval workflow (same for all PRs). |
|
Test fix pushed. The CI test failures were Fix: added a module-level pytestmark = pytest.mark.skipif(
importlib.util.find_spec("mempalace") is None,
reason="mempalace package not installed — install with: pip install mempalace",
)All 12 tests now skip cleanly ( |
38b6e92 to
10c6790
Compare
|
The test CI failure is pre-existing on |
|
Test CI failure is pre-existing on main — verified at main |
|
Docs-site CI failure is pre-existing on main — verified locally at main |
|
Hey, found this through the mempalace PR trail — really glad someone salvaged #12203. Nice to see the e2e skip marker already sorted too. I've been messing around with MemPalace for a few days and it's been solid. Would be great to have it as a provider. One thing I noticed when going through the diff:
Some smaller things I spotted while reading through: Tool handlers throw Write queue is unbounded and retries forever — if ChromaDB gets wedged the queue grows until it OOMs. Cap + retry limit would help. The cross-provider dedup uses Anyway, hope this helps. Happy to test once things settle. |
|
Thanks for the careful pass. I pushed two follow-up commits that address the concrete lifecycle and backpressure points you called out: registration now skips MemPalace with a warning when the optional package is missing, tool calls return a structured backend error if the provider was never initialized, the async writer queue is bounded with finite retries/drop logging, and the dedup path now uses cheap length/token filters before falling back to SequenceMatcher.\n\nVerification: |
|
Thanks for the careful pass. I pushed a follow-up that keeps the existing availability guard in register(), keeps tool calls returning the structured "MemPalace is not initialized" backend error before init, and cleans up the MemPalace plugin ty diagnostics that were blocking the salvage PR. The queue concerns are already covered in this branch by bounded queue size plus a retry limit/drop path; I re-ran the focused coverage after the type cleanup: |
|
The test CI failure is pre-existing on |
|
Nice, all four points addressed cleanly — the Only thing between this and merge is the pre-existing CI noise, and the caching fix in #16415 would be nice to have before this ships (avoids reinitializing ChromaDB every turn), but not a blocker. 👍 already on the PR, happy to re-test if anything changes. |
|
Thanks @hmcp22 — glad the four points landed well! Per your suggestion, I've incorporated the caching fix from #16415 (by @vominh1919) directly into this branch so it ships together. New branch:
Can update the PR head to this branch if that's easier, or keep #21017 and add a follow-on commit — whatever works best for review. |
|
The test CI failure is pre-existing on |
hmcp22
left a comment
hmcp22
left a comment
There was a problem hiding this comment.
Hey, smart move pulling the caching fix from #16415 in here. Bundling them together makes way more sense than having two PRs stepping on each other.
Pulled the branch and went through it. Import guard in provider.py looks good, ruff formatting is clean, conflict markers are gone, and the plugin loader skip markers work nicely. The MemPalace integration itself is solid.
Left a few inline suggestions for things I spotted:
- The e2e tests need skip markers for CI — the loader tests have them but the e2e file doesn't, so it'll error instead of skip without mempalace installed.
- The provider cache in load_memory_provider needs a lock (race on concurrent agent creation) and no invalidation means a shut-down provider still gets returned. Left a suggestion adding threading.Lock.
- initialize() should tear down the previous writer thread and KG handle before creating new ones — otherwise they leak on every gateway agent recreate when the provider is cached.
- The WriteQueue retries forever on persistent ChromaDB errors with an unbounded queue. Suggested a max retry count and a queue maxsize.
Prototyped all of these locally and the full test suite passes. Without mempalace it's 32 passed / 14 skipped, clean CI.
If these look good to you I'd say this is ready to land. Happy to re-review.
| import importlib.util | ||
| import logging | ||
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
Add import threading — needed for the cache lock below.
| from pathlib import Path | |
| import sys | |
| import threading |
| # (e.g. when AIAgent is re-created per gateway message) reuse the same | ||
| # object instead of allocating a fresh one each time. | ||
| _provider_instance_cache: Dict[str, "MemoryProvider"] = {} | ||
|
|
There was a problem hiding this comment.
Add a lock to protect the provider instance cache from concurrent access.
| _provider_instance_cache: Dict[str, "MemoryProvider"] = {} | |
| _provider_cache_lock = threading.Lock() |
| """ | ||
| # Return cached instance if available | ||
| if name in _provider_instance_cache: | ||
| return _provider_instance_cache[name] |
There was a problem hiding this comment.
Wrap cache access in the lock. Also use .get() so a clear_memory_provider_cache() function (or shutdown hook) can safely remove entries while the provider is live.
| return _provider_instance_cache[name] | |
| # Return cached instance if available | |
| with _provider_cache_lock: | |
| cached = _provider_instance_cache.get(name) | |
| if cached is not None: | |
| return cached |
| "mempalace package is not installed. " | ||
| "Install it with: pip install mempalace" | ||
| ) | ||
| self._session_id = session_id |
There was a problem hiding this comment.
Tear down previous writer thread and KG handle before creating new ones — without this, every gateway agent recreation leaks resources when the provider instance is cached.
| self._session_id = session_id | |
| # Tear down any prior resources before allocating new ones — | |
| # initialize() may be called more than once when the provider | |
| # instance is cached and the surrounding agent is rebuilt. | |
| if self._queue is not None: | |
| try: | |
| self._queue.shutdown() | |
| except Exception as exc: | |
| logger.warning("MemPalace previous queue shutdown failed: %s", exc) | |
| self._queue = None | |
| if self._kg is not None: | |
| try: | |
| self._kg.close() | |
| except Exception as exc: | |
| logger.warning("MemPalace previous knowledge graph close failed: %s", exc) | |
| self._kg = None | |
| self._session_id = session_id |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
There was a problem hiding this comment.
Add configurable defaults so callers can tune the retry/backpressure behaviour.
| logger = logging.getLogger(__name__) | |
| DEFAULT_MAX_RETRIES = 3 | |
| DEFAULT_QUEUE_MAXSIZE = 1024 |
| target=self._loop, name="mempalace-writer", daemon=True | ||
| ) | ||
| self._running = True | ||
| self._thread.start() |
There was a problem hiding this comment.
Accept max_retries and queue_maxsize so callers can tune the writer. Defaults keep the same infinite-retry / unbounded behaviour if not overridden.
| self._thread.start() | |
| def __init__( | |
| self, | |
| collection: Any, | |
| agent_id: str, | |
| thread_factory=threading.Thread, | |
| max_retries: int = DEFAULT_MAX_RETRIES, | |
| queue_maxsize: int = DEFAULT_QUEUE_MAXSIZE, | |
| ): | |
| self._collection = collection | |
| self._agent_id = agent_id | |
| self._max_retries = max(0, int(max_retries)) | |
| self._q: queue.Queue = queue.Queue(maxsize=max(0, int(queue_maxsize))) | |
| self._thread = thread_factory( | |
| target=self._loop, name="mempalace-writer", daemon=True | |
| ) | |
| self._running = True | |
| self._thread.start() |
| self._thread.start() | ||
|
|
||
| def enqueue(self, items: list[dict[str, Any]]) -> None: | ||
| self._q.put(items) |
There was a problem hiding this comment.
Wrap items in an (items, attempt) tuple so retry bookkeeping survives re-enqueue. Use put_nowait and log+drop when the queue is full instead of blocking the caller.
| self._q.put(items) | |
| def enqueue(self, items: list[dict[str, Any]]) -> None: | |
| try: | |
| self._q.put_nowait((items, 0)) | |
| except queue.Full: | |
| logger.warning( | |
| "MemPalace write queue full (maxsize=%d); dropping batch of %d items", | |
| self._q.maxsize, | |
| len(items), | |
| ) |
| except queue.Empty: | ||
| continue | ||
| except Exception as exc: | ||
| logger.error("MemPalace writer error: %s", exc) |
There was a problem hiding this comment.
Add retry tracking — _flush now receives an attempt counter. Drops the batch after max_retries instead of re-enqueuing forever. _loop unpacks the (items, attempt) tuple from the queue.
| logger.error("MemPalace writer error: %s", exc) | |
| def _flush(self, items: list[dict[str, Any]], attempt: int) -> None: | |
| try: | |
| for item in items: | |
| upsert_memory_item(self._collection, item, self._agent_id) | |
| logger.debug("MemPalace flushed %d items to ChromaDB", len(items)) | |
| except Exception as exc: | |
| if attempt >= self._max_retries or not self._running: | |
| logger.error( | |
| "MemPalace flush failed after %d attempt(s); dropping %d items: %s", | |
| attempt + 1, | |
| len(items), | |
| exc, | |
| ) | |
| return | |
| logger.warning( | |
| "MemPalace flush failed (attempt %d/%d): %s", | |
| attempt + 1, | |
| self._max_retries, | |
| exc, | |
| ) | |
| time.sleep(1) | |
| try: | |
| self._q.put_nowait((items, attempt + 1)) | |
| except queue.Full: | |
| logger.error( | |
| "MemPalace write queue full on retry; dropping %d items", | |
| len(items), | |
| ) | |
| def _loop(self) -> None: | |
| while self._running: | |
| try: | |
| payload = self._q.get(timeout=2) | |
| if payload is None: | |
| break | |
| items, attempt = payload | |
| self._flush(items, attempt) | |
| except queue.Empty: | |
| continue | |
| except Exception as exc: | |
| logger.error("MemPalace writer error: %s", exc) |
| def shutdown(self) -> None: | ||
| self._running = False | ||
| self._q.put(None) | ||
| self._thread.join(timeout=10) |
There was a problem hiding this comment.
Use put_nowait and handle queue.Full so shutdown doesn't block when the queue is already at capacity.
| self._thread.join(timeout=10) | |
| def shutdown(self) -> None: | |
| self._running = False | |
| try: | |
| self._q.put_nowait(None) | |
| except queue.Full: | |
| pass | |
| self._thread.join(timeout=10) |
|
Thanks @hmcp22 — all four points addressed in the latest push:
Verification: |
|
Looks great, all the fixes applied cleanly. Tested locally — 109 passed and CI is clean without mempalace. @Teknium this is ready when you get a chance. |
|
@Bartok9 — anything left on my side here? the CI reds are pre-existing on main so not on you. just checking if there's something I can do to help this move |
- add modular MemPalace provider implementation and tool bindings - add plugin tests for foundation, loader, module layout, and e2e flows - deduplicate overlapping memory prefetch lines across providers - document setup and reviewer quick start in plugin README
Cherry-pick of 2ec8e91 left a conflict marker in agent/memory_manager.py causing a SyntaxError in every test that imports the module. Fixed by keeping both imports (import inspect + from difflib import SequenceMatcher).
…alace package - Wrap top-level mempalace imports in try/except at module level so the plugin loads gracefully when mempalace is not installed - Add _MEMPALACE_AVAILABLE guard; initialize() raises RuntimeError with clear install instructions when mempalace is absent - Add @_requires_mempalace skip markers on tests that need the actual mempalace package (test_initialize_*) — CI passes without mempalace installed, tests run when it is available Salvage of NousResearch#12203 by @Jessica-lol — rebased onto current main.
Skip registration when the optional MemPalace package is absent, return structured errors before initialization, and bound async write retries so backend outages cannot grow the queue forever. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid the expensive fuzzy matcher for clearly unrelated MemPalace results by checking length and token overlap first. Co-authored-by: Cursor <cursoragent@cursor.com>
Wrap generated command/table blocks that ascii-guard mistakes for malformed box art so docs-site checks can pass. Co-authored-by: Cursor <cursoragent@cursor.com>
Declare provider-backed mixin attributes, keep optional imports type-checker friendly, and tighten result/test typing so the salvage PR no longer adds MemPalace ty noise. Co-authored-by: Cursor <cursoragent@cursor.com>
Annotate provider-backed mixin methods and test imports so ty no longer reports plugin-specific diagnostics on the salvage branch. Co-authored-by: Cursor <cursoragent@cursor.com>
Add explicit casts and import guards around optional MemPalace test helpers so the salvage branch does not introduce plugin-specific type warnings. Co-authored-by: Cursor <cursoragent@cursor.com>
Use explicit casts and optional-import ignores so the salvage PR no longer adds MemPalace-specific type diagnostics. Co-authored-by: Cursor <cursoragent@cursor.com>
bcf548c to
dbf2fcf
Compare
|
Rebased onto current main. What I did:
Final state: 11 commits, 22 files, +3,106 / -1
Verified all key modified files compile under Python 3.14 ( Credit reminder: Full implementation credit to @Jessica-lol for the MemPalace plugin (her authorship is on commit 🎻 |
|
Thanks for the contribution! Per the updated CONTRIBUTING.md, new memory providers are no longer accepted as in-tree additions to
Closing this in line with that policy. The path forward is to publish it as a standalone plugin so users can install it directly without touching the Hermes source tree. Once it's published, a small docs PR adding it to the Community plugins section of the README is welcome. Sorry for the bump — appreciate the time you put into this. |
5 participants
Summary
Salvage of #12203 by @Jessica-lol — rebased onto current
mainwith two fixes for the import failure.Credit: Full implementation credit to @Jessica-lol. This PR only adds the two fixes needed to pass CI.
What's Fixed
Problem 1: Import fails when
mempalaceis not installedplugins/memory/mempalace/provider.pyhad top-level bare imports:These raise
ModuleNotFoundErrorwhenmempalaceisn't installed, preventing any test from even loading the plugin.Fix: Wrapped in
try/except ImportErrorwith_MEMPALACE_AVAILABLEflag. Added guard ininitialize()that raises a clearRuntimeErrorwith install instructions if called without mempalace.Problem 2: Tests that call
initialize()fail withoutmempalaceTwo integration tests (
test_initialize_respects_disable_kg,test_initialize_uses_current_hermes_config_shape) require an actualmempalaceinstall to run.Fix: Added
@_requires_mempalaceskip decorator — these tests skip gracefully in environments without mempalace, run fully when it's installed.Testing
The 2 skipped tests pass when
mempalaceis installed (pip install mempalace).