lekss361/gendesign

Fork 0

fix(tradein-phase-c): prepend region prefix for IMV geocoder #540

Merged

lekss361 merged 1 commit from fix/tradein-phase-c-region-prefix into main

2026-05-24 15:39:56 +00:00

lekss361 commented

2026-05-24 15:28:04 +00:00

Owner

Problem

Live Phase C dry-run на проде показал ~30% HTTP 404 от Avito IMV. Логирование response body (added in PR #537) показало {"code":404,"message":"Not Found"}. Причина — Avito /coords/by_address mis-resolves ambiguous addresses to wrong regions:

House	Input address	Avito resolved to	Status
6703	`улица Академика Королева, 8Г`	Беларусь, Могилёв (там тоже Korolyov street)	404
6705	`р-н Орджоникидзевский, мкр. Уралмаш...`	Республика Хакасия (там тоже Орджоникидзевский район!)	404
6704	`р-н Кировский, ул. Студенческая, 1Нк4`	Екатеринбург ✓	OK

Наши houses.address для derived rows часто содержат только district + street без региона. Avito picks ANY matching location.

Fix

scripts/backfill-house-imv.py:

_REGION_BBOX — bounding box → region name (EKB, Свердловская обл, Киров, Ставрополь, Тюмень, Челябинск)
_REGION_DEFAULT = 'Свердловская область, Екатеринбург' для NULL coords
_detect_region_prefix(lat, lon) — walks bboxes
_enrich_address_for_imv(raw, lat, lon) — skip если есть область,/обл./край/респ., иначе prepend prefix
process_one() использует enriched address для evaluate_via_imv

Verify after deploy

Re-run failed houses:

DATABASE_URL=... python tradein-mvp/scripts/backfill-house-imv.py --batch 20 --delay 3 --only-status error
# Expected: 90%+ ok rate (vs 33% before fix)

Follow-up

Follow-up к PR #534 (Phase C kickoff), #537 (house_type mapping + 400 logging). Roadmap chain so far: 9/9 original + IMV + 3 backfill + Phase C kickoff + 2 phase-C fixups.

## Problem Live Phase C dry-run на проде показал ~30% **HTTP 404** от Avito IMV. Логирование response body (added in PR #537) показало `{"code":404,"message":"Not Found"}`. Причина — Avito `/coords/by_address` mis-resolves ambiguous addresses to wrong regions: | House | Input address | Avito resolved to | Status | |---|---|---|---| | 6703 | `улица Академика Королева, 8Г` | **Беларусь, Могилёв** (там тоже Korolyov street) | 404 | | 6705 | `р-н Орджоникидзевский, мкр. Уралмаш...` | **Республика Хакасия** (там тоже Орджоникидзевский район!) | 404 | | 6704 | `р-н Кировский, ул. Студенческая, 1Нк4` | Екатеринбург ✓ | OK | Наши `houses.address` для derived rows часто содержат только district + street без региона. Avito picks ANY matching location. ## Fix `scripts/backfill-house-imv.py`: - `_REGION_BBOX` — bounding box → region name (EKB, Свердловская обл, Киров, Ставрополь, Тюмень, Челябинск) - `_REGION_DEFAULT = 'Свердловская область, Екатеринбург'` для NULL coords - `_detect_region_prefix(lat, lon)` — walks bboxes - `_enrich_address_for_imv(raw, lat, lon)` — skip если есть `область,/обл./край/респ.`, иначе prepend prefix - `process_one()` использует enriched address для evaluate_via_imv ## Verify after deploy Re-run failed houses: ```bash DATABASE_URL=... python tradein-mvp/scripts/backfill-house-imv.py --batch 20 --delay 3 --only-status error # Expected: 90%+ ok rate (vs 33% before fix) ``` ## Follow-up Follow-up к PR #534 (Phase C kickoff), #537 (house_type mapping + 400 logging). Roadmap chain so far: 9/9 original + IMV + 3 backfill + Phase C kickoff + 2 phase-C fixups.

lekss361 added 1 commit 2026-05-24 15:28:04 +00:00

fix(tradein-phase-c): prepend region prefix to addresses for IMV geocoder 57dd989e58

Avito /coords/by_address mis-resolves ambiguous addresses to wrong regions.
Examples: 'улица Академика Королева, 8Г' -> Беларусь, Могилёв (404);
'р-н Орджоникидзевский, мкр. Уралмаш...' -> Хакасия (404).

Add _REGION_BBOX lookup (lat/lon -> region name) + _enrich_address_for_imv()
that prepends region prefix if address doesn't already have one. Default EKB
fallback when coords are NULL.

lekss361 commented

2026-05-24 15:38:04 +00:00

Author

Owner

hello world

lekss361 commented

2026-05-24 15:39:40 +00:00

Author

Owner

Deep Code Review — PR #540 verdict

Summary

Status: APPROVE (with follow-up notes for next iteration)
Files: 1 (P1 — backfill script, no production code path)
Lines: +47 / -1
SHA verified: 57dd989e58

Why approve

Fix is scoped to one-off backfill script (scripts/backfill-house-imv.py) — does NOT touch production estimator. Blast radius minimal.
Bbox detection deterministic, no extra HTTP, idempotent on re-run.
Skip-already-has-region guard prevents most double-prefix cases.
Debug logging added on enrichment (logger.debug — good restraint, no log spam at INFO).
Fallback default (EKB) matches PR body claim "bulk of data is EKB". Reasonable for current dataset.

MEDIUM — region-marker substrings miss common shapes (worth a follow-up)

Marker substrings require a comma immediately after the marker word. Real inputs that slip through and get prefixed with EKB anyway:

Input	Reason	Result
`Республика Татарстан, Казань`	substring `республика,` is not present — actual text is `республика`	`EKB, Республика Татарстан, Казань` (double region)
`Респ. Татарстан, Казань`	substring `респ.,` not present — actual is `респ.`	double region
`Свердловская область` (bare)	no trailing comma → no match	`EKB, Свердловская область`
`Свердловская обл.` (bare)	same	double prefix

Suggested fix next PR: use word-presence match without requiring trailing comma:

MARKERS = ("область", "обл.", "обл,", "край", "респ", "республика")
if any(m in addr_lower for m in MARKERS):
    return addr

MEDIUM — non-bbox houses (incl. Moscow/SPb) silently get EKB prefix

_REGION_DEFAULT = "Свердловская область, Екатеринбург" is applied to ANY lat/lon outside the 6 listed bboxes. Misclassifications:

City	Coords	Detected
Tobolsk	58.196, 68.255	EKB (out of Tyumen city bbox lon)
Magnitogorsk	53.412, 58.984	EKB (out of Chelyabinsk bbox lat)
Krasnoufimsk	56.611, 57.769	EKB (out of Sverdlovsk-obl bbox lon_min=58.0)
Anything Moscow	~55.75, ~37.6	EKB

If current dataset has no such rows — fine. Worth adding a WARN log when _detect_region_prefix falls back to default, so future non-EKB rows are visible in batch logs.

LOW — cache_key divergence between backfill and live estimator

compute_imv_cache_key is keyed on raw address.

Backfill writes cache with enriched address (Свердловская обл, Екатеринбург, ул. X, 1).
Live estimator (backend/app/services/estimator.py:189) computes with raw house.address → cache MISS → duplicate Avito API call.

Not a correctness issue (extra API call, not stale data), but defeats the 24h dedup goal. Future cleanup: push enrichment into evaluate_via_imv() or normalize before compute_imv_cache_key.

LOW — possible city duplication for already-city-qualified addresses

Address like Екатеринбург, ул. Победы, 1 (no region marker, just city) enriches to Свердловская область, Екатеринбург, Екатеринбург, ул. Победы, 1. Likely tolerated by Avito but inelegant.

LOW — no unit test for `_enrich_address_for_imv`

A 10-line test_enrich_address_for_imv.py parameterized over the marker-detection table would have caught issue #1. Not a blocker for a backfill script.

Cross-file impact

evaluate_via_imv(address=...) — only call site changed is process_one() in this script. Estimator service unchanged.
compute_imv_cache_key — not modified; address-key divergence noted above.
Migration files — none touched.

Positive

if enriched != address: logger.debug(...) — diff-only logging, no log spam.
Inline comments explain WHY (Khakasia ambiguity example) — future-reader friendly.
Bounding boxes are tuples (module-level immutable constants).

Recommended follow-ups (not blocking)

Fix marker detection to not require trailing comma (issue #1).
Add WARN log when bbox detection falls through to default (issue #2 visibility).
Move enrichment into evaluate_via_imv so estimator + backfill share cache (issue #3).
Add test_enrich_address_for_imv table-driven test.

Complexity / blast radius

Risk: LOW (backfill-only, idempotent, resumable, bad prefix produces 404 = same as today's failure mode).
Reversibility: trivial (delete 47 lines).
Merge window: anytime.

Posting verdict and auto-merging per any-scope policy.

## Deep Code Review — PR #540 verdict ### Summary - **Status:** APPROVE (with follow-up notes for next iteration) - **Files:** 1 (P1 — backfill script, no production code path) - **Lines:** +47 / -1 - **SHA verified:** 57dd989e580116e25fe27643e5ba7d3308516f28 ### Why approve - Fix is scoped to one-off backfill script (`scripts/backfill-house-imv.py`) — does NOT touch production estimator. Blast radius minimal. - Bbox detection deterministic, no extra HTTP, idempotent on re-run. - Skip-already-has-region guard prevents most double-prefix cases. - Debug logging added on enrichment (`logger.debug` — good restraint, no log spam at INFO). - Fallback default (EKB) matches PR body claim "bulk of data is EKB". Reasonable for current dataset. ### MEDIUM — region-marker substrings miss common shapes (worth a follow-up) Marker substrings require a comma immediately after the marker word. Real inputs that slip through and get prefixed with EKB anyway: | Input | Reason | Result | |---|---|---| | `Республика Татарстан, Казань` | substring `республика,` is not present — actual text is `республика ` | `EKB, Республика Татарстан, Казань` (double region) | | `Респ. Татарстан, Казань` | substring `респ.,` not present — actual is `респ. ` | double region | | `Свердловская область` (bare) | no trailing comma → no match | `EKB, Свердловская область` | | `Свердловская обл.` (bare) | same | double prefix | Suggested fix next PR: use word-presence match without requiring trailing comma: ```python MARKERS = ("область", "обл.", "обл,", "край", "респ", "республика") if any(m in addr_lower for m in MARKERS): return addr ``` ### MEDIUM — non-bbox houses (incl. Moscow/SPb) silently get EKB prefix `_REGION_DEFAULT = "Свердловская область, Екатеринбург"` is applied to ANY lat/lon outside the 6 listed bboxes. Misclassifications: | City | Coords | Detected | |---|---|---| | Tobolsk | 58.196, 68.255 | EKB (out of Tyumen city bbox lon) | | Magnitogorsk | 53.412, 58.984 | EKB (out of Chelyabinsk bbox lat) | | Krasnoufimsk | 56.611, 57.769 | EKB (out of Sverdlovsk-obl bbox lon_min=58.0) | | Anything Moscow | ~55.75, ~37.6 | EKB | If current dataset has no such rows — fine. Worth adding a WARN log when `_detect_region_prefix` falls back to default, so future non-EKB rows are visible in batch logs. ### LOW — cache_key divergence between backfill and live estimator `compute_imv_cache_key` is keyed on raw address. - Backfill writes cache with enriched address (`Свердловская обл, Екатеринбург, ул. X, 1`). - Live estimator (`backend/app/services/estimator.py:189`) computes with raw `house.address` → cache MISS → duplicate Avito API call. Not a correctness issue (extra API call, not stale data), but defeats the 24h dedup goal. Future cleanup: push enrichment into `evaluate_via_imv()` or normalize before `compute_imv_cache_key`. ### LOW — possible city duplication for already-city-qualified addresses Address like `Екатеринбург, ул. Победы, 1` (no region marker, just city) enriches to `Свердловская область, Екатеринбург, Екатеринбург, ул. Победы, 1`. Likely tolerated by Avito but inelegant. ### LOW — no unit test for `_enrich_address_for_imv` A 10-line `test_enrich_address_for_imv.py` parameterized over the marker-detection table would have caught issue #1. Not a blocker for a backfill script. ### Cross-file impact - `evaluate_via_imv(address=...)` — only call site changed is `process_one()` in this script. Estimator service unchanged. - `compute_imv_cache_key` — not modified; address-key divergence noted above. - Migration files — none touched. ### Positive - `if enriched != address: logger.debug(...)` — diff-only logging, no log spam. - Inline comments explain WHY (Khakasia ambiguity example) — future-reader friendly. - Bounding boxes are tuples (module-level immutable constants). ### Recommended follow-ups (not blocking) 1. Fix marker detection to not require trailing comma (issue #1). 2. Add WARN log when bbox detection falls through to default (issue #2 visibility). 3. Move enrichment into `evaluate_via_imv` so estimator + backfill share cache (issue #3). 4. Add `test_enrich_address_for_imv` table-driven test. ### Complexity / blast radius - **Risk:** LOW (backfill-only, idempotent, resumable, bad prefix produces 404 = same as today's failure mode). - **Reversibility:** trivial (delete 47 lines). - **Merge window:** anytime. --- *Posting verdict and auto-merging per any-scope policy.*