lekss361/gendesign

Fork 0

feat(tradein/cian): valuation backfill + runner script для prod #539

Merged

lekss361 merged 1 commit from feat/cian-valuation-backfill into main

2026-05-24 15:16:45 +00:00

lekss361 commented

2026-05-24 15:10:03 +00:00

Owner

What

Extends the backfill task from PR #535 to also populate price predictions (external_valuations via Cian Calculator), плюс shell runner который drains backlog в prod одной командой.

User ask: «скрипт для наполнения новой таблицы в проде всех исторических и предикта стоимости».

Backend extension

tradein-mvp/backend/app/tasks/cian_history_backfill.py — new block 3 if do_valuations: iterates Cian listings WITH full geo (address, area_m2, rooms, floor, total_floors) AND NOT EXISTS in external_valuations для них (filter expires_at > NOW()). For each: await estimate_via_cian_valuation(db, ..., use_cache=False, listing_id=l.id) — fresh fetch, auto-кэш в external_valuations с canonical listing_id FK (mig 044).
4 новых counter fields в CianBackfillResult + соответствующее в CianBackfillResp (valuations_total/processed/succeeded/failed).
Endpoint param do_valuations: bool = False — default False для backward compat.

Runner script

tradein-mvp/scripts/cian_backfill_all.sh — set -euo pipefail. Loops POST endpoint пока listings_total + valuations_total == 0. Env: TRADEIN_API_URL, BATCH_SIZE (max 200 per endpoint cap), MAX_ITERS, DO_LISTINGS, DO_VALUATIONS, DO_HOUSES (default false — нет cian_zhk_url col). --dry-run для пробы, --help для usage. Per-item commit в task → cancel-safe (Ctrl+C не оставит half-state).

How to use в prod

# Dry-run (count what would be processed):
TRADEIN_API_URL=https://gendsgn.ru/api ./scripts/cian_backfill_all.sh --dry-run

# Real backfill — оба слоя:
TRADEIN_API_URL=https://gendsgn.ru/api BATCH_SIZE=50 ./scripts/cian_backfill_all.sh

# Только история (без Calculator — экономит cookie session):
TRADEIN_API_URL=https://gendsgn.ru/api DO_VALUATIONS=false ./scripts/cian_backfill_all.sh

Rate limit: scraper_settings cian delay (default 5s). При 100 listings × 5s × 2 phases = ~17 мин/batch. Endpoint timeout safe — batch_size capped 1..200 (PR #535).

Test plan

ruff clean (verified)
Pre-push review ✅ APPROVE with cosmetic notes
Manual: --dry-run против local backend → expect listings_total/valuations_total non-zero counts
Real batch=10 → expect rows в offer_price_history (где не было) + external_valuations с source=cian_valuation, listing_id non-NULL
Idempotency: re-run → expect listings_total и valuations_total = 0 (drained)

Follow-ups

Houses backfill — заблокирован на отсутствии cian_zhk_url колонки (mig 020 имеет только cian_internal_house_id bigint). Migration 065 + populate во время SERP scrape — отдельный PR.
Если backfill batch > ~30 мин — перейти на FastAPI BackgroundTasks для async execution.

Follows up Cian history+valuation activation 2026-05-24 (4-PR sequence #523/#525/#530/#535).

## What Extends the backfill task from PR #535 to also populate **price predictions** (`external_valuations` via Cian Calculator), плюс shell runner который drains backlog в prod одной командой. User ask: «скрипт для наполнения новой таблицы в проде всех исторических и предикта стоимости». ## Backend extension - `tradein-mvp/backend/app/tasks/cian_history_backfill.py` — new block 3 `if do_valuations:` iterates Cian listings WITH full geo (address, area_m2, rooms, floor, total_floors) AND `NOT EXISTS` in `external_valuations` для них (filter `expires_at > NOW()`). For each: `await estimate_via_cian_valuation(db, ..., use_cache=False, listing_id=l.id)` — fresh fetch, auto-кэш в `external_valuations` с canonical `listing_id` FK (mig 044). - 4 новых counter fields в `CianBackfillResult` + соответствующее в `CianBackfillResp` (`valuations_total/processed/succeeded/failed`). - Endpoint param `do_valuations: bool = False` — default False для backward compat. ## Runner script `tradein-mvp/scripts/cian_backfill_all.sh` — `set -euo pipefail`. Loops POST endpoint пока `listings_total + valuations_total == 0`. Env: `TRADEIN_API_URL`, `BATCH_SIZE` (max 200 per endpoint cap), `MAX_ITERS`, `DO_LISTINGS`, `DO_VALUATIONS`, `DO_HOUSES` (default false — нет `cian_zhk_url` col). `--dry-run` для пробы, `--help` для usage. Per-item commit в task → cancel-safe (Ctrl+C не оставит half-state). ## How to use в prod ```bash # Dry-run (count what would be processed): TRADEIN_API_URL=https://gendsgn.ru/api ./scripts/cian_backfill_all.sh --dry-run # Real backfill — оба слоя: TRADEIN_API_URL=https://gendsgn.ru/api BATCH_SIZE=50 ./scripts/cian_backfill_all.sh # Только история (без Calculator — экономит cookie session): TRADEIN_API_URL=https://gendsgn.ru/api DO_VALUATIONS=false ./scripts/cian_backfill_all.sh ``` Rate limit: `scraper_settings` `cian` delay (default 5s). При 100 listings × 5s × 2 phases = ~17 мин/batch. Endpoint timeout safe — `batch_size` capped 1..200 (PR #535). ## Test plan - [x] ruff clean (verified) - [x] Pre-push review ✅ APPROVE with cosmetic notes - [ ] Manual: `--dry-run` против local backend → expect listings_total/valuations_total non-zero counts - [ ] Real batch=10 → expect rows в `offer_price_history` (где не было) + `external_valuations` с `source=cian_valuation`, `listing_id` non-NULL - [ ] Idempotency: re-run → expect listings_total и valuations_total = 0 (drained) ## Follow-ups - Houses backfill — заблокирован на отсутствии `cian_zhk_url` колонки (mig 020 имеет только `cian_internal_house_id bigint`). Migration 065 + populate во время SERP scrape — отдельный PR. - Если backfill batch > ~30 мин — перейти на FastAPI BackgroundTasks для async execution. Follows up [Cian history+valuation activation 2026-05-24](https://git.gendsgn.ru/lekss361/gendesign/pulls/535) (4-PR sequence #523/#525/#530/#535).

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

feat(tradein/cian): valuation backfill + runner script для prod da38cd0a5f

lekss361 commented

2026-05-24 15:16:36 +00:00

Author

Owner

Deep Code Review — PR #539

Summary

Status: ✅ APPROVE (with P1 follow-up note)
Files: 3 (P1: 2 backend, P2: 1 shell)
Lines: +141 / -2
Head SHA: da38cd0

Scope

Extends PR #535 backfill with block 3 (if do_valuations) iterating Cian listings missing valid external_valuations rows (mig 044 listing_id FK), calling estimate_via_cian_valuation(..., use_cache=False, listing_id=l.id). New 4 counters, new endpoint flag do_valuations: bool = False (backward-compat default). Adds scripts/cian_backfill_all.sh drain runner with env-knob config, --dry-run, --help, MAX_ITERS=1000 safety.

🟠 P1 — Idempotency hole on cache_key collision (recommended follow-up, not blocking)

external_valuations uniqueness is UNIQUE (source, cache_key) (mig 026). cache_key = sha256(address + total_area + rooms + floor + repair + deal_type) — not keyed by listing_id. In _save_to_cache (cian_valuation.py:418) the conflict path does listing_id = COALESCE(EXCLUDED.listing_id, external_valuations.listing_id) — preserves the first listing_id.

When two cian listings share identical normalized params (re-listings, agency duplicates, multi-flat same-building strings before per-flat refinement, etc.), only one ever gets ev.listing_id = l.id. The new NOT EXISTS filter:

NOT EXISTS (
    SELECT 1 FROM external_valuations ev
    WHERE ev.source = 'cian_valuation'
      AND ev.listing_id = l.id
      AND ev.expires_at > NOW()
)

never matches the second listing → it re-fetches Cian on every drain run despite a valid cached row existing under the same cache_key. The runner's stop condition (lt + vt == 0) may then never reach 0 — MAX_ITERS=1000 is the safety net but it's silent quota burn.

Suggested follow-up (one of):

Add fallback filter — skip if any cache_key-matching row exists:

AND NOT EXISTS (
    SELECT 1 FROM external_valuations ev2
    WHERE ev2.source = 'cian_valuation'
      AND ev2.address = l.address
      AND ev2.total_area = l.area_m2
      AND ev2.rooms_count = l.rooms
      AND ev2.floor = l.floor
      AND ev2.expires_at > NOW()
)

Or drop COALESCE for listing_id in _save_to_cache ON CONFLICT — overwrite with latest. Loses house-level linkage if upstream changes, but ensures each backfill round writes back its listing_id.
Compute cache_key in the SELECT (digest function) and filter on it directly.

Practical impact: low if address dedup upstream is tight; not regressing existing behavior (block 3 is new + opt-in).

🟡 P2 — Background mode parity

Peer endpoint POST /scrape/geocode-missing-listings (admin.py:370) supports background: bool + BackgroundTasks + own SessionLocal(). cian-backfill-history doesn't (inherited from #535). With batch_size=200 × 5s × 2 phases ≈ 33 min, synchronous run will 504 through Caddy/proxy. Runner mitigates by looping smaller batches (default 100 → ~17 min/iter), but still over typical 120s proxy timeouts.

Not blocking — already flagged on #535 by reviewer memory; runner provides workaround. Worth a 10-line follow-up to add background=true like the geocode peer.

🟡 P2 — `use_cache=False` wastes intra-batch cache hits

If the same address appears twice in a single 100-row batch (rare but possible), the second iteration re-fetches Cian instead of hitting the just-written cache row. Trade-off acknowledged in code comment (# force fresh fetch — populate cache). Acceptable for backfill semantics where the user wants fresh data.

🟢 P3 — Cosmetic

do_houses=true env override (non-default) would print the same no-op log every loop iteration. Default false, so usually moot.
cval.sale_price_rub truthy check treats 0 as failure (impossible value in practice, fine).
Shell --dry-run parsing runs before --help check; --dry-run --help sets DRY then exits via help. Harmless.

Positive

Dependency contract verified: estimate_via_cian_valuation signature matches (use_cache: bool, listing_id: int | None, returns CianValuationResult | None, graceful — no exceptions propagate from cookies/fetch failures). _save_to_cache uses correct CAST for psycopg v3 (CAST(:lid AS bigint), CAST(:ta AS numeric)).
Per-row try/except — error isolation, single bad row won't poison batch counter.
Counter accounting clean: valuations_succeeded vs valuations_failed from cval is not None and cval.sale_price_rub.
Opt-in do_valuations=False default → no behavioral change for existing #535 callers/cron.
Runner: set -euo pipefail, curl -fsS (fail on HTTP error), MAX_ITERS=1000 safety cap, env-knob config with sane defaults.
expires_at > NOW() in NOT EXISTS respects 24h TTL — re-runs after a day will refresh stale rows.
API endpoint Query(50, ge=1, le=200) cap honored; runner can't bypass.
No SQL injection vectors (all params bound via text()).
No literal secrets in diff.
No interaction risk with advisory lock (#501) or match_or_create_house (#527/#531/#534) — separate code path.

Tests

No new tests added for block 3. Consistent with #535 pattern (no backfill tests there either). tests/test_cian_valuation.py covers underlying API. Manual --dry-run + small batch verification documented in PR body. Acceptable.

Risk / blast radius

Reversibility: high — opt-in flag, default off; no migrations
Blast radius: contained to admin endpoint + new task block + new shell script (not auto-cron, not background scheduled)
Merge window: any

Ready to merge.

## Deep Code Review — PR #539 ### Summary - Status: ✅ APPROVE (with P1 follow-up note) - Files: 3 (P1: 2 backend, P2: 1 shell) - Lines: +141 / -2 - Head SHA: `da38cd0` ### Scope Extends PR #535 backfill with block 3 (`if do_valuations`) iterating Cian listings missing valid `external_valuations` rows (mig 044 listing_id FK), calling `estimate_via_cian_valuation(..., use_cache=False, listing_id=l.id)`. New 4 counters, new endpoint flag `do_valuations: bool = False` (backward-compat default). Adds `scripts/cian_backfill_all.sh` drain runner with env-knob config, `--dry-run`, `--help`, `MAX_ITERS=1000` safety. ### 🟠 P1 — Idempotency hole on cache_key collision (recommended follow-up, not blocking) `external_valuations` uniqueness is `UNIQUE (source, cache_key)` (mig 026). `cache_key = sha256(address + total_area + rooms + floor + repair + deal_type)` — *not* keyed by listing_id. In `_save_to_cache` (`cian_valuation.py:418`) the conflict path does `listing_id = COALESCE(EXCLUDED.listing_id, external_valuations.listing_id)` — preserves the *first* listing_id. When two cian listings share identical normalized params (re-listings, agency duplicates, multi-flat same-building strings before per-flat refinement, etc.), only one ever gets `ev.listing_id = l.id`. The new NOT EXISTS filter: ```sql NOT EXISTS ( SELECT 1 FROM external_valuations ev WHERE ev.source = 'cian_valuation' AND ev.listing_id = l.id AND ev.expires_at > NOW() ) ``` never matches the second listing → it re-fetches Cian on *every* drain run despite a valid cached row existing under the same `cache_key`. The runner's stop condition (`lt + vt == 0`) may then never reach 0 — `MAX_ITERS=1000` is the safety net but it's silent quota burn. **Suggested follow-up** (one of): 1. Add fallback filter — skip if any cache_key-matching row exists: ```sql AND NOT EXISTS ( SELECT 1 FROM external_valuations ev2 WHERE ev2.source = 'cian_valuation' AND ev2.address = l.address AND ev2.total_area = l.area_m2 AND ev2.rooms_count = l.rooms AND ev2.floor = l.floor AND ev2.expires_at > NOW() ) ``` 2. Or drop `COALESCE` for `listing_id` in `_save_to_cache` ON CONFLICT — overwrite with latest. Loses house-level linkage if upstream changes, but ensures each backfill round writes back its listing_id. 3. Compute `cache_key` in the SELECT (digest function) and filter on it directly. Practical impact: low if address dedup upstream is tight; not regressing existing behavior (block 3 is new + opt-in). ### 🟡 P2 — Background mode parity Peer endpoint `POST /scrape/geocode-missing-listings` (admin.py:370) supports `background: bool` + `BackgroundTasks` + own `SessionLocal()`. `cian-backfill-history` doesn't (inherited from #535). With `batch_size=200 × 5s × 2 phases ≈ 33 min`, synchronous run will 504 through Caddy/proxy. Runner mitigates by looping smaller batches (default 100 → ~17 min/iter), but still over typical 120s proxy timeouts. Not blocking — already flagged on #535 by reviewer memory; runner provides workaround. Worth a 10-line follow-up to add `background=true` like the geocode peer. ### 🟡 P2 — `use_cache=False` wastes intra-batch cache hits If the same address appears twice in a single 100-row batch (rare but possible), the second iteration re-fetches Cian instead of hitting the just-written cache row. Trade-off acknowledged in code comment (`# force fresh fetch — populate cache`). Acceptable for backfill semantics where the user wants fresh data. ### 🟢 P3 — Cosmetic - `do_houses=true` env override (non-default) would print the same no-op log every loop iteration. Default `false`, so usually moot. - `cval.sale_price_rub` truthy check treats 0 as failure (impossible value in practice, fine). - Shell `--dry-run` parsing runs before `--help` check; `--dry-run --help` sets DRY then exits via help. Harmless. ### Positive - Dependency contract verified: `estimate_via_cian_valuation` signature matches (`use_cache: bool`, `listing_id: int | None`, returns `CianValuationResult | None`, graceful — no exceptions propagate from cookies/fetch failures). `_save_to_cache` uses correct CAST for psycopg v3 (`CAST(:lid AS bigint)`, `CAST(:ta AS numeric)`). - Per-row try/except — error isolation, single bad row won't poison batch counter. - Counter accounting clean: `valuations_succeeded` vs `valuations_failed` from `cval is not None and cval.sale_price_rub`. - Opt-in `do_valuations=False` default → no behavioral change for existing #535 callers/cron. - Runner: `set -euo pipefail`, `curl -fsS` (fail on HTTP error), `MAX_ITERS=1000` safety cap, env-knob config with sane defaults. - `expires_at > NOW()` in NOT EXISTS respects 24h TTL — re-runs after a day will refresh stale rows. - API endpoint `Query(50, ge=1, le=200)` cap honored; runner can't bypass. - No SQL injection vectors (all params bound via `text()`). - No literal secrets in diff. - No interaction risk with advisory lock (#501) or `match_or_create_house` (#527/#531/#534) — separate code path. ### Tests No new tests added for block 3. Consistent with #535 pattern (no backfill tests there either). `tests/test_cian_valuation.py` covers underlying API. Manual `--dry-run` + small batch verification documented in PR body. Acceptable. ### Risk / blast radius - Reversibility: high — opt-in flag, default off; no migrations - Blast radius: contained to admin endpoint + new task block + new shell script (not auto-cron, not background scheduled) - Merge window: any Ready to merge.