From 399ae86c1fc2a5d528ab6d65f37781d32a99f0e7 Mon Sep 17 00:00:00 2001 From: Aaron Po Date: Mon, 31 Aug 2026 22:03:19 -0400 Subject: [PATCH] Document the pipeline's OpenAI generator and reverse-geocoding subsystem, and fix stale table/copy-list claims --- docs/pipeline/ETHICS-AND-KNOWN-ISSUES.md | 19 +++++++++++---- docs/pipeline/README.md | 31 +++++++++++++++++++----- docs/pipeline/ROADMAP.md | 23 ++++++++++++------ 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/docs/pipeline/ETHICS-AND-KNOWN-ISSUES.md b/docs/pipeline/ETHICS-AND-KNOWN-ISSUES.md index 1bc5b13a..92ff1c84 100644 --- a/docs/pipeline/ETHICS-AND-KNOWN-ISSUES.md +++ b/docs/pipeline/ETHICS-AND-KNOWN-ISSUES.md @@ -299,11 +299,20 @@ Each brewery and user address carries a longitude/latitude pair, sampled uniformly at random within a 5 km disc centred on the city's curated `latitude`/`longitude` from `locations.json`. -The resulting point is **plausible by construction**, always falling within the -city's vicinity, but it is not looked up from any real address registry. It does -not correspond to a real address, business, or resident, and should not be -treated as accurate geocoding data. This is consistent with the rest of the -dataset: fixture data for a proof-of-concept, not a source of truth. +The coordinate pair itself is **plausible by construction**, always falling +within the city's vicinity, but does not correspond to a real business or +resident, and should not be treated as accurate geocoding data. This is +consistent with the rest of the dataset: fixture data for a proof-of-concept, +not a source of truth. + +For brewery addresses specifically (not user addresses, which carry no street +address at all), `address_line1`/`postal_code` *are* looked up from a real +address registry — `NominatimAddressService` reverse-geocodes the synthetic +coordinate pair against the live OpenStreetMap/Nominatim database, so the +street address is a genuine nearby address, just not the address of an actual +brewery. This lookup only happens for non-`--mocked` runs; `--mocked` runs use +`MockAddressService`'s fixed placeholder address instead, which is not looked +up from any real registry. --- diff --git a/docs/pipeline/README.md b/docs/pipeline/README.md index a8ce4af9..9fa7db6d 100644 --- a/docs/pipeline/README.md +++ b/docs/pipeline/README.md @@ -113,7 +113,9 @@ Each run writes a fresh dated SQLite file such as | Flag | Purpose | | ------------------ | ------------------------------------------------------------------------------------------------------------ | | `--mocked` | Deterministic mock generator, no model required. | -| `--model, -m` | Path to a GGUF file. Required unless `--mocked` is set. | +| `--model, -m` | Path to a GGUF file. Selects the local Llama.cpp generator. | +| `--openai` | Use the OpenAI API (Chat Completions) for brewery/user data. Requires the `OPENAI_API_KEY` environment variable. | +| `--openai-model` | OpenAI model ID used when `--openai` is set. Default: `gpt-4o-mini`. | | `--prompt-dir` | Directory containing prompt files (for example, `BREWERY_GENERATION.md`). Required unless `--mocked` is set. | | `--output, -o` | Directory for generated SQLite artifacts. Default: `output`. | | `--log-path` | Path for application logs. Default: `pipeline.log`. | @@ -126,9 +128,9 @@ Each run writes a fresh dated SQLite file such as | `--n-gpu-layers` | Number of model layers to offload to GPU. Default: `0`. | | `--help, -h` | Print usage and exit. | -`--mocked` and `--model` are mutually exclusive. Omitting both exits with an -error before the pipeline starts. Sampling flags are ignored when `--mocked` is -set. +`--mocked`, `--model`, and `--openai` are mutually exclusive. Omitting all +three exits with an error before the pipeline starts. Sampling flags are +ignored unless `--model` is set (Llama). The post-build step copies `prompts/` into `build/prompts/`. Rebuild after editing any prompt file. @@ -236,8 +238,8 @@ to match your run. | Sample | `BiergartenPipelineOrchestrator::QueryCitiesWithCountries()` samples `--location-count` locations per run (default `10`). | | Enrich | `WikipediaEnrichmentService` fetches brewing and beer-related context. Keeps going when a lookup fails. `--mocked` runs use `MockEnrichmentService` instead and skip Wikipedia entirely. | | Generate Users | `GenerateUsers()` samples a persona and a forename/surname pair per enriched city (skipping countries with no name data), then `MockGenerator` or `LlamaGenerator` produces a username, bio, and activity weight around the sampled name. | -| Generate Breweries | `MockGenerator` or `LlamaGenerator` produces brewery names and descriptions in English and the local language. | -| Store | `SqliteExportService` writes each successful user and brewery into a fresh dated `.sqlite` database with normalized `locations`, `users`, and `breweries` tables. | +| Generate Breweries | `MockGenerator`, `LlamaGenerator`, or `OpenAIGenerator` produces brewery names and descriptions in English and the local language, then `IAddressService::ReverseGeocode()` resolves a street address for the brewery's jittered coordinates. | +| Store | `SqliteExportService` writes each successful user and brewery into a fresh dated `.sqlite` database with normalized `cities`, `breweries`, `brewery_addresses`, `users`, and `user_addresses` tables. | | Log | `spdlog` writes results and warnings to the console. | If name sampling, enrichment, or generation fails for a city, that city is @@ -263,6 +265,15 @@ skipped and the pipeline continues. `GenerateUsers()` runs before across attempts; it is not raised automatically on truncation. - `MockGenerator`: stable hash-based output so the same city/persona/name input always produces the same brewery or user. +- `NominatimAddressService`: reverse-geocodes each brewery's jittered + coordinates into a street address (`address_line1`, `postal_code`) via the + public [Nominatim API](https://nominatim.openstreetmap.org), respecting its + one-request-per-second usage policy. Falls back to a placeholder + `address_line1` ("Address unavailable") and empty `postal_code` when the + lookup fails or returns no usable street address. `MockAddressService` is + the no-network substitute (`"123 Mock Street"`, `"00000"`) used in + `--mocked` runs. Only breweries get a resolved street address; generated + users carry a coordinate pair with no address fields. - `SqliteExportService`: creates a dated SQLite file per run and persists each successful user and brewery into normalized tables. - Brewery payloads include English and local-language name and description @@ -288,6 +299,12 @@ valid output on the first pass; the retry path is kept for resilience. `MockGenerator` uses stable hashes for repeatable output in demos and Storybook runs. +`NominatimAddressService.ReverseGeocode()` sleeps one second after every +request to respect Nominatim's usage policy, and returns `std::nullopt` (logged +as a warning, not a skipped city) if the request fails, the response has no +`address` object, or neither `house_number` nor `road` is present — the +brewery is still generated and exported, just with a placeholder address. + `CuratedJsonDataService` memoizes each of `LoadLocations()`, `LoadPersonas()`, `LoadForenamesByCountry()`, and `LoadSurnamesByCountry()` independently the first time each is called, since `BiergartenPipelineOrchestrator` owns a single @@ -323,6 +340,8 @@ SQLite export file named with the current UTC timestamp. | `description_en` | Brewery description in English. | | `name_local` | Brewery name in the local language. | | `description_local` | Brewery description in the local language. | +| `address_line1` | Street address resolved via reverse geocoding, or a placeholder if resolution failed. | +| `postal_code` | Postal code resolved via reverse geocoding, or empty if resolution failed. | | Field | Meaning | | ----------------- | -------------------------------------------------------------------------------------- | diff --git a/docs/pipeline/ROADMAP.md b/docs/pipeline/ROADMAP.md index 858dd2b2..a4ec9a5f 100644 --- a/docs/pipeline/ROADMAP.md +++ b/docs/pipeline/ROADMAP.md @@ -10,8 +10,8 @@ tags: This is the engineering breakdown for closing the gap between the **current** pipeline and the **planned** architecture documented in -[`diagrams/planned/class.puml`](./diagrams/planned/class.puml) and -[`diagrams/planned/activity.puml`](./diagrams/planned/activity.puml). Nothing in +[`diagrams/planned/class.md`](./diagrams/planned/class.md) and +[`diagrams/planned/activity.md`](./diagrams/planned/activity.md). Nothing in `diagrams/planned/` is implemented yet — this file tracks what it would take to get there. For the current implementation, see [README.md](./README.md). @@ -105,7 +105,8 @@ architecture needs. is already copied into the Docker image (`runpod/Dockerfile`), but no loader reads it yet, and the native CMake build doesn't copy it into `build/` at all — `CMakeLists.txt`'s "Runtime Assets" step only copies - `locations.json` and `prompts/`. + `locations.json`, `personas.json`, `forenames-by-country.json`, + `surnames-by-country.json`, and `prompts/`. - [x] Add `LoadPersonas()` and author `personas.json` (doesn't exist yet). - [x] Add name-by-country loading, parsing `tooling/pipeline/forenames-by-country.json` and @@ -150,6 +151,12 @@ Entirely new — no `includes/policy/` (or equivalent) directory exists today. - [ ] Extend the `DataGenerator` interface with `GenerateBeer`, `GenerateCheckin`, `GenerateRating` (today: only `GenerateBrewery` and `GenerateUser`). +- [x] Add `OpenAIGenerator`, a third `DataGenerator` backend (alongside + `MockGenerator`/`LlamaGenerator`) that calls the OpenAI Chat Completions + API with Structured Outputs (`json_schema`, `strict`) to guarantee + schema-valid JSON without needing `IPromptFormatter` or a GBNF grammar — + selected via `--openai`/`GeneratorMode::kOpenAI`. This landed ahead of + the planned architecture in §3/§4 and isn't reflected there yet. - [x] Implement `LlamaGenerator::GenerateUser` for real, with a retry loop mirroring `GenerateBrewery` (GBNF grammar, `ValidateUserJson`, up to 3 attempts with corrective feedback) and a new `prompts/USER_GENERATION.md`. @@ -181,12 +188,14 @@ Entirely new — no `includes/policy/` (or equivalent) directory exists today. `ProcessRecord(const BreweryRecord&)` and `ProcessRecord(const UserRecord&)` exist). - [ ] Add `beers`, `checkins`, `ratings`, `follows` tables to - `SqliteExportService::InitializeSchema()` (`locations`, `breweries`, and - `users` already exist) — see `kCreateLocationsTableSql` / - `kCreateBreweriesTableSql` / `kCreateUsersTableSql` in + `SqliteExportService::InitializeSchema()` (`cities`, `breweries`, + `brewery_addresses`, `users`, and `user_addresses` already exist) — see + `kCreateCitiesTableSql` / `kCreateBreweriesTableSql` / + `kCreateBreweryAddressesTableSql` / `kCreateUsersTableSql` / + `kCreateUserAddressesTableSql` in `includes/services/database/sqlite_statement_helpers.h` for the existing pattern to follow. -- [ ] Add a `brewery_cache_` alongside the existing `location_cache_`, and move +- [ ] Add a `brewery_cache_` alongside the existing `city_cache_`, and move from one open transaction for the whole run to per-phase batched commits (`BEGIN` / `COMMIT & BEGIN` on a batch-size threshold), as shown in the planned activity diagram.