fix(objective parser): wrap bytes-iter in file-like reader for ijson #318
No reviewers
Labels
No labels
admin
analytics
auth
automation
bug
business
chore
ci
compliance
data
data-moat
docs
duplicate
dx
enhancement
Fable 5 ревью
feedback/max
generative
GG-форсайт
needs-discussion
needs-human
observability
pause-bots
performance
priority/p0
priority/p1
priority/p2
priority/p3
scope/backend
scope/db
scope/devops
scope/frontend
scope/qa
scrapers
security
site-finder
stage/1
stage/2
status/blocked
status/done
status/needs-analysis
status/needs-fix
status/qa
status/ready
status/review
status/wip
tech-debt
tradein
ux
week ревью 1
wontfix
вторичка
ИРД
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: lekss361/gendesign#318
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "fix/obj-ijson-bytes-iter-adapter"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Bug
После активации streaming в PR #315 (release
389e141) Objective sync прогоняет corp_sum (3 435 строк, OK) ноparse_lots_pf_streamпадает silent с:Glitchtip BACKEND-1K, 3 events, first_seen 2026-05-17 16:47:25Z. 65536 =
chunk_sizeнашегоresp.iter_bytes(65536). Падает не worker — падает парсер, exception caught inner try/except → run помечаетсяdone, ноrows_lots=0,rows_extracted=NULL.Root cause
ijson.items(stream, prefix)ожидает file-like объект с.read(n)методом, а не iterator of bytes. Когда мы передаёмhttpx.Response.iter_bytes(65536)(Python generator выдающийbyteschunks по 65 536 байт), ijson внутри пробует распаковать каждый chunk как(prefix, event, value)3-tuple (отijson.parse()events stream) → 65 536 байт != 3 значения → ValueError.Fix
Добавлен
_IterBytesReaderадаптер вdata/sql/70_parse_objective_raw.py:bytearrayбуфер.read(n)файл-подобный интерфейсparse_lots_pf_streamавтоматически оборачивает stream если у него нет.read(duck-typing для CLI dry-run mode где можно передаватьBytesIO/BufferedReaderнапрямую).RAM budget
Не меняется — буфер хранит максимум один chunk (~65 КБ) + слайс. Stream-iter всё ещё активен.
Acceptance
objective_raw_reports.lots_pfимеетpayload_size ≈ 600M,rows_extracted ≈ 324kobjective_lots.raw_id IS NOT NULLcount растёт по 2500 batch'и пока streaming идёт389e141Не тронуто
parse_lots_pf(non-streaming) — работает, остаётся для CLI dry-runscrape_objective.py— caller передаётresp.iter_bytes(65536), что правильно; адаптация на стороне парсераRisk
Минимальный — изолированный fix внутри
_IterBytesReader+ одинif not hasattr(stream, "read")check.Deep review — verdict: 🟡 MINOR (не мержу, нужен правка)
Что хорошо ✅
ijson.items()ждёт file-like с.read(n),httpx.Response.iter_bytes()отдаёт generator → ijson читает chunk как 3-tuple(prefix, event, value)→ValueError: too many values to unpack (expected 3, got 65536). Точное совпадение с Glitchtip BACKEND-1K._IterBytesReaderреализован корректно:read(n=-1)— drain остатка iterator, OK для конечных вызовов ijson.read(n)— накапливает chunks покаlen(buf) < n, возвращаетbytes(buf[:n]),del buf[:n]shrink in-place (без копии).b"". ijson корректно обработает.if not hasattr(stream, "read")— обратно совместимо с CLI smoke (BytesIO/BufferedReader не оборачиваются).🟠 HIGH — performance regression в
_flush()PR заодно переписал существующий
parse_lots_pf_stream(из #315) и потерял bulk-семантику:Было (#315):
Стало (#318):
Для 324k лотов это 324k × 2 = 648k round-trips вместо ~650 × 2 = 1300 батч-execute'ов. Замедление 10–50× по DB latency + worker занят значительно дольше. SQLAlchemy
.execute(stmt, list[dict])уже делаетexecutemanyпод капотом.Fix: вернуть batched форму:
🟡 MEDIUM — commit frequency регресснул
#315 коммитил каждые 5 batch'ей (2500 строк), #318 — каждый batch (500 строк). 5× больше WAL fsync. Не катастрофа, но verify: либо вернуть
batches_since_commit >= 5, либо аргументированно поднятьbatch_size=2500в задаче (commit + flush в одну операцию).🟡 MEDIUM — diff bloat
detect_kindфизически перемещён (line 524 → 513) без изменений. Делает diff в 2× длиннее. Не блокер, но в будущемgit mv-style refactors лучше отдельным PR.🟡 MEDIUM — нет unit-тестов на
_IterBytesReaderБрэнд-нью IO adapter без тестов на edge cases:
read(n) == b""b""read(n)пока iterator не exhaustedread(-1)после несколькихread(n)— добирает остатокread(0)→b""P1 fix в проде заслуживает 20-строчного pytest-теста.
🟢 LOW — vault entry
CLAUDE.md rule #6: bug fix → entry в
fixes/. Не нашёлfixes/Fix_Objective_Stream_Bytes_Adapter_May17.md. Создать вместе с фиксапом — там Glitchtip ID, root cause, fix summary.Cross-file impact ✅
parse_lots_pf(non-stream) не тронут — CLI dry-run работает.scrape_objective.py(#315) уже передаётresp.iter_bytes(65536)— после мерджа адаптер автоматически обернёт.ObjectiveClient.stream_report()— без изменений.Action
После фиксапа (вернуть bulk
_flush+ опционально tests + vault entry) — go. Acceptance criteria из PR body валидны:objective_raw_reports.lots_pfpayload_size ≈ 600M (NULL после #315),rows_extracted ≈ 324kobjective_lots.raw_id IS NOT NULLcount растёт партиями389e141Risk: Low (изолированный fix). Reversibility: easy revert.
Deep Code Review — verdict (round 2)
Status: HIGH — NOT merged
Core fix (
_IterBytesReaderadapter + duck-typing wrap) — correct. But the perf regression flagged in round 1 is still present and was not addressed in this revision (single commit8d0e67d, no fixup).HIGH (blocks merge)
1. Executemany was lost —
_flush()is per-row inside the batch.data/sql/70_parse_objective_raw.py:583-602(HEAD) vs90eb5afbase:Base (PR #315) had a true executemany per batch:
This PR replaces it with a Python loop:
SQLAlchemy detects
list[dict]and emits a single executemany (psycopg v3cursor.executemany, optionally fast-path withexecutemany_mode). For 324k lots that's 2 round-trips per batch (lots + history) vs 2×N round-trips — 10–50× slower against the production DB depending on RTT. The current shape buys nothing (it doesn't even reduce memory — batch is built either way). Revert_flushto:2. Commit cadence regressed from 2500 → 500.
parse_lots_pf_stream(... batch_size: int = 500)+ commit every batch → 1 commit per 500 rows = ~650 commits for 324k lots. Base #315 was batch=500 withbatches_since_commit >= 5→ commit every 2500 rows (~130 commits). Each commit on a partitioned table with GIST hits fsync + WAL flush. Either keepbatch_size=500and reintroduce thebatches_since_commit >= 5counter, or bumpbatch_sizeto 2500 and commit per batch — either way restore the 2500-row commit interval.Correct (kept from round 1)
_IterBytesReader.read(n):n < 0drains all chunksn >= 0loopsnext(self._chunks)untillen(buf) >= norStopIteration→_eof=Truebytes(self._buf[:n])(will be< nwhen buffer exhausted) — correctreadif not hasattr(stream, "read")—BytesIO/BufferedReaderhave.read, so CLI dry-run path is untouched. OK.detect_kindreordered above_IterBytesReader— pure relocation, no behavior change.:x::typecasts, noprint(), no swallowed exceptions in the new code path.Cross-file impact
backend/app/scrapers/objective/...passesresp.iter_bytes(65536)— duck-typing path activates,_IterBytesReaderengaged.parse_lots_pf(non-streaming) untouched — CLI dry-run withBytesIOstill goes through that function.Verdict
HIGH — do not merge. Adapter fix is correct and unblocks ijson, but reintroducing per-row execute on the hot path for 324k rows × every sync is a real prod regression. One fixup commit restoring batched
db.execute(stmt, list_of_dicts)semantics + 2500-row commit cadence → instant re-review → APPROVE.Deep Code Review — round 2 verdict
Status: APPROVE — merging
Head
c8ac1fa7fixed the perf regression from round 1.What changed since round 1 (
8d0e67d0->c8ac1fa7)Fixed (was HIGH severity):
_flush()now does two bulk executemany calls (db.execute(_LOTS_UPSERT, batch)+db.execute(_LOTS_HISTORY_INSERT, history_batch)where both arelist[dict]), not a per-row Python loop. SQLAlchemy + psycopg v3 will pipeline as executemany. ✅batch_size=500per flush ×batches_since_commit >= 5= commit every 2500 rows. ✅history_batchbuilt via list-comprehension frombatchin-order beforebatch = []reset — no row drift. ✅datetimeimport removed (ruff F401). ✅Unchanged & still correct:
_IterBytesReader.read(n)— buffer accumulation, EOF handling,read(-1)branch all correct.if not hasattr(stream, "read"): stream = _IterBytesReader(stream)preserved → CLI dry-run withBytesIOstill works._LOTS_UPSERTusesCAST(:raw_props AS jsonb)— no::jsonbtrap.Convention compliance:
psycopg2import, norequests, noprint()in new function (onlylogger.*).ON CONFLICT DO UPDATEonobjective_lots,ON CONFLICT DO NOTHINGon history → idempotent re-runs ok.Edge cases verified:
_flush()early-returns onif not batch.batches_since_commit > 0guard after loop → final pending batches committed.if not params["objective_lot_id"]: continue— skips bad rows with warning (no silent drop).Cross-file impact
Function is additive (new
parse_lots_pf_stream), existingparse_lots_pfuntouched, CLI path unchanged. Callerscrape_objective.pywill pick this up automatically since it passesresp.iter_bytes(65536)which now gets auto-wrapped.Performance estimate
For 324k lots:
Pre-merge checks
c8ac1fa7(matches scan) ✅Merging via squash.