rust stage 3 (a+b): diff_compressor port + retire python via pyo3 + dotenv test hygiene#278
Closed
chopratejas wants to merge 2 commits intorust-stage-3a-diff-compressorfrom
Closed
rust stage 3 (a+b): diff_compressor port + retire python via pyo3 + dotenv test hygiene#278chopratejas wants to merge 2 commits intorust-stage-3a-diff-compressorfrom
chopratejas wants to merge 2 commits intorust-stage-3a-diff-compressorfrom
Conversation
chopratejas
force-pushed
the
rust-stage-3a-diff-compressor
branch
from
dba733d to
48c1324
Compare
The python `DiffCompressor` is replaced by a thin pyo3-backed shim that delegates to `headroom._core.DiffCompressor`. There is no python implementation and no env-var fallback — the wheel is a hard import. Why now: opt-in defaults don't drive python retirement. Byte-equal parity was already proven across 27 fixtures (stage 3a); keeping a shadow python impl behind a flag is a permanent maintenance cost with no operational benefit. Stage 3b deletes ~700 lines of python parser / scorer / formatter code; the rust crate has its own coverage. Surface preserved: - `headroom.transforms.diff_compressor.DiffCompressor` — same class name, same `__init__`, same `compress(content, context)` shape. Returns python `DiffCompressionResult` dataclasses so call sites that destructure with `asdict()` work unchanged. - `DiffCompressorConfig` and `DiffCompressionResult` dataclasses kept. - Sidecar `compress_with_stats(...)` exposes the rust-only `DiffCompressorStats` (per-file hunk drops, context lines trimmed, file_mode normalizations) for observability. Removed: - Python parser / scorer / formatter (~700 lines). - Internal `DiffHunk` / `DiffFile` parser dataclasses (rust crate has parallel coverage). - 12 tests that probed `_parse_diff` / `_score_hunks` or the deleted parser dataclasses. The 29 public-API tests in `test_diff_compressor.py` remain and now exercise the rust backend through the same import path. - `HEADROOM_DIFF_COMPRESSOR_BACKEND` env var — no longer meaningful. Build: - `scripts/build_rust_extension.sh` runs `maturin develop` and symlinks the built `.so` into `headroom/` so `import headroom._core` resolves past the in-tree package shadowing the maturin overlay. - `.gitignore` excludes the symlinks and allowlists the build script. Tests: - 544 transforms pass; 33 rust-vs-fixture parity tests guard the pyo3 bridge against regressions (`tests/test_transforms/test_diff_compressor_rust_parity.py`). - Mypy clean.
…n during pytest collection
# The bug
Several test modules and two production modules loaded the project `.env`
at *import time*. During pytest collection (where every test module is
imported once), this populated `os.environ` with API keys from `.env`.
The skipif guards in `test_proxy_passthrough_integration.py` (and
others) evaluate at collection time:
@pytest.mark.skipif(not os.environ.get("OPENAI_API_KEY"), reason="...")
If the polluter module was collected *before* the guard, the guard saw
the leaked key, decided not to skip, and the integration tests ran
live against a fake key and failed. In a fresh local-dev venv with
`.env` + full `[dev]` extras, this manifested as ~16 spurious test
failures plus a misleading test runtime of 6+ minutes (live HTTP).
# Why now
CI does not see this (no `.env`). It only manifests when:
1. `litellm` (and friends) are installed — they run `dotenv.load_dotenv()`
on import, populating `os.environ` from `.env`.
2. A `.env` file with real API keys exists locally.
Until the venv was provisioned with the full `[dev]` extras during
recent test work, `pytest.importorskip("litellm")` and
`from headroom.pricing import litellm_pricing` both silently no-op'd
(via try/except ImportError → `LITELLM_AVAILABLE=False`), so the leak
never triggered. With litellm now installed, the latent bug surfaced.
# The fix — three patterns
1. **Production modules** (`headroom/pricing/litellm_pricing.py`,
`headroom/backends/litellm.py`): wrap the eager `import litellm` with
a snapshot/restore of `os.environ`. Any keys litellm's bundled
`python-dotenv` adds during import are deleted immediately. The
module is fully imported and cached in `sys.modules` so subsequent
imports hit the cache without re-running the side effect.
2. **Test modules using `pytest.importorskip("litellm")`**
(`test_backend_bugs.py`, `test_bedrock_region.py`,
`test_cost_tracker_counterfactual.py`): replace with
`tests._dotenv.importorskip_no_env_leak("litellm")`, which does the
same snapshot/restore around `importlib.import_module`.
3. **Test modules that intentionally need `.env` values for skipif
guards** (`test_compression_summary_*.py`, `test_query_echo.py`,
`test_cost_tracker_counterfactual.py`, `test_memory_usage_integration.py`,
`test_bundled_tools_savings.py`): replace module-level
`os.environ.setdefault(...)` / `dotenv.load_dotenv()` with
`tests._dotenv.load_env_overrides()` (returns a local dict — does
NOT mutate `os.environ`) plus `autouse_apply_env(...)` (function-
scoped fixture that applies via `monkeypatch.setenv`, auto-cleaned
at teardown). The skipif still works because
`ANTHROPIC_KEY = os.environ.get(...) or _env_overrides.get(...)`
reads from the local dict as fallback.
# Helper module
New `tests/_dotenv.py` exposes:
- `load_env_overrides() -> dict[str, str]` — read `.env` into a dict.
- `autouse_apply_env(overrides) -> fixture` — function-scoped autouse
fixture that applies via `monkeypatch.setenv`.
- `importorskip_no_env_leak(module) -> module` — drop-in
`pytest.importorskip` substitute that quarantines env mutations.
# Results
Local full-suite (excluding live-LLM and live-feed tests):
- Before: 46 failed, 4830 passed, 387s
- After: 2 failed, 4672 passed, 134s
The remaining 2 failures are unrelated environment-dependent tests
(missing `PIL` / Docker daemon).
chopratejas
force-pushed
the
rust-stage-3b-diff-bridge
branch
from
d2b5b4b to
d5ca50c
Compare
chopratejas
changed the title
5 tasks
Owner
Author
|
Superseded by #277 (3a+3b combined into one PR going to main). |
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
1 participant
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
Combines stages 3a + 3b into one PR going to
main. The original stack (#276 + #278) is collapsed because the Python deletion in 3b makes 3a's intermediate state (Python-and-Rust coexisting) less interesting to land on its own.What's in here (6 commits)
Stage 3a — port
DiffCompressorto Rust:feat(rust): diff_compressor port — byte-equal parity + sidecar stats— full port tocrates/headroom-core/src/diff_compressor.rs. 27 fixtures, 27 byte-equal matches incargo run -p headroom-parity run.refactor(rust): diff_compressor — surface hidden cutoffs + lossy-emit stats— cutoff signals (per-file hunk drops, context lines trimmed, file_mode normalizations) exposed as aDiffCompressorStatssidecar struct so observability isn't dropped on the floor.fix(diff_compressor): four silent information-loss paths in Python AND Rust— bugs in BOTH impls: rename/dissimilarity markers discarded, combined-diff hunks (merge commits) parsed-then-dropped, no-newline markers lost across distance, pre-diff content order mangled. All four fixed in both the Python and Rust impls.fix(diff): close ContentRouter routing gaps for merge diffs and long preambles—ContentRouterwas failing to route real merge-commit diffs (diff --combined/diff --cc) and long-preamble diffs toDiffCompressor. Detection scan widened; combined-diff headers added to the regex tier.Stage 3b — retire the Python implementation:
feat(rust): retire python diff_compressor, ship rust-only via pyo3— the PythonDiffCompressoris replaced by a thin pyo3-backed shim that delegates toheadroom._core.DiffCompressor(built fromcrates/headroom-py). No env-var fallback. No Python implementation. The wheel is a hard import. Deletes ~700 lines of Python parser/scorer/formatter code.fix(tests): stop module-level dotenv loaders from polluting os.environ during pytest collection— pre-existing latent bug surfaced by the new[dev]extras provisioning.litellm(anddotenv.load_dotenv()) at module-level loaded.envintoos.environduring pytest collection, breaking@pytest.mark.skipif(not os.environ.get("..."))guards in unrelated integration tests. Local full-suite went from 46 fails / 387s to 2 fails / 134s.Surface preserved
headroom.transforms.diff_compressor.DiffCompressor— same class name, same__init__, samecompress(content, context)shape. Returns PythonDiffCompressionResultdataclass instances.DiffCompressorConfigandDiffCompressionResultdataclasses kept.compress_with_stats(...)exposes the rust-onlyDiffCompressorStats.What's removed
DiffHunk/DiffFileparser dataclasses (rust crate has parallel coverage).test_diff_compressor.pythat probed_parse_diff/_score_hunksor the deleted parser dataclasses. The remaining 29 public-API tests now run against the rust backend through the same import path — kept unchanged.HEADROOM_DIFF_COMPRESSOR_BACKENDenv var.Build
`scripts/build_rust_extension.sh` runs `maturin develop` and symlinks the built `.so` into `headroom/` so `import headroom._core` resolves past the in-tree package shadowing the maturin overlay.
CI note
CI does not yet build the wheel. The 25 failing tests in `test_diff_compressor.py` on this PR are because `headroom._core` isn't importable in CI yet. Adding a CI step for `maturin develop` (or shipping a prebuilt wheel) is required before this can merge — that work is out of scope for this PR but should land alongside.
Test plan
🤖 Generated with Claude Code