fix(lib): isolate build-mode store caches and tolerate non-numeric cells in release filters - #140
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SkyeAv
added a commit
that referenced
this pull request
Sep 4, 2026
…lls in release filters Two release-mode bugs shipped applied_to_treat edges with number_of_cases < 25 in --release KGs: 1. The section-parquet cache key ignored build mode. Stage 3 named each section's cached subgraph <mkhash(s)>.parquet with release injected outside the hashed dict, and Tcode.collect quick-exits on any existing store file -- so a parquet cached by a non-release (or pre-16.6.0, filter-no-op) build was reused verbatim in --release builds, skipping the release filters. --head already dodged this with .head.parquet; _section_store_path generalizes that: each of head/release/qc gets its own suffix, composing when combined. qc belongs because fullmap_audit drops rejects from the cached parquet too. 2. drop_low_number_of_cases and drop_zero_effect_size cast with strict Float64, but the csv op reads sources with has_header=False, so a TSV header row flows through as data and the strict cast raised InvalidOperationError on the non-numeric header cell. Both ops now cast with strict=False (the clean_numeric idiom); non-numeric cells become null and are kept, dying later at entity resolution as before. Tests: non-numeric/null tolerance for both drop ops, per-mode store-path suffixes, and a quick-exit regression proving a non-release parquet is never served to a release build.
SkyeAv
force-pushed
the
fix/release-mode-store-cache
branch
from
September 4, 2026 20:37
fd9c24b to
ea5a0b4
Compare
SkyeAv
added a commit
that referenced
this pull request
Sep 4, 2026
Cut 16.6.2 and bump the package version in pyproject.toml, uv.lock, and CITATION.cff. Patch: two fix PRs ship. `number_of_cases` is now emitted as a JSON integer instead of raw source text — the slot is typed `int` on biolink-model but sat outside `lib.numeric_columns`, so string cells shipped on the edge NDJSON and left the graph type-inconsistent edge to edge (#139). `--release` builds no longer reuse section parquets cached by other build modes: `--head`, `--release`, and `--qc` now cache under distinct suffixes via `_section_store_path`, closing a cache-poisoning bug where release filters were silently skipped and `applied_to_treat` edges with `number_of_cases < 25` shipped (#140). The release-mode `drop_low_number_of_cases` and `drop_zero_effect_size` filters also tolerate non-numeric cells (e.g. a TSV header row read as data) instead of crashing with `InvalidOperationError` (#140). Changelog: - Versioned the Unreleased section as 16.6.2 and gave the `number_of_cases` entry its missing PR link (#139). Docs: none needed here — #140's `cli.py` change is an internal cache-key helper and #139 changes emission types only; both are fully documented in their changelog entries. Testing: - uv run pytest -q -> 1253 passed, 3 skipped (96% coverage) - uv run ruff check . && uv run ruff format --check . && uv run pyright -> clean / 0 errors - uv lock --check -> up to date - uv run mkdocs build --strict -> clean
SkyeAv
added a commit
to glusman-team/dakp
that referenced
this pull request
Sep 4, 2026
Picks up the 16.6.2 fix for --release section-parquet cache poisoning (SkyeAv/Tablassert#140): the cache key previously ignored the build mode, so --release builds could reuse parquets cached by --head/--qc/pre-16.6.0 builds and ship applied_to_treat edges with number_of_cases < 25. --qc gets its own suffix for the same reason (fullmap_audit drops rejects from the cached parquet). Floor bumped 16.6.0 -> 16.6.2 and uv.lock re-locked. Verified: uv run pyright (0 errors), uv run pytest (1058 passed, 100.00% coverage), uvx ruff check (clean).
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
tablassert build-kg --releaseshippedapplied_to_treatedges withnumber_of_cases < 25even though the release-mode case-count filter exists. Reproduced against installed 16.6.0: a 1-case edge shipped in a--releaseKG. Two independent bugs, both fixed here.Bug 1 — section-parquet cache key ignores build mode
In
build_graph_pipeline(stage 3), each section's cached subgraph parquet was namedSTORE / f"{mkhash(s)}.parquet"wheresis only the section config dict;releasewas injected intoTcode.model_validate({**s, ..., "release": release})outside the hashed dict.Tcode.collect()then quick-exits on any existing store file (if self.store.is_file(): return self.store), skipping the entire op chain — including the release filters wired in_source_ops. So a parquet cached by a non-release build (or a pre-16.6.0 build, where the filter silently no-op'd per #137) was reused verbatim in--releasebuilds: cache poisoning shipped the 1-case edge.--headalready dodged this class of bug with a distinct.head.parquetname. The fix generalizes that idiom: a small_section_store_path(h, head, release, qc)helper gives each build-mode flag its own suffix (<h>.parquet,<h>.head.parquet,<h>.release.parquet,<h>.qc.parquet), composing when flags combine (<h>.head.release.qc.parquet).--qcis included in the fix, not deferred:fullmap_auditdrops QC rejects from the output LazyFrame (qc.py:138-139), so a--qcbuild's cached parquet genuinely differs in content from a non-qc one — the same cache-poisoning class, fixed for free by the same helper.Bug 2 — strict cast crashes on header rows
drop_low_number_of_casesanddrop_zero_effect_sizefiltered withpl.col(col).cast(pl.Float64)(strict). Thecsvop reads sources withhas_header=False, so a TSV's header row flows through as a data row and the strict cast raisedpolars.exceptions.InvalidOperationErroron the non-numeric header cell (reproduced:conversion from str to f64 failed in column 'number_of_cases' ... ["case_count"]).Both ops now cast with
strict=False— theclean_numericidiom. Non-numeric cells become null and are kept (ne_missing(0.0)for effect size;.ge(threshold).fill_null(True)for case counts, since polars 1.40 has noge_missing): header rows die later at entity resolution as they did before the filters existed. This tolerates arbitrary non-numeric cells, including the pending downstream TSV rename of thecase_countheader tonumber_of_cases.drop_not_significantcasts to String and was never affected.Test coverage
test_drop_low_number_of_cases_keeps_non_numeric_cells— header-cell row and nulls kept, "10" dropped, "30" kept.test_drop_zero_effect_size_keeps_non_numeric_cells— same tolerance.test_section_store_path_suffixes_every_build_mode— every flag combination gets a distinct, composing cache name.test_tcode_collect_quick_exit_never_serves_another_mode_cached_parquet— a non-release cached parquet never quick-exits a release build; once the release parquet exists, the quick exit serves it.uv run pytest tests/test_lib.py -k "release or number_of_cases or effect_size or store or not_significant or head_mode"→ 32 passed;tests/test_cover_cli.py→ 52 passed; ruff check/format and pyright clean.