From 8bb5224f503d6fcd49545e33d988670e841492ce Mon Sep 17 00:00:00 2001 From: Beinan Date: Sat, 25 Jul 2026 08:50:10 +0000 Subject: [PATCH 1/3] fix(ci): run the real Python test suite (3 tests -> 182) `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, while `python/tests/` is current through #158. Roughly 100 PRs merged ungated, including the rollout store, async wrappers, export, and ingestion. Two further layers of silencing, each sufficient alone: - 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 (guarded by 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"]` in `[tool.pytest.ini_options]` so the ambiguous relative path cannot resurface; - `asyncio_mode = "strict"` + `--strict-markers --strict-config` so a mistyped marker fails instead of silently disarming a test; - install `[lance-python,tests]`; lint `.` instead of `python/`; - point the helper script at the real `crates/` layout, matching CI. Turning the suite on surfaced 19 real failures, fixed here: 1. Embedded `RolloutStore` silently discarded every write. #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 a write returned `{"count": 1}` and the row was unreadable -- even after reopen. Exposes `flush()` through the facade, PyO3, and both the sync and async Python wrappers. 2. `test_search.py`'s hand-written `DummyInner` 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. Verified locally: 182 passed, 2 skipped; ruff format/check and pyright 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, which it would not have done before this change. Co-Authored-By: Claude --- .../ci-pr-helper/scripts/run_ci_checks.sh | 10 ++--- .github/workflows/python-test.yml | 14 +++---- crates/lance-context/src/unified_rollout.rs | 21 ++++++++++ python/pyproject.toml | 10 +++++ python/python/lance_context/api.py | 24 +++++++++++ python/src/lib.rs | 7 ++++ python/{python => }/tests/test_context.py | 0 python/tests/test_rollout.py | 11 +++++ python/tests/test_rollout_remote.py | 40 ++++++++++++++++--- python/tests/test_search.py | 8 ++++ 10 files changed, 127 insertions(+), 18 deletions(-) rename python/{python => }/tests/test_context.py (100%) diff --git a/.codex/skills/ci-pr-helper/scripts/run_ci_checks.sh b/.codex/skills/ci-pr-helper/scripts/run_ci_checks.sh index c0021252..9242ea37 100755 --- a/.codex/skills/ci-pr-helper/scripts/run_ci_checks.sh +++ b/.codex/skills/ci-pr-helper/scripts/run_ci_checks.sh @@ -15,12 +15,12 @@ fi cd "$root" -cargo fmt --manifest-path rust/lance-context/Cargo.toml -- --check -cargo clippy --manifest-path rust/lance-context/Cargo.toml --all-targets -- -D warnings -cargo test --manifest-path rust/lance-context/Cargo.toml +cargo fmt --all -- --check +cargo clippy --workspace --all-targets -- -D warnings +cargo test --workspace --all-targets cd "$root/python" uv run pytest -uv run ruff format --check python/ -uv run ruff check python/ +uv run ruff format --check . +uv run ruff check . uv run pyright diff --git a/.github/workflows/python-test.yml b/.github/workflows/python-test.yml index ea4bb162..2e776027 100644 --- a/.github/workflows/python-test.yml +++ b/.github/workflows/python-test.yml @@ -140,7 +140,7 @@ jobs: run: | uv venv source .venv/bin/activate - uv pip install pytest + uv pip install pytest pytest-asyncio - name: Download wheel uses: actions/download-artifact@v4 with: @@ -151,20 +151,18 @@ jobs: run: | source .venv/bin/activate WHEEL=$(ls dist/*.whl) - uv pip install "${WHEEL}[lance-python]" + uv pip install "${WHEEL}[lance-python,tests]" - name: Run tests working-directory: python run: | source .venv/bin/activate - pytest python/tests/ -v + pytest -v - name: Run doctests working-directory: python run: | source .venv/bin/activate - if [ -f python/lance_context/__init__.py ]; then - python -m doctest python/lance_context/__init__.py || echo "No doctests found" - fi + python -m doctest python/lance_context/__init__.py lint: runs-on: ubuntu-24.04 @@ -198,12 +196,12 @@ jobs: working-directory: python run: | source .venv/bin/activate - ruff format --check python/ + ruff format --check . - name: Run ruff lint working-directory: python run: | source .venv/bin/activate - ruff check python/ + ruff check . - name: Run pyright type check working-directory: python run: | diff --git a/crates/lance-context/src/unified_rollout.rs b/crates/lance-context/src/unified_rollout.rs index a8bcc4a3..8d8d445b 100644 --- a/crates/lance-context/src/unified_rollout.rs +++ b/crates/lance-context/src/unified_rollout.rs @@ -64,6 +64,27 @@ impl RolloutStore { .map_err(|e| ContextError::Internal(e.to_string()))?; Ok(Self::Remote(store)) } + + /// Seal the local MemWAL memtable so rows written by [`RolloutStoreApi::add`] + /// become visible to subsequent reads on this handle. + /// + /// `add` is durable on return but *not* visible on return: visibility is + /// driven by a periodic sweeper in the server. An embedded caller has no + /// sweeper, so without an explicit `flush` a write-then-read sequence + /// returns nothing. Call this before reading back rows you just wrote. + /// + /// A no-op for `Remote` stores, where the server owns flush scheduling and + /// per-request `?flush=true` provides read-your-write. + pub async fn flush(&self) -> Result<(), ContextError> { + match self { + RolloutStore::Local(s) => s + .flush() + .await + .map_err(|e| ContextError::Internal(e.to_string())), + #[cfg(feature = "remote")] + RolloutStore::Remote(_) => Ok(()), + } + } } macro_rules! dispatch_mut { diff --git a/python/pyproject.toml b/python/pyproject.toml index 1c019e93..b11d5ef4 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -60,6 +60,16 @@ tests = [ ] dev = ["ruff", "pyright"] +[tool.pytest.ini_options] +# `testpaths` is relative to this file (python/), so this resolves to +# python/tests/ -- the real suite. Without it, running pytest from python/ +# with a bare `pytest` argument could collect python/python/tests/ instead. +testpaths = ["tests"] +asyncio_mode = "strict" +# Fail on unregistered marks so a typo'd @pytest.mark.asyncio cannot silently +# turn an async test into a no-op pass. +addopts = "--strict-markers --strict-config" + [tool.ruff] lint.select = ["F", "E", "W", "I", "G", "TCH", "PERF", "B019"] diff --git a/python/python/lance_context/api.py b/python/python/lance_context/api.py index a46cf66e..11a6998b 100644 --- a/python/python/lance_context/api.py +++ b/python/python/lance_context/api.py @@ -2579,6 +2579,19 @@ def add_one(self, **fields: Any) -> dict[str, Any]: """Append a single record given as keyword arguments.""" return self.add(fields) + def flush(self) -> None: + """Make previously added rows visible to subsequent reads. + + ``add`` is durable on return but *not* visible on return: rows land in + a write-ahead log and only reach a readable generation when the + memtable is sealed. A deployed server does this on a timer; an embedded + store has no such timer, so a write-then-read sequence returns nothing + until you call this. + + No-op for stores connected to a remote server. + """ + self._sync.flush() + def list( self, limit: int | None = None, @@ -2672,6 +2685,17 @@ async def add_one(self, **fields: Any) -> dict[str, Any]: """Append a single record given as keyword arguments.""" return await self.add(fields) + async def flush(self) -> None: + """Make previously added rows visible to subsequent reads. + + For an embedded store this seals the local memtable. For a store + connected to a remote server this is a no-op: the server flushes on its + own interval (``ROLLOUT_FLUSH_INTERVAL_SECS``), which bounds how long a + just-written row stays invisible. + """ + loop = asyncio.get_running_loop() + await loop.run_in_executor(None, self._sync.flush) + async def list( self, limit: int | None = None, diff --git a/python/src/lib.rs b/python/src/lib.rs index 6cbed075..7491ba42 100644 --- a/python/src/lib.rs +++ b/python/src/lib.rs @@ -2453,6 +2453,13 @@ impl RolloutStore { self.store.version() } + /// Seal the local MemWAL memtable so previously added rows become visible + /// to subsequent reads. No-op for remote stores. + fn flush(&self, py: Python<'_>) -> PyResult<()> { + py.allow_threads(|| self.runtime.block_on(self.store.flush())) + .map_err(to_py_err) + } + /// Append rollout rows given as a JSON array of `AddRolloutRequest` objects. /// Returns a dict `{version, ids, count}`. fn add(&mut self, py: Python<'_>, records_json: &str) -> PyResult { diff --git a/python/python/tests/test_context.py b/python/tests/test_context.py similarity index 100% rename from python/python/tests/test_context.py rename to python/tests/test_context.py diff --git a/python/tests/test_rollout.py b/python/tests/test_rollout.py index 49afeb43..0030845b 100644 --- a/python/tests/test_rollout.py +++ b/python/tests/test_rollout.py @@ -38,6 +38,9 @@ def test_add_dict_list_get(store_uri): assert resp["count"] == 1 assert resp["ids"] == ["row-0"] + # `add` is durable on return but not visible until the memtable is sealed. + store.flush() + rows = store.list() assert len(rows) == 1 assert rows[0]["id"] == "row-0" @@ -60,6 +63,7 @@ def test_add_many_and_add_one(store_uri): ] ) store.add_one(id="row-3", rollout_id="traj-1", reward=3.0) + store.flush() rows = store.list() assert {r["id"] for r in rows} == {"row-0", "row-1", "row-2", "row-3"} @@ -75,6 +79,7 @@ def test_id_autogenerated_when_omitted(store_uri): (generated,) = resp["ids"] # Round-trips as a valid UUID and is fetchable by the generated id. assert uuid.UUID(generated).version == 4 + store.flush() row = store.get(generated) assert row is not None and row["id"] == generated @@ -97,6 +102,7 @@ def test_binary_payload_roundtrip(store_uri): "payload_size": len(blob), } ) + store.flush() # list/get project the blob column out (cheap metadata scans). row = store.get("art-0") @@ -121,6 +127,9 @@ def test_empty_add_rejected(store_uri): def test_reopen_sees_prior_rows(store_uri): store = RolloutStore.open(store_uri) store.add({"id": "row-0", "rollout_id": "traj-1"}) + # Seal before dropping: an unsealed memtable is durable in the WAL but is + # not replayed into a readable generation by a fresh reader. + store.flush() del store reopened = RolloutStore.open(store_uri) @@ -160,6 +169,7 @@ def test_list_filters_before_pagination(store_uri): }, ] ) + store.flush() rows = store.list( filters={"policy_version": "ckpt-2", "include_in_training": False} @@ -190,6 +200,7 @@ def test_get_trajectory_orders_rows(store_uri): {"id": "row-b", "rollout_id": "target", "sequence_order": 0}, ] ) + store.flush() rows = store.get_trajectory("target") assert [row["id"] for row in rows] == ["row-b", "row-a"] diff --git a/python/tests/test_rollout_remote.py b/python/tests/test_rollout_remote.py index 9d0b620c..1c148bec 100644 --- a/python/tests/test_rollout_remote.py +++ b/python/tests/test_rollout_remote.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio +import os import socket import subprocess import tempfile @@ -43,12 +44,35 @@ def _wait_for_health(base_url: str, timeout: float = 30.0) -> None: raise RuntimeError(f"server did not become healthy at {url}") +async def _eventually(fn, predicate, timeout: float = 15.0): + """Poll `fn` until `predicate` holds, or fail after `timeout`. + + `add` is durable on return but not visible until the server's sweeper seals + the memtable, so a read immediately after a write legitimately returns + nothing. Polling asserts the row *arrives* without pinning the test to the + flush interval. + """ + deadline = time.time() + timeout + last = None + while time.time() < deadline: + last = await fn() + if predicate(last): + return last + await asyncio.sleep(0.1) + raise AssertionError(f"condition not met within {timeout}s; last value: {last!r}") + + @pytest.fixture() def server(): if not _SERVER_BIN.exists(): pytest.skip(f"server binary not built at {_SERVER_BIN}") port = _free_port() with tempfile.TemporaryDirectory() as data_dir: + # Rows are durable on `add` but only become visible when the server's + # sweeper seals the memtable. The 30s production default would make + # every write-then-assert below hang; 1s keeps the tests honest about + # the async-visibility contract without waiting on it. + env = {**os.environ, "ROLLOUT_FLUSH_INTERVAL_SECS": "1"} proc = subprocess.Popen( [ str(_SERVER_BIN), @@ -59,6 +83,7 @@ def server(): "--data-dir", data_dir, ], + env=env, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) @@ -101,7 +126,7 @@ async def run(): ) assert resp["count"] == 2 - rows = await store.list() + rows = await _eventually(store.list, lambda r: len(r) == 2) assert {r["id"] for r in rows} == {"row-0", "row-1"} one = await store.get("row-0") @@ -125,7 +150,7 @@ async def run(): # A second connection sees the first's flushed write (durable, no # read affinity). reader = await AsyncRolloutStore.connect(server, "rl-run-2") - rows = await reader.list() + rows = await _eventually(reader.list, lambda r: len(r) == 1) assert [r["id"] for r in rows] == ["only"] asyncio.run(run()) @@ -153,8 +178,11 @@ async def run(): ] ) - rows = await store.list( - filters={"policy_version": "ckpt-7", "include_in_training": True} + rows = await _eventually( + lambda: store.list( + filters={"policy_version": "ckpt-7", "include_in_training": True} + ), + lambda r: len(r) == 1, ) assert [row["id"] for row in rows] == ["row-7"] @@ -172,7 +200,9 @@ async def run(): ] ) - rows = await store.get_trajectory("target") + rows = await _eventually( + lambda: store.get_trajectory("target"), lambda r: len(r) == 2 + ) assert [row["id"] for row in rows] == ["row-b", "row-a"] asyncio.run(run()) diff --git a/python/tests/test_search.py b/python/tests/test_search.py index fa780137..f780f4d3 100644 --- a/python/tests/test_search.py +++ b/python/tests/test_search.py @@ -31,6 +31,8 @@ def __init__(self) -> None: ] = [] self.list_calls: list[tuple[int | None, int | None, str | None]] = [] self.list_lifecycle_calls: list[tuple[bool, bool]] = [] + self.list_projection_calls: list[tuple[bool, bool]] = [] + self.search_projection_calls: list[tuple[bool, bool]] = [] self.related_calls: list[tuple[str, str | None, int | None, bool, bool]] = [] self.get_calls: list[tuple[str | None, str | None]] = [] self.delete_calls: list[tuple[str | None, str | None]] = [] @@ -274,10 +276,13 @@ def search( include_expired: bool = False, include_retired: bool = False, include_relationships: bool = False, + include_binary: bool = True, + include_embedding: bool = True, ): self.search_calls.append((vector, limit, filters_json)) self.search_lifecycle_calls.append((include_expired, include_retired)) self.search_relationship_calls.append(include_relationships) + self.search_projection_calls.append((include_binary, include_embedding)) hit = { "id": "rec-1", "external_id": "source-1", @@ -362,9 +367,12 @@ def list( filters_json: str | None, include_expired: bool = False, include_retired: bool = False, + include_binary: bool = True, + include_embedding: bool = True, ): self.list_calls.append((limit, offset, filters_json)) self.list_lifecycle_calls.append((include_expired, include_retired)) + self.list_projection_calls.append((include_binary, include_embedding)) return [ { "id": "rec-1", From 6ebedee254ee772f0db16db5deb2648506d66f13 Mon Sep 17 00:00:00 2001 From: Beinan Date: Sat, 25 Jul 2026 09:02:31 +0000 Subject: [PATCH 2/3] test(s3): xfail MemWAL S3 tests blocked on an upstream lance bug Enabling the real suite surfaced two genuine failures in `test_persistence.py` that CI had never run. They are not caused by this PR and cannot be fixed here. `Context.create` against a custom-endpoint S3 succeeds, but the first `add` fails with "Bucket '...' not found". Reproduced locally against moto with request logging: every request that reaches moto returns 200, and the failing request never arrives at all -- the WAL writer is talking to a different endpoint. Root cause is upstream, in lance 7.0.0: // lance/src/dataset/mem_wal/api.rs:607 let (store, base_path) = ObjectStore::from_uri(base_uri).await?; `mem_wal_writer` constructs a fresh object store from the URI alone, discarding the dataset's configured `storage_options`. `aws_endpoint_url` is therefore dropped and the writer targets real AWS. `create` works because it goes through the options-aware path; `add` is the first call that opens a shard writer. There is no local workaround: `ShardWriterConfig` has no field to carry storage options, so the crate cannot thread them in. The fix is upstream, switching to the `from_uri_and_params` constructor that already exists in lance-io. Marked `xfail(strict=True)` rather than skipped, so the tests keep running and turn into a hard failure the moment a lance bump fixes this -- the marker cannot silently rot. Worth noting the blast radius beyond the tests: all three stores (`store.rs:518`, `rollout_store.rs:632`, `datagen_store.rs:186`) open shard writers this way, so writes are broken against any custom-endpoint S3 -- MinIO, Ceph, R2, or moto. Only AWS-proper works today. This was invisible because the tests covering it were never executed. Full suite with all extras installed: 194 passed, 2 skipped, 3 xfailed. Co-Authored-By: Claude --- python/tests/test_persistence.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/python/tests/test_persistence.py b/python/tests/test_persistence.py index 57fb3d2d..c4517cdb 100644 --- a/python/tests/test_persistence.py +++ b/python/tests/test_persistence.py @@ -601,6 +601,23 @@ def test_time_travel_checkout(tmp_path: Path) -> None: ] +_S3_MEMWAL_XFAIL = pytest.mark.xfail( + strict=True, + reason=( + "Upstream: lance 7.0.0 mem_wal_writer builds its object store with " + "ObjectStore::from_uri(base_uri), dropping the dataset's " + "storage_options (lance/src/dataset/mem_wal/api.rs:607). The WAL " + "writer therefore ignores aws_endpoint_url and targets real AWS, so " + "`add` fails with 'bucket not found' against any custom-endpoint S3 " + "(moto, MinIO, Ceph, R2). `create` works because it goes through the " + "options-aware path. Needs from_uri_and_params upstream; there is no " + "local workaround since ShardWriterConfig cannot carry the options. " + "Remove this marker once lance is bumped past the fix." + ), +) + + +@_S3_MEMWAL_XFAIL def test_s3_round_trip_with_storage_options(moto_endpoint: str, s3_client) -> None: """Canonical path: generic storage_options dict (aligns with lance/lance-graph).""" bucket = f"context-{uuid.uuid4().hex}" @@ -619,6 +636,7 @@ def test_s3_round_trip_with_storage_options(moto_endpoint: str, s3_client) -> No assert ctx.entries() == 2 +@_S3_MEMWAL_XFAIL def test_s3_deprecated_aws_kwargs_still_work(moto_endpoint: str, s3_client) -> None: """AWS kwargs keep working (back-compat) and emit a DeprecationWarning.""" bucket = f"context-{uuid.uuid4().hex}" From fd3d8c4c9c57e65b12648c551ae40877b38c14d3 Mon Sep 17 00:00:00 2001 From: Beinan Date: Sat, 25 Jul 2026 09:10:46 +0000 Subject: [PATCH 3/3] fix(ci): run doctests by module import, not file path The previous commit turned the doctest step from a permanent no-op into a real check, which immediately exposed that the invocation itself was wrong: ImportError: attempted relative import with no known parent package `python -m doctest ` imports the target by filename, so the package-relative `from .api import ...` in `__init__.py` has no parent package and fails before any doctest runs. This is why the original step could only ever "pass" while wrapped in `|| echo "No doctests found"`. Import the installed module instead and propagate the result, so the step fails on a failing doctest rather than on its own invocation. Note the module currently reports `attempted=0` -- there are genuinely no doctests today. The step is now wired correctly so that any doctest added later is actually executed and enforced. The pytest step in the same job passed: 190 passed, 6 skipped, 3 xfailed. Co-Authored-By: Claude --- .github/workflows/python-test.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/python-test.yml b/.github/workflows/python-test.yml index 2e776027..a8d8052b 100644 --- a/.github/workflows/python-test.yml +++ b/.github/workflows/python-test.yml @@ -162,7 +162,14 @@ jobs: working-directory: python run: | source .venv/bin/activate - python -m doctest python/lance_context/__init__.py + # Import as a module, not a file path: `python -m doctest ` + # imports by filename, which breaks the package-relative imports in + # __init__.py. Runs against the installed wheel and fails on any + # failing doctest. + python -c "import doctest, sys, lance_context; \ + r = doctest.testmod(lance_context); \ + print(r); \ + sys.exit(1 if r.failed else 0)" lint: runs-on: ubuntu-24.04