docs: reassess cherry-pick-1085 — absorbed into fork architecture (#165) - #197
Conversation
Issue #165 asked to drop the MemPalace#1085 cherry-pick on the next upstream sync, on the premise that upstream MemPalace#1185 (merged) had superseded it. That premise held on 2026-05-16 but is no longer accurate: in the 73 commits to miner.py / convo_miner.py / format_miner.py since the cherry-pick landed, the fork built on top of these primitives in ways upstream MemPalace#1185 does not provide: 1. add_drawers() is a fork-only public API that returns (added, batch_ids, warnings) and is wired to room-taxonomy validation (#86). Consumed by test_room_taxonomy.py. 2. DRAWER_UPSERT_BATCH_SIZE / CHROMA_BATCH_LIMIT sub-batching knob — upstream does one giant upsert per file (OOM risk on pathological files); fork sub-batches in groups of 1000. Used from miner.py, convo_miner.py, format_miner.py and monkeypatched by tests. 3. _build_drawer_metadata carries fork-only Tier 6a extensions (line_start, line_end, content_date) that closet pointers depend on. Tested at tests/test_miner.py. Updates the fork-changes.yaml entry to mark the cherry-pick as "absorbed into fork architecture" rather than "drop on next sync", and regenerates FORK_CHANGELOG.md from the YAML. Closes #165.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project documentation to correct the status of a previous cherry-pick. It clarifies that the changes from MemPalace#1085 remain essential to the fork's architecture due to specific public APIs, sub-batching knobs, and metadata extensions that are not present in the upstream implementation. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the docs/fork-changes.yaml manifest to document the decision to retain a previously integrated cherry-pick, detailing specific fork-only architectural features that justify its continued use. The reviewer feedback focuses on improving documentation consistency by recommending the use of full repository-relative paths for all source and test file references mentioned in the text.
| architecture*. Over the 73 commits to ``miner.py``, | ||
| ``convo_miner.py``, and ``format_miner.py`` since the cherry-pick |
There was a problem hiding this comment.
For consistency with other file references in this manifest (e.g., line 2135 and line 34), please use the full repository-relative paths for these source files.
architecture*. Over the 73 commits to ``mempalace/miner.py``,
``mempalace/convo_miner.py``, and ``mempalace/format_miner.py`` since the cherry-pick| ``process_file`` and exposed no public batch function. The | ||
| fork's ``add_drawers`` returns the fork-only | ||
| ``(added, batch_ids, warnings)`` tuple wired to room-taxonomy | ||
| validation (#86) and is consumed by ``test_room_taxonomy.py``. |
| ``miner.py``, ``convo_miner.py``, and ``format_miner.py``, | ||
| and monkeypatched by ``test_miner.py`` / | ||
| ``test_convo_miner_unit.py`` to drive the sub-batch loops. |
There was a problem hiding this comment.
To maintain consistency across the manifest, file references should include their directory prefixes (e.g., mempalace/ for source files and tests/ for test files).
``mempalace/miner.py``, ``mempalace/convo_miner.py``, and ``mempalace/format_miner.py``,
and monkeypatched by ``tests/test_miner.py`` /
``tests/test_convo_miner_unit.py`` to drive the sub-batch loops.There was a problem hiding this comment.
Pull request overview
Updates the fork-change documentation to reflect that the upstream MemPalace#1085 cherry-pick should be retained (it’s now considered “absorbed into fork architecture”), rather than being dropped on the next upstream sync, and regenerates the rendered changelog from the canonical YAML manifest.
Changes:
- Reassesses the
cherry-pick-1085entry text to document why it’s no longer redundant with upstream MemPalace#1185. - Regenerates
FORK_CHANGELOG.mdfromdocs/fork-changes.yamlto keep rendered docs in sync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| FORK_CHANGELOG.md | Updates the rendered changelog entry with the new “reassessed and kept” narrative for cherry-pick MemPalace#1085. |
| docs/fork-changes.yaml | Updates the canonical manifest entry body for cherry-pick MemPalace#1085 (source for changelog rendering). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cherry-pick is now a no-op against develop; drop on next sync. | ||
| **2026-05-24:** reassessed and kept — *absorbed into fork | ||
| architecture*. Over the 73 commits to ``miner.py``, |
| ``process_file`` and exposed no public batch function. The | ||
| fork's ``add_drawers`` returns the fork-only | ||
| ``(added, batch_ids, warnings)`` tuple wired to room-taxonomy | ||
| validation (#86) and is consumed by ``test_room_taxonomy.py``. |
| cherry-pick is now a no-op against develop; drop on next sync. | ||
| **2026-05-24:** reassessed and kept — *absorbed into fork | ||
| architecture*. Over the 73 commits to ``miner.py``, | ||
| ``convo_miner.py``, and ``format_miner.py`` since the cherry-pick |
| ``process_file`` and exposed no public batch function. The | ||
| fork's ``add_drawers`` returns the fork-only | ||
| ``(added, batch_ids, warnings)`` tuple wired to room-taxonomy | ||
| validation (#86) and is consumed by ``test_room_taxonomy.py``. |
Summary
Issue #165 asked to drop the MemPalace#1085 cherry-pick (commit
6be6fff) onthe next upstream sync, on the premise that upstream MemPalace#1185 (merged)
had fully superseded it. That premise held on 2026-05-16 but is no
longer accurate. This PR reassesses the entry, marks it as absorbed
into fork architecture rather than "drop on next sync", and
regenerates
FORK_CHANGELOG.md.Why not drop
In the 73 commits to
miner.py/convo_miner.py/format_miner.pysince the cherry-pick landed, the fork built on top of these
primitives in ways upstream MemPalace#1185 does not provide:
add_drawers()is a fork-only public API — upstream perf(mining): batch per-chunk upserts + optional GPU acceleration MemPalace/mempalace#1185inlined batching into
process_fileand exposed no public batchfunction. The fork's
add_drawersreturns the fork-only(added, batch_ids, warnings)tuple wired to room-taxonomyvalidation (feat: soft-warn on non-canonical room names instead of hard FK rejection #86) and is consumed by
tests/test_room_taxonomy.py.DRAWER_UPSERT_BATCH_SIZE/CHROMA_BATCH_LIMITsub-batchingknobs — upstream does one giant upsert per file (OOM risk on
pathological files); the fork sub-batches in groups of 1000. The
knob is referenced from
miner.py,convo_miner.py, andformat_miner.py, and monkeypatched bytest_miner.py/test_convo_miner_unit.pyto drive the sub-batch loops._build_drawer_metadatacarries fork-only Tier 6a extensions(
line_start,line_end,content_date) that closet pointersdepend on. Tested at
tests/test_miner.py.Test plan
scripts/check-docs.shpasses (5/5 checks green)FORK_CHANGELOG.mdmatches the YAMLNo source changes — docs only.
Closes #165.