Repository navigation
feat: add logical delete/forget API - #59
Merged
Merged
Conversation
beinan
added a commit
that referenced
this pull request
Jul 25, 2026
## The problem `python-test.yml` ran `pytest python/tests/` with `working-directory: python`, which resolves to **`python/python/tests/`** — a single stale file with 3 tests. The real suite, **`python/tests/` (24 files, 179 tests)**, was never collected. pytest exits 0 on a valid-but-nearly-empty directory, so this failed silently. Git history dates the split: `python/python/tests/` last changed in #59; `python/tests/` is current through #158. **~100 PRs merged ungated** — rollout store, async wrappers, export, ingestion. Two further layers of silencing, each sufficient on its own: - Only bare `pytest` was installed, never the `[tests]` extra — so `pytest-asyncio` was absent. - No `asyncio_mode` anywhere, so the 12 `async def` tests were collected, never awaited, and **passed without executing a line**. The same path bug appeared three more times: the doctest step was a permanent no-op (an `if` on a path that never exists, plus `|| echo`), and `ruff format --check python/` / `ruff check python/` **never linted the 24 test files**. `.codex/.../run_ci_checks.sh` pointed at a `rust/` directory that does not exist, so `set -e` killed it before any Python check ran. ## CI changes - Pin collection with `testpaths = ["tests"]` so the ambiguous relative path cannot resurface. - `asyncio_mode = "strict"` + `--strict-markers --strict-config` — a mistyped marker now fails instead of silently disarming a test. - Install `[lance-python,tests]`; lint `.` instead of `python/`; make the doctest step real. - Point the helper script at the real `crates/` layout. ## Turning the suite on surfaced 19 real failures **1. Embedded `RolloutStore` silently discarded every write.** This is the significant one. #181 made `add` durable-but-not-visible, with visibility driven by a server-side sweeper. The embedded path has no sweeper, and `flush()` was never exposed to Python — so: ```python s = RolloutStore.open(d) s.add({'id':'a','rollout_id':'t1'}) # -> {'version': 2, 'ids': ['a'], 'count': 1} s.list() # -> [] ... and still [] after reopen ``` Data acknowledged, then unreadable forever. This PR exposes `flush()` through the facade, PyO3, and both sync and async Python wrappers. **2. `test_search.py`'s `DummyInner` fake had drifted** from the real binding signature (missing `include_binary`/`include_embedding`), so all 55 tests raised `TypeError` on contact with current code. **3. Remote rollout tests** asserted read-your-write against a server whose default flush interval is 30s. They now set `ROLLOUT_FLUSH_INTERVAL_SECS=1` and poll — asserting the row *arrives* rather than pinning to the interval. The 3 stale tests are **moved** into `python/tests/` rather than dropped (they cover `snapshot`/`fork`/`memory://` and still pass), and the orphaned directory is removed so nothing can silently collect it again. ## Verification - **182 passed, 2 skipped** (179 real + 3 rescued). - `ruff format --check .`, `ruff check .`, `pyright` — all clean. - `cargo fmt --all -- --check` and clippy clean on the changed crates. - Confirmed the async tests now genuinely execute by injecting a failing assertion and watching it fail — it would not have before this change. ## Note on scope This is deliberately scoped to *making CI honest* plus the failures that gating surfaced. It does **not** address: publish workflows having no `needs:` on any test job (a release cut from a red `main` ships to PyPI/crates.io), `cargo test` covering only 2 of 8 crates and `--lib` only, or the `DummyInner` fake asserting hardcoded literals so RRF ranking is still never exercised. Happy to file those separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Note: broader local python/tests run has 4 pre-existing raw-fragment/compaction failures with MemWAL visibility (test_compaction..., test_persistence...). Focused delete tests and the exact workflow test path pass.
Closes #55