feat(yandex-valuation): link history rows to houses via match_or_create_house #531

Merged

lekss361 merged 1 commit from feat/tradein-yandex-valuation-house-link into main

2026-05-24 14:28:31 +00:00

lekss361 commented

2026-05-24 14:18:32 +00:00

Owner

Summary

_save_yandex_history_items resolves house_id ONCE per page using existing match_or_create_house (3-tier: cadastral, fingerprint, geo-proximity, new)
Persists house_id, source_confidence, removed_date, total_floors, notes columns of house_placement_history
Graceful fallback to house_id=NULL + source_confidence=0 when match fails

Why

Prior to this PR, 160 rows in house_placement_history with source='yandex_valuation' had ALL of house_id, source_confidence, removed_date, total_floors as NULL — comment in code admitted "estimator doesn't compute target_house_id yet". This PR wires the existing match_or_create_house() infra (was unused for yandex_valuation) and adds the columns now provided by the PR #526 parser.

Tests

17/17 pytest pass (6 new in test_yandex_valuation_save.py + 11 existing test_estimator_yandex_integration.py)
ruff clean
New tests cover: once-per-page house resolution, row contents propagation, removed_date persistence, active-listing null removed_date, match failure graceful fallback, kwargs forwarding

Followup

PR #3 — bulk-runner CLI sweep for all EKB yandex addresses (re-runs to backfill the 160 orphan rows)

## Summary - `_save_yandex_history_items` resolves `house_id` ONCE per page using existing `match_or_create_house` (3-tier: cadastral, fingerprint, geo-proximity, new) - Persists `house_id`, `source_confidence`, `removed_date`, `total_floors`, `notes` columns of `house_placement_history` - Graceful fallback to `house_id=NULL` + `source_confidence=0` when match fails ## Why Prior to this PR, 160 rows in `house_placement_history` with `source='yandex_valuation'` had ALL of `house_id`, `source_confidence`, `removed_date`, `total_floors` as NULL — comment in code admitted "estimator doesn't compute target_house_id yet". This PR wires the existing `match_or_create_house()` infra (was unused for yandex_valuation) and adds the columns now provided by the PR #526 parser. ## Tests - 17/17 pytest pass (6 new in `test_yandex_valuation_save.py` + 11 existing `test_estimator_yandex_integration.py`) - ruff clean - New tests cover: once-per-page house resolution, row contents propagation, removed_date persistence, active-listing null removed_date, match failure graceful fallback, kwargs forwarding ## Followup - PR #3 — bulk-runner CLI sweep for all EKB yandex addresses (re-runs to backfill the 160 orphan rows)

lekss361 added 1 commit 2026-05-24 14:18:32 +00:00

feat(yandex-valuation): link history rows to houses via match_or_create_house f04e391bf6

- _save_yandex_history_items now resolves house_id ONCE per page using
  match_or_create_house (3-tier: cadastral, fingerprint, geo-proximity, new)
- Persists house_id, source_confidence, removed_date, total_floors, notes
- Graceful fallback to house_id=NULL when match fails
- 160 orphan rows on prod will get linked on next run

Builds on PR #526 (parser fixes: removed_date, total_floors, NBSP).

lekss361 commented

2026-05-24 14:28:18 +00:00

Author

Owner

Deep Code Review — PR #531 verdict

Summary

Status: APPROVE (squash-merge ready)
Files: 3 (1 P0 service, 1 changed test, 1 new test)
Lines: +497 / -155 (most is reformatting from a ruff format pass)
Head SHA verified: f04e391 matches pulls/531.head.sha
Net behaviour change: ~120 lines (rest is whitespace/wrap reformat)

Scope

Wires the existing 3-tier match_or_create_house() (cadastr → source_exact → fingerprint → geo-proximity → new) into _save_yandex_history_items and persists the 5 columns that PR #526 made available in the parser but _save_yandex_history_items was silently dropping: house_id, source_confidence, total_floors, removed_date, notes. Closes the persistence gap flagged in #526 review.

Also reformatted call-sites of db.execute(text(...)) in the unchanged _fetch_analogs / _fetch_deals / _get_or_fetch_imv_cached blocks; no behaviour delta there.

Verified correctness (the boring stuff that matters)

Idempotency: UNIQUE (source, ext_item_id) (017_house_placement_history.sql:50) + ON CONFLICT (source, ext_item_id) DO NOTHING preserved. Synthetic ext_item_id = sha256(address|publish_date|area|floor|prices)[:32] is deterministic — re-runs no-op. ext_id for house resolution = sha256(strip+lower(address))[:16] — also stable.
Schema match: All bound columns exist — house_id (017:22), total_floors (017:32), removed_date (017:41), source_confidence (045:11), notes (045:12). No migration needed.
psycopg v3: CAST(:raw AS jsonb) correct. house_id/total_floors/removed_date bind None → NULL for INSERT VALUES (no CAST needed, that trap is for IS NOT NULL predicates only).
Once-per-page: confirmed by test test_save_resolves_house_once_per_page (m.call_count == 1, saved == 2). Correct — avoids hammering pg_advisory_xact_lock(42, hashtext(fp)) N times per page.
Graceful failure path: match_or_create_house raises → db.rollback() + house_id=None, confidence=0.0, method='fail', history INSERT then proceeds standalone. notes gets None (not match_method=fail) per line 524. Test test_save_handles_match_failure_gracefully asserts row dict shape correctly.
Bootstrap context: PR #527 already populated 6,507 canonical houses in houses with tradein_normalize_short_addr. PR #531's calls into match_or_create_house (via address_fingerprint → normalize_address) hit a populated house_address_aliases table → most yandex pages will resolve via Tier 2 (fingerprint, conf=0.9) immediately. Step #5 of migration 063 left 160 yandex rows with house_id=NULL because raw_payload had no address — bulk re-run (followup PR #3 per description) backfills them through this new path.
Dependency contract verified: opened matching/houses.py at head_sha (per memory rule feedback_verify_dependency_contract_in_review) — match_or_create_house does raise on real DB errors (db.execute(...).one() will raise NoResultFound/IntegrityError). It is NOT a silent-swallow-and-return-default; the PR's try/except is needed and correct.

Findings

Cosmetic (non-blocking)

estimator.py:793-794 — stale log: "(house_id=NULL — matching deferred)" is now wrong on every successful match. INFO-level prod log scrape will show misleading message. Suggested next-PR fix:
```
# estimator.py around line 792
logger.info(
    "yandex_valuation: history items processed=%d saved=%d",
    len(yandex_val.history_items), saved_hist,
)
```

Quality enhancement (next iteration)

_save_yandex_history_items calls match_or_create_house with no lat/lon. geo.lat/geo.lon are available at the call site in estimate_quality (Stage 8 runs after geocoding succeeded). Without coords, Tier 3 geo-proximity (30 m PostGIS) is skipped, and the new-house INSERT lands with lat=NULL, lon=NULL (so the houses_set_geom_trg trigger leaves geom NULL too). Recommend threading geo into the call:
```
house_id, mc, m = match_or_create_house(
    db, ext_source="yandex_valuation", ext_id=house_ext_id,
    address=result.address, lat=geo.lat, lon=geo.lon,  # NEW
    year_built=result.house.year_built,
)
```
Requires passing geo through _save_yandex_history_items signature. Not a blocker for this PR — the bootstrap from #527 covers most EKB houses via fingerprint Tier 2, but Tier 3 is the safety net for off-by-one address normalizations.

Worth tracking

db.rollback() inside the inner try/except only clobbers uncommitted state in the request-scoped Session. Upstream _get_or_fetch_yandex_valuation_cached does its own db.commit() on line 433 before returning, so the external_valuations cache write is durable. IMV cache writes via save_imv_evaluation happen even further upstream — also safe assuming they commit (would need a separate verification pass on save_imv_evaluation if that flow is ever changed). For this PR's scope: tx boundaries are fine.
Confidence is currently capped at 0.9 (fingerprint) for known houses. When the addresses literally match houses.address via cadastral_number (Tier 0) we'd get 1.0, but Yandex pages don't carry cadastr → Tier 0 always skipped. Acceptable.

Tests

6 new tests (test_yandex_valuation_save.py) cover: once-per-page resolution, row dict house_id/confidence/total_floors/notes propagation, removed_date populated for sold listing, removed_date None for active listing, graceful match failure (saved=N, house_id=None, notes=None, confidence=0.0), kwargs forwarding (address/year_built/ext_source).
4 existing tests (test_estimator_yandex_integration.py) updated with patch("app.services.estimator.match_or_create_house") to keep them deterministic. No integration coverage was lost.
Author claims 17/17 pass + ruff clean. The mock patterns and assertions look correct on inspection.

Security

No credential / token / API-key literals in diff. No SQL injection vectors (all named params). No git --no-verify traces in recent log.

Vault cross-check

Cross_Source_Matching_Strategy.md (referenced in matching/houses.py docstring) describes the 3-tier algorithm and source_confidence semantics from sec 14.1 — PR honors that contract (1.0 cadastr/new, 0.9 fp, 0.7 geo, 0.0 fail). 045_house_placement_history_extend.sql:18-22 also documents these confidence values.
Schema_Avito_Houses_Listings_History.md sec 6: house_id is FK, NULL allowed for orphan-rows, comment-documented as "Backfill через UPDATE … при первом scrape дома" — this PR is the realtime-scrape version of that backfill, complementary not contradictory.

Verdict reasoning

This is a focused, well-tested change that closes a concrete persistence gap. Code reuses existing infra (match_or_create_house) rather than reinventing matching logic, transaction boundaries are sound, all schema columns exist, idempotency is preserved, psycopg v3 conventions followed. The two findings (stale log, missing lat/lon) are low-priority and orthogonal — fine as separate followups.

APPROVE — squash-merge + delete branch.

## Deep Code Review — PR #531 verdict ### Summary - **Status**: APPROVE (squash-merge ready) - **Files**: 3 (1 P0 service, 1 changed test, 1 new test) - **Lines**: +497 / -155 (most is reformatting from a `ruff format` pass) - **Head SHA verified**: `f04e391` matches `pulls/531.head.sha` - **Net behaviour change**: ~120 lines (rest is whitespace/wrap reformat) ### Scope Wires the existing 3-tier `match_or_create_house()` (cadastr → source_exact → fingerprint → geo-proximity → new) into `_save_yandex_history_items` and persists the 5 columns that PR #526 made available in the parser but `_save_yandex_history_items` was silently dropping: `house_id`, `source_confidence`, `total_floors`, `removed_date`, `notes`. Closes the persistence gap flagged in #526 review. Also reformatted call-sites of `db.execute(text(...))` in the unchanged `_fetch_analogs` / `_fetch_deals` / `_get_or_fetch_imv_cached` blocks; no behaviour delta there. ### Verified correctness (the boring stuff that matters) - **Idempotency**: `UNIQUE (source, ext_item_id)` (017_house_placement_history.sql:50) + `ON CONFLICT (source, ext_item_id) DO NOTHING` preserved. Synthetic `ext_item_id = sha256(address|publish_date|area|floor|prices)[:32]` is deterministic — re-runs no-op. `ext_id` for house resolution = `sha256(strip+lower(address))[:16]` — also stable. - **Schema match**: All bound columns exist — `house_id` (017:22), `total_floors` (017:32), `removed_date` (017:41), `source_confidence` (045:11), `notes` (045:12). No migration needed. - **psycopg v3**: `CAST(:raw AS jsonb)` correct. `house_id`/`total_floors`/`removed_date` bind None → NULL for INSERT VALUES (no CAST needed, that trap is for `IS NOT NULL` predicates only). - **Once-per-page**: confirmed by test `test_save_resolves_house_once_per_page` (m.call_count == 1, saved == 2). Correct — avoids hammering `pg_advisory_xact_lock(42, hashtext(fp))` N times per page. - **Graceful failure path**: `match_or_create_house` raises → `db.rollback()` + `house_id=None, confidence=0.0, method='fail'`, history INSERT then proceeds standalone. `notes` gets None (not `match_method=fail`) per line 524. Test `test_save_handles_match_failure_gracefully` asserts row dict shape correctly. - **Bootstrap context**: PR #527 already populated 6,507 canonical houses in `houses` with `tradein_normalize_short_addr`. PR #531's calls into `match_or_create_house` (via `address_fingerprint` → `normalize_address`) hit a populated `house_address_aliases` table → most yandex pages will resolve via Tier 2 (`fingerprint`, conf=0.9) immediately. Step #5 of migration 063 left 160 yandex rows with `house_id=NULL` because raw_payload had no address — bulk re-run (followup PR #3 per description) backfills them through this new path. - **Dependency contract verified**: opened `matching/houses.py` at head_sha (per memory rule [[feedback_verify_dependency_contract_in_review]]) — `match_or_create_house` *does* raise on real DB errors (`db.execute(...).one()` will raise `NoResultFound`/`IntegrityError`). It is NOT a silent-swallow-and-return-default; the PR's try/except is needed and correct. ### Findings **Cosmetic (non-blocking)** - `estimator.py:793-794` — stale log: `"(house_id=NULL — matching deferred)"` is now wrong on every successful match. INFO-level prod log scrape will show misleading message. Suggested next-PR fix: ```python # estimator.py around line 792 logger.info( "yandex_valuation: history items processed=%d saved=%d", len(yandex_val.history_items), saved_hist, ) ``` **Quality enhancement (next iteration)** - `_save_yandex_history_items` calls `match_or_create_house` with **no `lat`/`lon`**. `geo.lat`/`geo.lon` are available at the call site in `estimate_quality` (Stage 8 runs after geocoding succeeded). Without coords, Tier 3 geo-proximity (30 m PostGIS) is skipped, and the new-house INSERT lands with `lat=NULL, lon=NULL` (so the `houses_set_geom_trg` trigger leaves `geom` NULL too). Recommend threading `geo` into the call: ```python house_id, mc, m = match_or_create_house( db, ext_source="yandex_valuation", ext_id=house_ext_id, address=result.address, lat=geo.lat, lon=geo.lon, # NEW year_built=result.house.year_built, ) ``` Requires passing `geo` through `_save_yandex_history_items` signature. Not a blocker for this PR — the bootstrap from #527 covers most EKB houses via fingerprint Tier 2, but Tier 3 is the safety net for off-by-one address normalizations. **Worth tracking** - `db.rollback()` inside the inner try/except *only* clobbers uncommitted state in the request-scoped Session. Upstream `_get_or_fetch_yandex_valuation_cached` does its own `db.commit()` on line 433 before returning, so the external_valuations cache write is durable. IMV cache writes via `save_imv_evaluation` happen even further upstream — also safe assuming they commit (would need a separate verification pass on `save_imv_evaluation` if that flow is ever changed). For this PR's scope: tx boundaries are fine. - Confidence is currently capped at 0.9 (fingerprint) for known houses. When the addresses literally match `houses.address` via `cadastral_number` (Tier 0) we'd get 1.0, but Yandex pages don't carry cadastr → Tier 0 always skipped. Acceptable. ### Tests - 6 new tests (`test_yandex_valuation_save.py`) cover: once-per-page resolution, row dict house_id/confidence/total_floors/notes propagation, removed_date populated for sold listing, removed_date None for active listing, graceful match failure (saved=N, house_id=None, notes=None, confidence=0.0), kwargs forwarding (address/year_built/ext_source). - 4 existing tests (`test_estimator_yandex_integration.py`) updated with `patch("app.services.estimator.match_or_create_house")` to keep them deterministic. No integration coverage was lost. - Author claims 17/17 pass + ruff clean. The mock patterns and assertions look correct on inspection. ### Security - No credential / token / API-key literals in diff. No SQL injection vectors (all named params). No `git --no-verify` traces in recent log. ### Vault cross-check - `Cross_Source_Matching_Strategy.md` (referenced in `matching/houses.py` docstring) describes the 3-tier algorithm and `source_confidence` semantics from sec 14.1 — PR honors that contract (1.0 cadastr/new, 0.9 fp, 0.7 geo, 0.0 fail). 045_house_placement_history_extend.sql:18-22 also documents these confidence values. - `Schema_Avito_Houses_Listings_History.md` sec 6: `house_id` is FK, NULL allowed for orphan-rows, comment-documented as "Backfill через UPDATE … при первом scrape дома" — this PR is the realtime-scrape version of that backfill, complementary not contradictory. ### Verdict reasoning This is a focused, well-tested change that closes a concrete persistence gap. Code reuses existing infra (`match_or_create_house`) rather than reinventing matching logic, transaction boundaries are sound, all schema columns exist, idempotency is preserved, psycopg v3 conventions followed. The two findings (stale log, missing lat/lon) are low-priority and orthogonal — fine as separate followups. **APPROVE — squash-merge + delete branch.**