Architecture review: fix critical bugs, add observability, modernize dedup#12
Merged
Conversation
1. Fix reflection loop JSON preservation: iterative_improve() now uses
coerce_to_json_text() to serialize the full Pydantic model (tags,
confidence, sources) instead of extracting only the .text field,
which broke the reflection loop by losing the JSON envelope.
2. Fix event prompt/schema drift: Guantanamo events prompt now matches
the actual Event model fields (event_type, start_date, description,
end_date, is_fuzzy_date, tags) instead of outdated field names
(type, date) that caused parse failures.
3. Fix local mode extraction: extract_local() now uses List[Entity]
(same as cloud) instead of container models (ArticlePeople, etc.)
that expected {"people": [...]} while prompts output bare arrays.
This was causing near-zero recall for all entity types in local mode.
4. Fix hardcoded OLLAMA_API_URL: Now reads from environment variable
with localhost default, instead of hardcoded LAN IP.
Bonus: extraction exceptions are now logged instead of silently swallowed.
…se 2) New modules: - src/utils/outcomes.py: PhaseOutcome datatype that carries success/failure, error payload, QC flags, and counts alongside a usable fallback value. Replaces silent return-empty-list patterns with structured outcomes. - src/utils/quality_controls.py: Deterministic QC for extraction (required- field validation, name normalization, within-article dedup, suspicious- result flagging) and profiles (min text length, citation checks, tag count, confidence range). QC never raises — it drops/fixes and reports. Integration points: - article_processor.py: check_relevance() and extract_single_entity_type() now return PhaseOutcome objects. Extraction QC runs automatically after each entity type extraction. Outcomes stored in processing_metadata. - profiles.py: Added create_profile_with_outcome() and update_profile_with_outcome() wrappers that never raise, run profile QC, and return PhaseOutcome alongside the existing return tuple. - process_and_extract.py: Relevance outcomes surfaced in phase_outcomes. Processing metadata persisted back to article rows. - constants.py: Added QC threshold constants (min name length, min text length, min tag count). All existing APIs preserved — new outcome functions are additive wrappers.
…rs optional Make the project installable and usable on Intel Mac (where PyTorch has no wheels) by making sentence-transformers an optional dependency and adding robust embedding backend selection. Tier 1 — Unblock Intel Mac: - Move sentence-transformers from base deps to [local-embeddings] extra - Add graceful ImportError in LocalEmbeddingProvider with actionable guidance - Change template default embedding mode from 'local' to 'cloud' Tier 2 — Robust portability: - Add EmbeddingMode.AUTO that auto-detects available backends via find_spec - Add explicit device config (auto/cpu/cuda/mps) for local embeddings - Persist embedding model name + dimension alongside profile_embedding - Add compatibility checking in find_similar_entity to prevent silent dedup failures from dimension/model mismatches across environments Also fixes: - Embedding manager cache key now includes mode (was domain-only) - Mutable default dict in EmbeddingConfig/Result replaced with Field(default_factory) - Cloud provider returns stable configured model name (not response model) - DomainConfig defaults aligned to 'cloud' for platform safety
… fingerprints (Phase 4) - Add RapidFuzz lexical blocking in find_similar_entity() to pre-filter candidates before cosine similarity (O(n) → O(k)) - Replace single SIMILARITY_THRESHOLD with per-entity-type thresholds loaded from domain config (dedup.similarity_thresholds section) - Store profile_embedding_fingerprint on all entities for model change detection - Add DomainConfig.get_similarity_threshold(entity_type) with 4-level fallback - Add DomainConfig.get_lexical_blocking_config() accessor - Add EmbeddingManager.fingerprint_from_result() and make_fingerprint() helpers - 15 new tests covering lexical blocking, threshold resolution, and fingerprints
Runs on push to main and on PRs. Two jobs: - lint: ruff check + format verification - test: pytest with async tests excluded (pre-existing config issue)
- Document quality controls (PhaseOutcome, extraction QC, profile QC) - Document dedup modernization (lexical blocking, per-type thresholds, fingerprints) - Add CI workflow mention to testing sections - Update project structure with new utility modules - Update "Built with" to include RapidFuzz
ruff is in [project.optional-dependencies] dev (--extra dev), while pytest-asyncio etc. are in [dependency-groups] dev (--group dev). CI needs both.
- Add E501 ignore (ruff format handles line length) - Add per-file-ignores for FastHTML star imports (F403/F405) and config_loader lru_cache on methods (B019) - Fix bare excepts → except Exception (E722) - Fix ambiguous variable name l → loc (E741) - Fix type comparison == → is (E721) - Fix lambda loop variable binding (B023) - Remove unused test variables (F841) - Remove f-strings without placeholders (F541) - Update lint.sh to run full ruff check (matching CI)
…d quality Must-fix items: - Change embedding default from hybrid to cloud (prevents crash when local-embeddings extra not installed) - Make QC required fields schema-driven from Pydantic models instead of hardcoded (supports multi-domain without breakage) - Fix normalize_name: strip/collapse whitespace + Unicode NFC only, drop .title() which corrupted acronyms (FBI→Fbi, McDonald→Mcdonald) - Fix CI test filtering: -m "not asyncio" (marker) instead of -k (substring) - Guard against KeyError on older entities missing "articles" key Suggestions: - asyncio.get_event_loop() → asyncio.get_running_loop() (deprecation) - Shallow copy → deepcopy for nested config defaults - Fix misleading log: show actual embedding mode/model, not LLM mode - PhaseOutcome.to_metadata_dict() uses exclude_none=True - Update smoke test stub with mode/get_active_model_name attrs
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
Implements phases 1, 2, and 4 from the architecture review, plus cross-platform compatibility, CI/lint infrastructure, and code review fixes.
Phase 1: Fix critical bugs (
8af0664)iterative_improve()now preserves the full JSON envelope instead of losing it after the first iterationevents.mdprompt to match actual Pydantic schema fieldsPhase 2: Add observability (
acdebd2)[]returns, carrying error context through the pipelineCross-platform compatibility (
68de0d0)EmbeddingManagerstores model name + dimension on entities for tracking_embeddings_compatible()skips incompatible embeddings during similarity searchPhase 4: Dedup modernization (
c6d528a)WRatiopre-filters candidates before cosine similarity (O(n) → O(k))dedup.similarity_thresholds)"{model}:{dim}"fingerprint stored on all entities for model change detectionCI & Lint (
1489be2,ad9e2b0,8c4f44b)ruff check+ruff format --check) and testslint.shwith CI — both now run full ruff rulesetCode review fixes (
b2ef469)hybridtocloud(prevents crash when local-embeddings not installed)normalize_name(): strip/collapse whitespace + Unicode NFC only (no more.title()corrupting acronyms like FBI→Fbi)-m "not asyncio"(marker) instead of-k(substring)articleskeyasyncio.get_event_loop()→asyncio.get_running_loop()(deprecation fix)PhaseOutcome.to_metadata_dict()usesexclude_none=Trueto reduce storage noiseTest plan
pytest -m "not asyncio")ruff format) and lint (ruff check) clean