Skip to content

fix(pipeline): make run_sift.py invocable by path; skip infra files in entity resolution - #40

Open
chrisdavidson wants to merge 3 commits into
usathyan:mainfrom
chrisdavidson:fix/pipeline-invocation-ergonomics
Open

fix(pipeline): make run_sift.py invocable by path; skip infra files in entity resolution#40
chrisdavidson wants to merge 3 commits into
usathyan:mainfrom
chrisdavidson:fix/pipeline-invocation-ergonomics

Conversation

@chrisdavidson

Copy link
Copy Markdown
Collaborator

Two defects found while running /epistract:ingest against an FDA product label corpus (Ozempic MAR2022 SPL). Both are invocation/hygiene bugs rather than extraction bugs.

1. run_sift.py cannot be invoked by path

commands/ingest.md documents invoking this script by absolute path. That fails:

$ python3 core/run_sift.py build ./out --domain domains/drug-discovery/domain.yaml
ModuleNotFoundError: No module named 'core'

run_sift.py:21 imports core.domain_resolver at module scope, but running a script by path puts core/ on sys.path[0] — not the project root. The file already inserts the project root, at line 240 inside cmd_dashboard() for examples.workbench.* imports; that is far too late for an import that runs at module load. Until now it only worked with an explicit PYTHONPATH=<repo root>.

Fixed by hoisting the bootstrap above the import, using the idiom already established in core/build_extraction.py:18-22 and core/normalize_extractions.py:43-45.

2. entity_resolution.py parses its own infrastructure files

preprocess_extractions() globs extractions/*.json unfiltered, so it also reads _normalization_report.json. The report has no entities key, so nothing is mutated — but it is parsed on every run and inflates the returned files_processed count by one per infrastructure file.

core/normalize_extractions.py:63,167-170 already solved this for the same directory with _SKIP_PREFIXES = ("_",). entity_resolution.py simply did not follow the convention. Applied the same guard.

Related, and deliberately not fixed here

Every graph build logs:

Failed to load _normalization_report.json: 2 validation errors for DocumentExtraction
  document_id   Field required
  document_path Field required

This originates upstream, not in epistractsift_kg/graph/builder.py:308-313, where load_extractions() globs *.json and calls DocumentExtraction(**raw) on every match, warning on failure. It cannot be patched from this repo.

The only epistract-side lever would be to stop writing the report into extractions/, but that path is contractual — documented twice in commands/ingest.md, asserted by UT-023 (tests/test_unit.py) and tests/test_e2e.py (x2), and specified in tests/TEST_REQUIREMENTS.md. That is a breaking change and out of scope here.

Change 2 therefore does not silence that warning. Suggested follow-up: an upstream issue against sift-kg to apply the same underscore-skip convention in load_extractions().

Tests

Two regression tests in tests/test_unit.py, both verified failing before the fixes and passing after:

  • test_run_sift_importable_by_path_without_pythonpath — subprocess invocation by absolute path with PYTHONPATH scrubbed and cwd outside the repo, so a stray '' on sys.path cannot mask the regression.
  • test_preprocess_extractions_skips_underscore_infra_files — one real extraction plus a report; asserts only the former is counted and the report is left byte-identical.

Verification

Check Result
New tests with fixes reverted both FAIL
New tests with fixes applied both PASS
Full unit suite 184 passed, 1 failed
Ruff delta on touched files 25 -> 25 (net zero)

The one failure, tests/test_kg_provenance.py::TestPCCVenueProvenance::test_akka_party_referenced_in_response, fails identically on origin/main — pre-existing and unrelated to this PR. For what it is worth, the assertion looks self-contradictory: it requires the string "akka" while its failure message reads "Response does not mention STA", and the fixture response discusses STA's obligations without ever containing "akka". Worth a separate look.

Test plan

  • pytest tests/ -m unit — 184 passed (2 new), 1 pre-existing unrelated failure
  • Regression tests confirmed failing without the fixes
  • ruff check — no new findings introduced
  • python3 core/run_sift.py --list-domains from a cwd outside the repo with no PYTHONPATH
  • Reviewer: confirm the sift-kg upstream issue is worth filing

run_sift.py:21 imports core.domain_resolver at module scope. Running a
script by path puts core/ on sys.path[0], not the project root, so the
documented invocation in commands/ingest.md failed with:

    ModuleNotFoundError: No module named 'core'

The file already inserted the project root, but at line 240 inside
cmd_dashboard() for examples.workbench.* imports — far too late for a
module-level import. It only worked with an explicit PYTHONPATH.

Hoist the bootstrap above the import, matching the idiom already used in
core/build_extraction.py and core/normalize_extractions.py.
preprocess_extractions() globbed extractions/*.json unfiltered, so it also
read _normalization_report.json. The report has no entities key so nothing
was mutated, but it was parsed on every run and inflated files_processed
by one per infrastructure file.

core/normalize_extractions.py already solved this for the same directory
with _SKIP_PREFIXES = ("_",); entity_resolution.py just did not follow the
convention. Apply the same guard.
Both tests fail without the accompanying fixes and pass with them.

- run_sift.py is invoked by absolute path in a subprocess with PYTHONPATH
  scrubbed and cwd outside the repo, so a stray '' on sys.path cannot mask
  the regression.
- preprocess_extractions() is given one real extraction plus a
  _normalization_report.json and must count only the former, leaving the
  report byte-identical.

@usathyan usathyan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix merge conflicts and merge away!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants