diff --git a/app/api/batch_common.py b/app/api/batch_common.py index 458897d..3c95059 100644 --- a/app/api/batch_common.py +++ b/app/api/batch_common.py @@ -65,9 +65,10 @@ async def read_uploads( ) -> List[Tuple[str, bytes]]: """Read every upload into memory, enforcing the count and size ceilings. - Read here rather than in the worker for the reason `store_catalog.py` gives: - `UploadFile` is backed by a temporary file tied to the request, and it is - gone before a background thread would reach it. + Read here rather than in the worker because `UploadFile` is backed by a + temporary file tied to the request: FastAPI closes it when the response is + returned, so a background thread reaching for it later finds nothing. The + bytes have to be taken while the request is still alive. """ if not files: raise HTTPException(status_code=400, detail="No files were uploaded.") @@ -114,9 +115,10 @@ async def read_uploads( def parse_all(read: List[Tuple[str, bytes]], limits: UploadLimits): """Split the uploads into (valid, invalid) by trying to parse each one. - Failing here is what stops a batch transitioning straight to "failed" a - second after it started - the same reasoning as `store_catalog.py`, applied - per file so one bad sheet does not condemn the others. + Parsing up front is what stops a batch transitioning straight to "failed" a + second after it started: an unreadable sheet is rejected in the response the + caller is still waiting on, not in a manifest they have to go and poll for. + Applied per file, so one bad sheet does not condemn the others. """ valid: List[Tuple[str, bytes, int]] = [] invalid: List[Tuple[str, str]] = [] diff --git a/app/api/routers/batch_catalog.py b/app/api/routers/batch_catalog.py index bd4c941..d7e4149 100644 --- a/app/api/routers/batch_catalog.py +++ b/app/api/routers/batch_catalog.py @@ -10,20 +10,25 @@ POST /api/admin/catalog-batch/from-inbox - run selected files POST /api/admin/catalog-batch/inbox/dismiss - discard selected files -This is the multi-file sibling of `store_catalog.py`, and it deliberately does -not replace it: the single-file endpoints are untouched and still work. What is -different is the unit of work. Five files are one batch with one id, so the -question a colleague actually asks - "did the drop land?" - has one answer -rather than five. +These began as the multi-file sibling of a single-file admin uploader +(`store_catalog.py` and its Store Catalog Ingestion tab). THAT UPLOADER AND ITS +ROUTES HAVE SINCE BEEN DELETED - do not go looking for them - and this router is +now the only admin way in. What made it the survivor is the unit of work: five +files are one batch with one id, so the question a colleague actually asks - +"did the drop land?" - has one answer rather than five. WHY THE NETWORK STAGES DEFAULT OFF HERE --------------------------------------- -The single-file UI sends `use_llm=true, fetch_images=true`. At one file that is -a considered trade. At twenty it is thousands of outbound requests and, for the -image stage, a Playwright subprocess that can burn three minutes on its own - -on a single-vCPU container that is also serving the API. So a batch opts IN to -those stages; it does not opt out. `USE_OLLAMA` is false in production anyway, -which makes `use_llm` a no-op there and the honest default obvious. +Turning both on for a single file is a considered trade. At twenty files it is +thousands of outbound requests and, for the image stage, a Playwright subprocess +that can burn three minutes on its own - on a single-vCPU container that is also +serving the API. So a batch opts IN to those stages; it does not opt out. +`USE_OLLAMA` is false in production anyway, which makes `use_llm` a no-op there +and the honest default obvious. + +Note that these defaults bind THIS router only. The open upload endpoint runs +itself and takes its two flags from `UPLOAD_AUTORUN_FETCH_IMAGES` and +`UPLOAD_AUTORUN_USE_LLM`, both true - see the section below. THE OTHER WAY INTO THE SAME PIPELINE ------------------------------------ @@ -54,9 +59,10 @@ from app.infrastructure.settings import ( router = APIRouter(prefix="/admin/catalog-batch", tags=["admin", "catalog"]) -# Per-file ceilings match store_catalog.py exactly. A file that is too big for -# the single-file endpoint is not somehow acceptable because it arrived with -# four friends. +# Per-file ceilings. They bound ONE spreadsheet, and they hold whether it +# arrived alone or with nineteen friends: a file too big to parse safely does +# not become acceptable by being part of a batch. The batch-wide ceilings +# (BATCH_MAX_*, imported above) then bound the set on top of these. MAX_UPLOAD_BYTES = 10 * 1024 * 1024 MAX_UPLOAD_ROWS = 2000 PREVIEW_ROWS = 10 diff --git a/app/core/batch_ingest.py b/app/core/batch_ingest.py index aba8e9b..904cb3a 100644 --- a/app/core/batch_ingest.py +++ b/app/core/batch_ingest.py @@ -15,10 +15,11 @@ file. What is new is everything *around* a file: WHY THE FILES GO TO DISK ------------------------ -The single-file path reads the upload into memory and hands the bytes to a -daemon thread (store_catalog.py). That is fine for one file and one operator -watching it: if the process dies, they re-upload. A five-file batch is a -different proposition - the colleague who sent them is not sitting there, and +The obvious cheap design - read the upload into memory and hand the bytes to a +daemon thread - is fine for one file and one operator watching it: if the +process dies, they re-upload. (An earlier single-file admin route did exactly +that; it has since been removed.) A five-file batch is a different proposition +- the colleague who sent them is not sitting there, and silently losing the drop is worse than any amount of extra code. So the bytes are staged under BATCH_UPLOAD_DIR, which is on the container's declared volume, and a manifest records what state each file reached. diff --git a/app/infrastructure/settings.py b/app/infrastructure/settings.py index 16b7b1b..e80dfc4 100644 --- a/app/infrastructure/settings.py +++ b/app/infrastructure/settings.py @@ -148,8 +148,12 @@ MODEL_ARTIFACTS_DIR = _dir( # endpoints - whatever Traefik defaults to is, and it is not ours to rely on. BATCH_UPLOAD_DIR = _dir("BATCH_UPLOAD_DIR", DATA_DIR / "batch_uploads") -# Per-file limits stay at the single-upload values (10MB / 2000 rows, see -# app/api/routers/store_catalog.py); these bound the BATCH on top of that. +# Per-file limits stay at the single-upload values, 10MB / 2000 rows. They are +# NOT settings and are not read from here: each router declares its own +# MAX_UPLOAD_BYTES / MAX_UPLOAD_ROWS pair with the same two numbers - see +# routers/batch_catalog.py, routers/uploads.py and routers/user_products.py. +# Change one and you have changed one. The ceilings below bound the BATCH on +# top of whichever per-file pair applied. BATCH_MAX_FILES = int(os.getenv("BATCH_MAX_FILES", "20")) BATCH_MAX_TOTAL_BYTES = int(os.getenv("BATCH_MAX_TOTAL_BYTES", str(50 * 1024 * 1024))) BATCH_MAX_TOTAL_ROWS = int(os.getenv("BATCH_MAX_TOTAL_ROWS", "20000"))