Skip to content

feat(miner): symbol_header_prefix kwarg in chunk_text - #1508

Open
jphein wants to merge 2 commits into
MemPalace:developfrom
techempower-org:pr/1384-symbol-header-prefix
Open

feat(miner): symbol_header_prefix kwarg in chunk_text#1508
jphein wants to merge 2 commits into
MemPalace:developfrom
techempower-org:pr/1384-symbol-header-prefix

Conversation

@jphein

@jphein jphein commented May 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an optional keyword-only symbol_header_prefix callback to mempalace.miner.chunk_text. When supplied, the returned header is prepended to each chunk with a blank-line separator before storage. Default None preserves current behavior exactly — every existing call site is unaffected.

def chunk_text(
    content: str,
    source_file: str,
    *,
    symbol_header_prefix=None,
) -> list:
    ...

Why

Discussed in #1384. Lets representation-axis experiments — AST-lite symbol headers (function name + class path + imports + decorators), encoder fine-tunes that benefit from explicit symbol disambiguation (cf. nakata-app's adaptmem FT-Code series), and any future enrichment that needs to change "what the encoder sees" — stack on the existing chunk path without forking it.

The callback signature (chunk_text, source_file, chunk_index) -> str gives the caller everything they need to do AST inspection or per-chunk metadata lookup; returning an empty string is a per-chunk no-op.

Example: AST-lite caller

import ast
from mempalace.miner import chunk_text

def ast_symbol_header(chunk: str, source_file: str, idx: int) -> str:
    if not source_file.endswith(".py"):
        return ""
    try:
        tree = ast.parse(chunk)
    except SyntaxError:
        return ""
    names = [n.name for n in tree.body if isinstance(n, (ast.FunctionDef, ast.ClassDef))]
    return f"[{source_file}] {' | '.join(names)}" if names else ""

chunks = chunk_text(content, source_file, symbol_header_prefix=ast_symbol_header)

Tradeoffs

  • Chunks get ~50–200 chars longer when a header fires. The caller controls the budget.
  • No behavior change for the 11 existing call sites (verified with the full test suite on our fork).
  • Keyword-only signature so positional callers can't accidentally pass a non-callable.

Test plan

  • tests/test_miner.py (62 cases) passes unchanged on the techempower-org/mempalace fork where this patch first shipped.
  • Default-None path: byte-identical chunk output vs main on the mempalace package directory.
  • CI: workflow approval needed (fork PR).

Reference: techempower-org/mempalace@0a3340e.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 15, 2026 06:36
@jphein
jphein requested a review from milla-jovovich as a code owner May 15, 2026 06:36

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a symbol_header_prefix parameter to the chunk_text function in mempalace/miner.py, allowing for optional chunk enrichment by prepending a header generated by a callback. Review feedback points out that this new parameter is not yet exposed in the primary mining entry points, which limits its accessibility in standard workflows. Additionally, it was noted that the current caching mechanism does not account for changes in the enrichment logic, and a docstring update was suggested to clarify the callback's parameter names to avoid confusion with the function itself.

Comment thread mempalace/miner.py
content: str,
source_file: str,
*,
symbol_header_prefix=None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The symbol_header_prefix parameter is added here but not yet exposed in the process_file function (line 882) or the mine entry point. This makes the enrichment feature inaccessible for the standard mining workflow without further modifications to those functions. Additionally, since file_already_mined only checks source_mtime and NORMALIZE_VERSION, changes to the logic within a provided callback will not automatically trigger a re-mine of already processed files.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch on both. Wired symbol_header_prefix through process_file and the mine entry point (mine -> _mine_impl -> process_file -> chunk_text) as an optional kwarg, default None = exactly current behavior, in 57550a6 — so it's now reachable from the standard mining workflow without forking the path. On the cache key: agreed it's out of scope to solve here, so I documented it instead — both chunk_text and process_file docstrings now note that file_already_mined keys only on source_mtime + NORMALIZE_VERSION, so changing the callback does not auto-invalidate already-mined files; a re-mine needs --force or a NORMALIZE_VERSION bump. Added tests proving the kwarg flows end-to-end and that the default path is byte-identical to the no-kwarg call.

Comment thread mempalace/miner.py Outdated
source_file: file path used for room/topic inference and (when
``symbol_header_prefix`` is supplied) chunk enrichment.
symbol_header_prefix: optional callable
``(chunk_text, source_file, chunk_index) -> str``. When

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Renaming the first argument of the callback from chunk_text to chunk (or content) in the documentation would avoid confusion with the name of the function itself (chunk_text).

Suggested change
``(chunk_text, source_file, chunk_index) -> str``. When
``(chunk, source_file, chunk_index) -> str``. When

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done — applied your exact wording in 57550a6. The docstring now documents the callback as (chunk, source_file, chunk_index) -> str, so the first arg no longer collides with the function name chunk_text.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jphein

jphein commented May 15, 2026

Copy link
Copy Markdown
Collaborator Author

Closing — premature on my side; want to do more local validation before opening upstream. Will revisit. Patch lives on techempower-org fork for now.

@jphein jphein closed this May 15, 2026
@jphein jphein reopened this May 17, 2026
@jphein

jphein commented May 17, 2026

Copy link
Copy Markdown
Collaborator Author

Local validation landed — reopening with new data.

When I closed this on 2026-05-15 I said I wanted to do more local validation before opening upstream. That validation just landed as a 2×2 encoder × chunker ablation on 48 git-derived markdown probes. Posted in the parent discussion thread at #1384 discussioncomment-16950816. The relevant numbers:

Encoder Chunker R@5
base-MiniLM-L6-v2 paragraph 0.6250
base-MiniLM-L6-v2 heading-aware 0.5000
FT-300 (LongMemEval-domain) paragraph 0.5833
FT-300 (LongMemEval-domain) heading-aware 0.5625

Δ (heading-aware vs paragraph), per encoder:

  • base-MiniLM: −0.1250 (12.5pp swing)
  • FT-300: −0.0208 (2.1pp — within noise)

The kwarg is encoder-conditional: chunking-axis variance is 12.5pp at base-MiniLM and ~2pp post-FT. Symbol-header enrichment has headroom to move recall when the encoder hasn't been calibrated to the domain — which is where most projects sit until they invest in domain FT. That gives this PR a clearer value proposition than the original framing did.

Concretely:

  1. Default None preserves existing behavior byte-identically — non-callers see no change. That hasn't changed since 2026-05-15.
  2. The callback signature (chunk_text, source_file, chunk_index) -> str gives the caller enough to do AST inspection per-chunk for symbol headers, or any other per-chunk enrichment that wants to mutate "what the encoder sees" — without forking chunk_text.
  3. The use case is now empirically grounded — at base-MiniLM (which is what most non-tuned MemPalace installs use), chunking-strategy variance dominates the retrieval-quality story. The kwarg is a no-API-change lever to test enrichment strategies against that variance.

xg-gh-25's "skip chunking for code, use structured extraction + call graph" framing in #1384 is the orthogonal answer for code-specific retrieval, but it requires a substrate rewrite (entity extraction + graph traversal). This PR is the cheap lever that lets the encoder-not-yet-FT'd common case experiment with representation enrichment without forking miner.

Happy to add the AST-lite example caller into a test fixture if useful, or land just the kwarg surface and let representation experiments live as separate adapters.

@igorls / @milla-jovovich — would value your take on whether the encoder-conditional framing makes the kwarg compelling enough to land, or whether the better path is to keep the upstream chunk_text surface minimal and let representation enrichment live downstream.

🫏

@jphein

jphein commented May 17, 2026

Copy link
Copy Markdown
Collaborator Author

Quick precedent worth noting: @igorls just shipped #1519 (merged 2026-05-15) with structurally the same shape as the symbol_header_prefix kwarg here — a small surgical addition to the miner config surface that uses a sentinel return value to distinguish "unset / unusable" from "explicit value," letting the caller decide the fallback policy. That PR's MempalaceConfig.min_chunk_size_explicit() returns the validated int or None; this PR's symbol_header_prefix callback returns the prefix string or "". Same shape, same low-risk migration path (default behavior preserved byte-identically for non-callers).

Cross-referenced here in case the maintainer-review takeaway from #1519 — "small kwarg surfaces are fine when the default preserves existing behavior and the sentinel is unambiguous" — is the framing this PR also fits under. Both PRs gate optional enrichment behind a kwarg the caller opts into; neither changes anything for existing call sites.

🫏

@igorls
igorls changed the base branch from main to develop May 17, 2026 19:35
@jphein
jphein force-pushed the pr/1384-symbol-header-prefix branch 2 times, most recently from 2686883 to 43fd508 Compare May 23, 2026 19:37
jphein and others added 2 commits June 14, 2026 07:05
Optional keyword-only callable ``symbol_header_prefix(chunk_text,
source_file, chunk_index) -> str``. When supplied, the returned
header is prepended to each chunk with a blank-line separator
before storage. Default ``None`` preserves current behavior
exactly — all existing callers are unaffected.

Discussed in MemPalace#1384 — gives AST-lite symbol-header enrichment (and
other representation-axis experiments like adaptmem's FT-Code +
header stacking) a clean seam without forking ``chunk_text``.
Chunk content gets ~50–200 chars longer when a header fires; the
caller controls the budget.

Existing miner test suite (tests/test_miner.py, 96 cases) passes
unchanged on the techempower-org fork where the kwarg was first
shipped.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address Gemini review on MemPalace#1508. The kwarg was reachable only by calling
chunk_text directly; wire it through the standard mining entry points so
the enrichment seam is usable from mine() without forking the path:

  mine -> _mine_impl -> process_file -> chunk_text

symbol_header_prefix is an optional kwarg (default None) at every level,
fully backward-compatible — no existing call site changes behavior.

Also from the review:
- Document the re-mine caveat in chunk_text + process_file docstrings:
  file_already_mined keys only on source_mtime + NORMALIZE_VERSION, so
  changing the callback does not auto-invalidate already-mined files;
  re-mine needs --force or a NORMALIZE_VERSION bump.
- Rename the documented callback arg chunk_text -> chunk in the docstring
  to avoid confusion with the function name: (chunk, source_file,
  chunk_index) -> str.

Tests: prove symbol_header_prefix flows mine -> process_file -> chunk_text,
that the default (no kwarg) reaches process_file as None, that a non-empty
header is prepended with a blank-line separator, and that the default-None
path is byte-identical to the no-kwarg call.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jphein
jphein force-pushed the pr/1384-symbol-header-prefix branch from 43fd508 to 57550a6 Compare June 14, 2026 14:10
@jphein
jphein requested a review from igorls as a code owner June 14, 2026 14:10
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.

2 participants