Skip to content

feat: add configurable multilingual embedding model support - #1

Closed
NickShtefan wants to merge 1 commit into
mainfrom
feat/multilingual-embedding
Closed

feat: add configurable multilingual embedding model support#1
NickShtefan wants to merge 1 commit into
mainfrom
feat/multilingual-embedding

Conversation

@NickShtefan

@NickShtefan NickShtefan commented Apr 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Centralizes embedding function selection through get_embedding_function() in config.py
  • Configurable via MEMPALACE_EMBEDDING_MODEL env var or embedding_model in config.json
  • Falls back to ChromaDB default (all-MiniLM-L6-v2) when no model configured or sentence-transformers not installed
  • Adds [multilingual] optional dependency: pip install mempalace[multilingual]
  • Reduces default chunk size from 800→450 to respect model token limits (Chunk size (800) exceeds embedding model token limit (256 tokens / ~512 chars) MemPalace/mempalace#390)

Motivation

The default all-MiniLM-L6-v2 model is English-centric. Non-English users get poor search quality. Tested with intfloat/multilingual-e5-base on 245 Russian blog posts — relevance scores improved from 0.19–0.40 to 0.70–0.77.

Usage

# Install with multilingual support
pip install mempalace[multilingual]

# Configure model (env var or config.json)
export MEMPALACE_EMBEDDING_MODEL=intfloat/multilingual-e5-base

# Or in ~/.mempalace/config.json:
# { "embedding_model": "intfloat/multilingual-e5-base" }

# Re-mine after changing model (embeddings are model-specific)
mempalace mine ~/my-project

Test plan

  • pytest tests/test_multilingual.py -v — all 11 tests pass
  • Default behavior unchanged (no config → ChromaDB default model)
  • Setting MEMPALACE_EMBEDDING_MODEL selects the specified model
  • Graceful fallback when sentence-transformers not installed
  • Search quality on non-English content with multilingual model

Closes MemPalace#231
Refs MemPalace#390

kAI Shtefan @claude
feat: add configurable multilingual embedding model support
6edac2e 
Centralizes embedding function selection via get_embedding_function()
in config.py. Supports MEMPALACE_EMBEDDING_MODEL env var and
embedding_model in config.json. Falls back to ChromaDB default when
sentence-transformers is not installed.

All ChromaDB collection access points now pass the configured
embedding_function, ensuring consistent embeddings across mine,
search, MCP server, layers, graph traversal, and CLI commands.

Also reduces default chunk size from 800 to 450 to fit within
embedding model token limits.

Closes MemPalace#231
Refs MemPalace#390

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@NickShtefan NickShtefan closed this Apr 9, 2026
NickShtefan pushed a commit that referenced this pull request Apr 26, 2026
@felipetruman @claude @igorls
fix: resolve hooks_cli.py merge conflict + add mine_global_lock tests
7e18a70 
- Resolve UU conflict in hooks_cli.py: take develop/HEAD approach
  (mine synchronously via _mine_sync, then pass through unconditionally).
  _mine_sync already catches subprocess.TimeoutExpired — fixes Copilot #1.
- Add tests/test_palace_locks.py: 4 tests covering mine_global_lock
  non-blocking semantics (acquire, second-acquire raises MineAlreadyRunning,
  reusable after release, release on exception) — fixes Copilot MemPalace#4.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NickShtefan pushed a commit that referenced this pull request Apr 26, 2026
@felipetruman @igorls
fix: address PR review — per-palace lock, MCP server path, hook timeo…
99b820c 
…ut, tests

Addresses the six Copilot review comments on the initial commit.

1) MemPalace#6 (critical) — mcp_server.py `_get_collection` bypassed ChromaBackend

   The MCP server creates its palace collection directly via
   `chromadb.PersistentClient.get_or_create_collection` in `_get_collection`,
   not through `ChromaBackend.get_collection`. That path was missing the
   `hnsw:num_threads=1` metadata, so the primary crash surface for MemPalace#974
   and MemPalace#965 was untouched by the original patch. Fixed by passing
   `hnsw:num_threads=1` at the mcp_server create site too. Documented
   in a code comment that the setting is only honored at creation
   time — existing palaces created before this fix still need a
   `mempalace nuke` + re-mine to gain the protection.

2) MemPalace#3 — mine_global_lock over-serialized mines across unrelated palaces

   Replaced the single global lock file `mine_global.lock` with a
   per-palace lock keyed by `sha256(os.path.abspath(palace_path))`
   (`mine_palace_<hash>.lock`). Mines against the same palace still
   collapse to a single runner (the correctness boundary), but mines
   against *different* palaces are now free to run in parallel.
   `mine_global_lock` is kept as a backward-compatible alias for
   `mine_palace_lock` so any external callers that imported the
   previous name keep working.

3) #1 — hook_precompact swallowed OSError but not subprocess.TimeoutExpired

   `subprocess.run(..., timeout=60)` raises `TimeoutExpired` on slow
   palaces. The previous `except OSError` clause didn't catch it, so
   the hook could raise and fail to emit any JSON decision — leaving
   the harness without a block/passthrough signal. Fixed by catching
   `(OSError, subprocess.TimeoutExpired)` together and always falling
   through to the block decision so the hook reliably emits a response.

4) #2 + MemPalace#4 — tests

   - tests/test_hooks_cli.py: added
     `test_precompact_first_two_attempts_block`,
     `test_precompact_passes_through_after_cap`, and
     `test_precompact_counter_is_per_session` to lock in the MemPalace#955
     deadlock fix.
   - tests/test_palace_locks.py (new): covers `mine_palace_lock`
     single-acquire, reuse-after-release, cross-process serialization
     on the same palace, non-interference across different palaces,
     path normalization, and the `mine_global_lock` back-compat alias.

5) MemPalace#5 — known limitation, documented but not auto-fixed

   Copilot suggested detecting collections missing `hnsw:num_threads=1`
   and calling `collection.modify(metadata=...)` to retrofit existing
   palaces. Verified against chromadb 1.5.7: `modify(metadata=...)`
   replaces metadata rather than merging, and re-passing
   `hnsw:space="cosine"` then raises `ValueError: Changing the
   distance function of a collection once it is created is not
   supported currently.` The HNSW runtime configuration
   (`configuration_json`) also does not expose `num_threads` in
   chromadb 1.5.x, so the flag appears to be read only at creation
   time. Rather than paper over the limitation with a best-effort
   `modify` that silently drops `hnsw:space`, documented in the
   mcp_server comment that pre-existing palaces need a
   `mempalace nuke` + re-mine to gain the protection. Fresh palaces
   are always protected.

Testing
- pytest tests/test_palace_locks.py tests/test_hooks_cli.py
  tests/test_backends.py tests/test_cli.py → **98 passed, 0 failed**.
- Runtime validation with two concurrent `mempalace mine` calls:
  - Different palaces → both complete in parallel ✓
  - Same palace     → one completes, the other exits with
    "another `mine` is already running against <palace> — exiting
    cleanly." ✓
NickShtefan added a commit that referenced this pull request May 11, 2026
@NickShtefan @claude
fix: address davidglidden's two findings on multilingual embedding PR
ceccbfa 
Two follow-ups to PR MemPalace#442 surfaced by davidglidden's 8 MB French/Spanish/
English typography corpus test on commit c652c2f
(MemPalace#442 (comment)).

1) `--auto-mine` ignored `--palace`. `_maybe_run_mine_after_init` read
   `cfg.palace_path` while the matching init step honoured `args.palace`,
   so `mempalace init --palace X --auto-mine` initialised X but mined
   into the default palace — leaving X empty and polluting the default.
   Use the same `getattr(args, "palace", None)` fallback pattern as
   `cmd_init`.

2) Silent fallback to MiniLM when sentence-transformers was missing for a
   non-default model. `init --model BAAI/bge-m3` against a venv without
   the `[multilingual]` extra succeeded, stamped `embedding_model =
   BAAI/bge-m3` into collection metadata, and bound the embedding
   function to all-MiniLM-L6-v2 (384d, English-leaning) — every later
   query returned the wrong model's neighbours under a multilingual
   label. Probe `import sentence_transformers` directly and raise
   ImportError with the install hint. (ChromaDB transmutes the missing
   dep to a generic ValueError inside `SentenceTransformerEmbeddingFunction`
   so catching ImportError around the chromadb call alone is insufficient.)

Cascading fix in `palace.get_collection`: the non-silent embedder change
exposed that chromadb's internal EF-conflict validator fires before our
domain `EmbeddingModelMismatchError` could, masking the actionable
message. Read `embedding_model` from collection metadata up front and
raise `EmbeddingModelMismatchError` (or take the explicit force-path
that opens the collection with the stored EF and re-stamps metadata)
before instantiating the new EF. Also avoids loading a heavy model only
to discard it on mismatch.

Tests:
- Replace `TestGetEmbeddingFunctionFallback` (asserted silent fallback)
  with `TestGetEmbeddingFunctionMissingDependency` (asserts hard raise +
  install hint), plus a guard that the default `chromadb-default` path
  is unaffected when sentence-transformers is missing.
- Add a `sentence_transformers` sys.modules stub fixture so the existing
  mocked-`SentenceTransformerEmbeddingFunction` tests still pass on a
  stock `[dev]` env (no `[multilingual]` extra required for unit tests).
- Add `test_maybe_run_mine_honours_args_palace_over_cfg` and
  `test_maybe_run_mine_falls_back_to_cfg_when_palace_unset` regression
  guards for fix #1.

Full suite green (1339 passed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NickShtefan pushed a commit that referenced this pull request May 26, 2026
@igorls
fix(mcp): basename source_file in tool_get_drawer responses
7fc260f 
The MCP `mempalace_get_drawer` tool returned the entire raw drawer
metadata blob to any connected client, and the `source_file` field
in that blob is the absolute filesystem path written by the miners
(`miner.py`, `convo_miner.py` — `source_file = str(filepath)`). On
a single-user local deployment this is self-disclosure, but in
nested-agent or multi-server MCP topologies the client is a separate
trust domain and the host's directory layout has no documented
client-side use.

Mirror the mitigation that `searcher.search_memories()` already applies
on its own return path: reduce `source_file` to its basename via
`Path(source_file).name` before handing the metadata to the client.
Citations still work — the directory layout does not leak.

Companion to #1 (omit palace_path from tool_status). Same threat class,
different surface:

- mempalace_status — palace dir path     → fixed in #1
- mempalace_get_drawer — per-drawer source_file path → this PR

Other read tools were audited and do not leak host paths:
- mempalace_search    — already basenames source_file
- mempalace_list_drawers — returns wing/room/preview only
- mempalace_diary_read   — date/timestamp/topic/content only
- mempalace_reconnect    — success/message/drawers only
- mempalace_kg_*         — entity/predicate strings, counts
- mempalace_check_duplicate — wing/room/preview only

Changes:
- mempalace/mcp_server.py: tool_get_drawer() now basenames metadata.source_file
- tests/test_mcp_server.py: regression test asserting the absolute path
  and its parent directory do not appear anywhere in the response
- website/reference/mcp-tools.md: clarify the documented return shape
NickShtefan pushed a commit that referenced this pull request May 26, 2026
@milla-jovovich
fix(closets): address PR MemPalace#1584 Igor review — wire + harden T…
65f24b3 
…ier 6a

Igor's review on PR MemPalace#1584 (2026-05-22) flagged four issues:

  1. The feature wasn't wired into any production caller — the new
     ``drawer_metas`` kwarg on ``build_closet_lines`` had no real
     consumer in ``miner.py`` / ``diary_ingest.py``, so the 4-segment
     pointer form only existed in tests. Real palaces kept emitting
     the legacy 3-segment shape.
  2. ``_extract_content_date`` hallucinated dates on benign inputs.
     ``dateutil.parser.parse(fuzzy=True)`` would accept anything with
     digits and return a plausible-looking but wrong date —
     ``Version 3.3.6`` → ``2006-03-03``, ``Tested with 1000 drawers``
     → ``1000-05-22``, ``tmp_random_file_5`` → ``2026-05-05``, etc.
     Mtime almost never got reached because fuzzy returned *something*
     from filename or body first. Bad dates were silently persisted
     to ChromaDB.
  3. ``python-dateutil`` was an undeclared dependency, available only
     transitively via ``chromadb → kubernetes → python-dateutil``. Not
     a contract — upstream kubernetes has been trending toward
     stdlib-only.
  4. Two-digit-year disambiguation (70 → 19xx / 00-69 → 20xx) had no
     test pinning the boundary.

This commit addresses all four.

## Changes

### Issue 2 — kill the hallucination (the load-bearing fix)

``mempalace/miner.py``:

- New ``_VALID_DATE_RE`` gate. Three accepted shapes (all require a
  4-digit year explicitly):

    1. Numeric YYYY-MM-DD with ``[-/.\\s]`` separators
       (covers ISO and space-normalized filenames)
    2. Month-name + day + year ("November 8 2024", "Nov 8 2024")
    3. Day + month-name + year ("8 November 2024")

  Partial dates ("2024-06", "April 6", "notes.2024") are
  DELIBERATELY rejected — without all three components we'd pad from
  today's date, which is hallucination not extraction.

- ``_try_filename_date`` and ``_try_content_body_date`` now run the
  gate BEFORE invoking dateutil, and pass ``fuzzy=True`` is REMOVED.
  Dateutil only runs in strict mode on a substring the gate already
  validated.

### Issue 1 — wire the feature into production

``mempalace/miner.py`` batched-upsert path:

- Accumulate ``batch_metas`` across all batches into ``all_metas``
- Pass ``drawer_metas=all_metas`` to ``build_closet_lines``

End-to-end integration test added that mines a real file with a
filename-derived content date and asserts the produced closet
documents contain the 4-segment pointer with that date.

``diary_ingest.py`` is left as-is for this PR. Diary entries are
entry-keyed, not chunk-keyed — they carry no natural
``line_start`` / ``line_end``, so the 4-segment form would return
None for them regardless. Wiring the diary path can land cleanly in
a follow-up once Tier 6a gains an "approximate line range for diary
entries" story.

### Issue 3 — declare the dateutil dependency

``pyproject.toml``: add ``python-dateutil>=2.8`` to
``[project].dependencies``. One-line change; cheaper than the
stdlib-only refactor alternative and keeps the natural-language
recall surface.

### Issue 4 — pin the two-digit-year boundary

Four new tests cover the 1969/1970/1999/2000 corner cases of the
slash-date locale heuristic.

## Tests added (RED-first then GREEN)

  tests/test_miner.py::TestExtractContentDate (11 new):
    Hallucination cases verbatim from Igor's review:
    - test_no_hallucination_junk_filename_with_trailing_digit
    - test_no_hallucination_untitled_with_index
    - test_no_hallucination_filename_year_only
    - test_no_hallucination_filename_year_and_month_only
    - test_no_hallucination_content_with_issue_number
    - test_no_hallucination_content_with_count
    - test_no_hallucination_content_with_version_number
    Two-digit-year boundary cases:
    - test_two_digit_year_69_is_2069
    - test_two_digit_year_70_is_1970
    - test_two_digit_year_99_is_1999
    - test_two_digit_year_00_is_2000

  tests/test_closets.py::TestMinerClosetRebuild (1 new):
    - test_production_miner_emits_4_segment_pointers_with_content_date
      (regression for Issue #1 — real ``mine()`` end-to-end produces
      4-segment closet pointers via the new ``drawer_metas`` wiring)

## Verification

  pytest tests/test_miner.py tests/test_closets.py
         tests/test_format_miner.py tests/test_palace.py
    → 242 passed, 2 skipped, 0 regressions

  pytest tests/test_miner.py::TestExtractContentDate
    → 26 passed (15 prior + 11 new)

  pytest tests/test_closets.py::TestMinerClosetRebuild
    → end-to-end wiring test GREEN

  Sanity (Igor's exact repros):
    "tmp_random_file_5"           → None (was: 2026-05-05)
    "untitled-1"                  → None (was: 2026-05-01)
    "notes.2024.md"               → None (was: 2024-05-22)
    "2024-06.md"                  → None (was: 2024-06-22)
    "Bug fix for issue 42 in module 7" → None (was: 2042-07-22)
    "Tested with 1000 drawers"    → None (was: 1000-05-22)
    "Version 3.3.6 released"      → None (was: 2006-03-03)

  Real dates still extract correctly:
    "2024-11-08.md"               → "2024-11-08"
    "April-6th-2011-notes.md"     → "2011-04-06"
    "Nov-8-2024.md"               → "2024-11-08"

  ruff check + ruff format --check (pinned 0.15.9)
    → All checks passed

  OrbStack triple-Linux verify (Py 3.9 / 3.11 / 3.13)
    → all targeted tests pass; python-dateutil installs explicitly
       via the new declared dependency.
NickShtefan pushed a commit that referenced this pull request Jul 17, 2026
@FBISiri
feat: add Gemini CLI / AI Studio JSON session import support
a9a2c35 
Adds _try_gemini_json parser to normalize.py for three layouts:

  1. Gemini API contents format (~/.gemini/sessions/*.json):
     {"contents": [{"role": "user", "parts": [{"text": "..."}]}, ...]}
  2. Messages-wrapper variant:
     {"messages": [{"role": "user", ...}, {"role": "model", ...}]}
  3. Flat top-level list with role="model".

This complements the existing _try_gemini_jsonl parser (which handles
~/.gemini/tmp/<hash>/chats/session-*.jsonl with session_metadata
sentinel) — JSONL covers Gemini CLI runtime sessions, JSON covers
exported / Studio-saved transcripts.

## Review feedback addressed (PR MemPalace#204)

bgauryy review:
- #1 Parser-precedence bug: _try_gemini_json runs *before*
  _try_claude_ai_json so the {"messages":[..., role=model, ...]}
  layout is no longer silently claimed by the Claude parser. The
  Gemini parser's has_model_role guard prevents false-positives
  against Claude / ChatGPT data.
- #2 Layout 2a coverage: TestGeminiJson.test_messages_wrapper_format
  + test_messages_wrapper_does_not_get_claimed_by_claude pin the
  fix in place.
- MemPalace#3 Test conflicts with current main: rebased onto develop;
  tests restructured into TestGeminiJson class.
- MemPalace#4 tempfile/os.unlink → pytest tmp_path everywhere.
- MemPalace#5 elif not text → else (the elif branch was dead).
- MemPalace#6 Module docstring updated to mention Google AI Studio.

Tests: 9 new cases in TestGeminiJson covering all three layouts,
multi-part text joining, non-text part skipping, has_model_role
disambiguation, dispatch-chain regression for review #1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature] Add Multilingual Support

1 participant

@NickShtefan