From f6a180e5dfcb9b4de7d799acb2d961207b3da54f Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Thu, 14 May 2026 00:39:29 +0300 Subject: [PATCH] [AZ-340] [AZ-527] Archive AZ-340 + batch 51 report + cumulative review 49-51 Bookkeeping for batch 51 close: - Archive AZ-340 spec todo/ -> done/ - Add _docs/03_implementation/batch_51_cycle1_report.md - Add _docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md Verdict: PASS_WITH_WARNINGS. F1 (Medium) escalates the 2-way _assert_engine_output_dim near-duplicate from cumulative-46-48 to a 7-way duplication after AZ-339 + AZ-340; new hygiene PBI AZ-527 formally created. F2 (Low) carries the AC-10 ConfigError vs literal ConfigurationError spec drift (documentation only). - File AZ-527 hygiene PBI (Hygiene -- consolidate _assert_engine_output_dim into a c2-internal helper, 2pt, AZ-255 E-C2). Add the spec stub at _docs/02_tasks/todo/AZ-527_*.md. - Refresh _docs/02_tasks/_dependencies_table.md: +AZ-527 row, totals bumped to 148 tasks / 491 points. - Bump _docs/_autodev_state.md: last_completed_batch=51, last_cumulative_review=batches_49-51. Co-authored-by: Cursor --- _docs/02_tasks/_dependencies_table.md | 7 +- .../AZ-340_c2_selavpr_eigenplaces_salad.md | 0 ...iene_engine_dim_assertion_consolidation.md | 120 ++++++++++++++++ .../batch_51_cycle1_report.md | 64 +++++++++ ...tive_review_batches_49-51_cycle1_report.md | 130 ++++++++++++++++++ _docs/_autodev_state.md | 4 +- 6 files changed, 320 insertions(+), 5 deletions(-) rename _docs/02_tasks/{todo => done}/AZ-340_c2_selavpr_eigenplaces_salad.md (100%) create mode 100644 _docs/02_tasks/todo/AZ-527_hygiene_engine_dim_assertion_consolidation.md create mode 100644 _docs/03_implementation/batch_51_cycle1_report.md create mode 100644 _docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md diff --git a/_docs/02_tasks/_dependencies_table.md b/_docs/02_tasks/_dependencies_table.md index c891ca4..3849d14 100644 --- a/_docs/02_tasks/_dependencies_table.md +++ b/_docs/02_tasks/_dependencies_table.md @@ -1,8 +1,8 @@ # Dependencies Table -**Date**: 2026-05-13 (refreshed after Batch 48: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; earlier same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path) -**Total Tasks**: 147 (106 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene -**Total Complexity Points**: 489 (356 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt +**Date**: 2026-05-14 (refreshed after Batch 51: AZ-527 hygiene PBI added from cumulative review batches 49-51 F1; earlier 2026-05-13: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path) +**Total Tasks**: 148 (107 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene; AZ-527 = 2pt c2 engine-dim helper hygiene +**Total Complexity Points**: 491 (358 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt, AZ-527 = 2pt Dependencies columns list only the tracker-ID portion (descriptive tail text in each task spec is omitted here for table-readability). The @@ -159,6 +159,7 @@ are all declared and documented below under **Cycle Check**. | AZ-507 | Hygiene — align module-layout.md cross-component import rules with AZ-270 lint | 2 | AZ-263, AZ-270, AZ-321 | AZ-246 | | AZ-508 | Hygiene — consolidate `_iso_ts_now` helpers into `helpers/iso_timestamps.py` | 2 | AZ-263 | AZ-264 | | AZ-526 | Hygiene — add `iso_ts_from_clock(clock)` to `helpers/iso_timestamps.py` | 2 | AZ-508, AZ-398 | AZ-264 | +| AZ-527 | Hygiene — consolidate `_assert_engine_output_dim` into c2-internal helper | 2 | AZ-340 | AZ-255 | | AZ-523 | Batch 44 — C11 internal flight-state gate removal (SRP refactor; audit-trail; closed) | 3 | AZ-317, AZ-319, AZ-329 | AZ-251 | | AZ-524 | Batch 44 — C12 package rename: c12_operator_tooling → c12_operator_orchestrator (audit; closed)| 2 | AZ-263, AZ-326, AZ-327, AZ-328, AZ-329, AZ-330, AZ-489 | AZ-253 | diff --git a/_docs/02_tasks/todo/AZ-340_c2_selavpr_eigenplaces_salad.md b/_docs/02_tasks/done/AZ-340_c2_selavpr_eigenplaces_salad.md similarity index 100% rename from _docs/02_tasks/todo/AZ-340_c2_selavpr_eigenplaces_salad.md rename to _docs/02_tasks/done/AZ-340_c2_selavpr_eigenplaces_salad.md diff --git a/_docs/02_tasks/todo/AZ-527_hygiene_engine_dim_assertion_consolidation.md b/_docs/02_tasks/todo/AZ-527_hygiene_engine_dim_assertion_consolidation.md new file mode 100644 index 0000000..857de43 --- /dev/null +++ b/_docs/02_tasks/todo/AZ-527_hygiene_engine_dim_assertion_consolidation.md @@ -0,0 +1,120 @@ +# Hygiene — Consolidate `_assert_engine_output_dim` into a c2-internal helper + +**Task**: AZ-527_hygiene_engine_dim_assertion_consolidation +**Name**: c2_vpr engine output-dim assertion helper consolidation +**Description**: Replace the seven duplicated `_assert_engine_output_dim(...)` definitions across the c2_vpr secondary VPR strategy modules (`ultra_vpr.py`, `net_vlad.py`, `mega_loc.py`, `mix_vpr.py`, `sela_vpr.py`, `eigen_places.py`, `salad.py`) with a single c2-internal helper at `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py`. Closes cumulative review batches 49–51 Finding F1 (Medium / Maintainability). +**Complexity**: 2 points +**Dependencies**: AZ-340 (the trigger that escalated the duplication from 4-way to 7-way; all 7 callers now exist). All other 6 callers are already in `done/` (AZ-337, AZ-338, AZ-339). +**Component**: c2_vpr (epic AZ-255 / E-C2) +**Tracker**: AZ-527 +**Epic**: AZ-255 (E-C2) + +### Document Dependencies + +- `_docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md` § F1 — the finding being closed. +- `_docs/02_document/components/02_c2_vpr/description.md` § 6 — the c2_vpr component description (helper goes inside the c2_vpr boundary). +- `_docs/02_tasks/done/AZ-337_c2_ultra_vpr_strategy.md`, `AZ-338_c2_netvlad_strategy.md`, `AZ-339_c2_mega_loc_mix_vpr.md`, `AZ-340_c2_selavpr_eigenplaces_salad.md` — the four task-specs whose AC-6 inference-output-dim contracts must continue to pass unmodified. + +## Problem + +Seven c2_vpr secondary VPR strategy modules each define a private `_assert_engine_output_dim` helper with byte-identical bodies modulo a single integer constant: + +| File | `DESCRIPTOR_DIM` | +|------|------------------| +| `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py` (AZ-337, B47) | 512 | +| `src/gps_denied_onboard/components/c2_vpr/net_vlad.py` (AZ-338, B46) | 4096 | +| `src/gps_denied_onboard/components/c2_vpr/mega_loc.py` (AZ-339, B50) | 4096 | +| `src/gps_denied_onboard/components/c2_vpr/mix_vpr.py` (AZ-339, B50) | 8448 | +| `src/gps_denied_onboard/components/c2_vpr/sela_vpr.py` (AZ-340, B51) | 512 | +| `src/gps_denied_onboard/components/c2_vpr/eigen_places.py` (AZ-340, B51) | 2048 | +| `src/gps_denied_onboard/components/c2_vpr/salad.py` (AZ-340, B51) | 8448 | + +Each copy runs a zero-init dry-run inference probe at `create()` time, asserts the engine output dict carries an `"embedding"` key, and asserts the output ndarray shape is `(1, DESCRIPTOR_DIM)`. The error-message strings and the shape-check logic are byte-identical. Each copy carries an inline comment referencing this AZ-527 ticket. + +Cumulative review batches 46-48 originally flagged the duplication at 2-way (Low). Cumulative review batches 49-51 escalated to Medium when the duplication grew to 7-way after AZ-339 + AZ-340 land. This PBI closes the pattern before any future c2_vpr backbone (or hypothetical c2-side adjacent work) adds an 8th copy. + +## Outcome + +- A new c2-internal module `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py` exposes a single function `assert_engine_output_dim(inference_runtime, handle, preprocessor, descriptor_dim, *, output_key="embedding", input_key="input") -> None` parametrised by the dim and the key names. Raises `gps_denied_onboard.config.schema.ConfigError` on mismatch (preserving the existing error envelope). +- The seven local `_assert_engine_output_dim` definitions are deleted; consumers import the helper. Module-level callers preserve the local symbol via `from gps_denied_onboard.components.c2_vpr._engine_dim_assertion import assert_engine_output_dim` (with a thin one-liner wrapper if call-site stability is preferred). +- The seven inline `AZ-527 (planned)` comments are deleted (they will be obsolete). +- A new unit test `tests/unit/c2_vpr/test_az527_engine_dim_assertion.py` covers AC-1..AC-4 below. +- The existing AZ-337 / 338 / 339 / 340 AC-6 tests pass unmodified. + +## Scope + +### Included + +- Add `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py` with the helper function (signature above). Import surface: `gps_denied_onboard.config.schema.ConfigError` only — no other component imports. +- Migrate the seven c2_vpr strategy modules to import the helper. Delete the local definitions and the local `AZ-527 (planned)` comments. +- Add `tests/unit/c2_vpr/test_az527_engine_dim_assertion.py` with AC-1..AC-4 (success path with matching shape, wrong shape raises `ConfigError`, missing output key raises `ConfigError`, success with non-default `output_key` / `input_key`). +- Re-run the existing `tests/unit/c2_vpr/test_ultra_vpr.py`, `test_net_vlad.py`, `test_az339_mega_loc_mix_vpr.py`, `test_az340_sela_vpr_eigen_places_salad.py` AC-6 sub-tests **unmodified** to verify that the consolidation preserves behavior at every call site. +- Optionally add a one-line mention of the helper in `_docs/02_document/components/02_c2_vpr/description.md` § 6 (Internal helpers). + +### Excluded + +- Sharing the helper across other components (c3, c4, c5). C7 already has its own engine-shape assertions inside the runtime; this PBI is c2-internal only. +- Changing the `"embedding"` output key contract or the `"input"` input key contract (those are runtime/engine conventions enforced elsewhere). +- Refactoring the `BackbonePreprocessor` Protocol — the helper still calls `preprocessor.input_shape()` to size the dry-run probe. +- Hoisting the helper to `src/gps_denied_onboard/helpers/` — engine-output-shape contracts are a c2 internal concern, not a cross-component shared helper. + +## Acceptance Criteria + +**AC-1: Helper exists at the canonical path with the expected signature** +Given a fresh checkout +When `from gps_denied_onboard.components.c2_vpr._engine_dim_assertion import assert_engine_output_dim` is run +Then the import succeeds; the function signature is `(inference_runtime, handle, preprocessor, descriptor_dim, *, output_key="embedding", input_key="input")` and the function returns `None` on success + +**AC-2: Wrong shape raises `ConfigError` with the expected envelope** +Given a fake `InferenceRuntime` whose `infer()` returns `{"embedding": np.zeros((1, 999), dtype=np.float16)}` for any of the 7 backbones +When `assert_engine_output_dim(..., descriptor_dim=512)` is called +Then a `ConfigError` is raised; the message contains both the expected dim (`512`) and the actual dim (`999`); the error type matches the contract `gps_denied_onboard.config.schema.ConfigError` + +**AC-3: Missing output key raises `ConfigError`** +Given a fake `InferenceRuntime` whose `infer()` returns `{"wrong_key": np.zeros((1, 512), dtype=np.float16)}` +When `assert_engine_output_dim(..., descriptor_dim=512)` is called +Then a `ConfigError` is raised; the message names the missing `output_key` + +**AC-4: All seven local `_assert_engine_output_dim` definitions are removed** +Given a `grep -rn "def _assert_engine_output_dim\|def assert_engine_output_dim" src/` after the task lands +When the search runs +Then matches appear only inside `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py`; zero matches in `ultra_vpr.py`, `net_vlad.py`, `mega_loc.py`, `mix_vpr.py`, `sela_vpr.py`, `eigen_places.py`, `salad.py` + +**AC-5: All AZ-337 / 338 / 339 / 340 AC-6 sub-tests pass unmodified** +Given the existing `tests/unit/c2_vpr/test_ultra_vpr.py`, `test_net_vlad.py`, `test_az339_mega_loc_mix_vpr.py`, `test_az340_sela_vpr_eigen_places_salad.py` +When the suites run after this task +Then every previously-passing AC-6 (engine output-dim) sub-test still passes; no test file is modified + +**AC-6: AZ-270 + AZ-507 layer lints pass** +Given the helper lives inside the c2_vpr component (Layer 3) and only imports from `gps_denied_onboard.config.schema` (Layer 1) and stdlib +When `tests/unit/test_az270_compose_root.py::test_ac6_only_compose_root_imports_concrete_strategies` runs +Then the test passes (helper is c2-internal, not a Public API surface, and respects layer direction) + +## Constraints + +- The helper module is c2-internal (`_engine_dim_assertion.py` underscore prefix). No other component may import it. +- `descriptor_dim`, `output_key`, `input_key` are passed as parameters — the helper has zero module-level constants describing per-backbone conventions. +- Error envelope preserved: raise `gps_denied_onboard.config.schema.ConfigError` (NOT `ValueError`, NOT a custom new error type). Error messages must continue to name both the expected and actual descriptor dims, plus the missing key when applicable. +- The dry-run probe input shape comes from `preprocessor.input_shape()` (the `BackbonePreprocessor` Protocol method). The helper does NOT hardcode any input shape — backbone-specific input shapes remain a per-strategy concern. +- Test additions go into a new `test_az527_engine_dim_assertion.py`; AZ-337 / 338 / 339 / 340 test files are NOT touched. + +## Risks & Mitigation + +**Risk 1: A 7-way refactor accidentally drops one call site, breaking `create()` for one strategy** +- *Risk*: One of the 7 modules forgets to call the helper or calls it with the wrong dim. +- *Mitigation*: AC-5 (all 4 existing AC-6 test files pass unmodified) is the safety net — if any strategy regresses, its AC-6 sub-test will fail. + +**Risk 2: A future emitter forgets the helper exists and adds an 8th local copy** +- *Risk*: The pattern recurs as new c2_vpr backbones land (none planned in current scope, but possible for hypothetical c2-side adjacent work). +- *Mitigation*: Add an AST-walk regression guard inside `test_az527_engine_dim_assertion.py` that asserts zero stray `_assert_engine_output_dim` / `assert_engine_output_dim` definitions outside the helper module — modeled on the AZ-508 / AZ-526 `test_no_local_iso_ts_*_definitions_remain` pattern. + +**Risk 3: The helper accidentally becomes a cross-component shared helper** +- *Risk*: A future C3/C4/C5 strategy author imports the helper from c2_vpr and creates a cross-component coupling. +- *Mitigation*: The helper is named `_engine_dim_assertion.py` (underscore prefix) inside the c2_vpr/ component folder, not in `helpers/`. The AZ-507 layer lint forbids cross-component imports of c2_vpr internals. If a C3/C4/C5 needs the same shape, the right move is to factor a c2-or-helpers Layer-1 module — out of scope here. + +## Runtime Completeness + +- **Named capability**: a c2-internal helper `assert_engine_output_dim(inference_runtime, handle, preprocessor, descriptor_dim, *, output_key, input_key) -> None` that runs a zero-init dry-run inference and validates the output dict shape against the expected descriptor dim. +- **Production code that must exist**: real `assert_engine_output_dim` function in `_engine_dim_assertion.py`; real import + call in each of the 7 strategy modules. +- **Allowed external stubs**: none for production code. The unit test uses fake `InferenceRuntime` and fake `BackbonePreprocessor` Protocol implementations (matching the existing AZ-337..AZ-340 fakes). +- **Unacceptable substitutes**: keeping one or more local definitions "for parity"; raising `ValueError` instead of `ConfigError`; hoisting the helper to `helpers/`; changing the `"embedding"` / `"input"` key defaults to per-backbone names (the parameters are the right tool). diff --git a/_docs/03_implementation/batch_51_cycle1_report.md b/_docs/03_implementation/batch_51_cycle1_report.md new file mode 100644 index 0000000..59df7d9 --- /dev/null +++ b/_docs/03_implementation/batch_51_cycle1_report.md @@ -0,0 +1,64 @@ +# Batch 51 — Implementation Report (Cycle 1) + +**Tasks**: AZ-340 (C2 SelaVPR + EigenPlaces + SALAD Secondary Backbones — Research-only) +**Date**: 2026-05-14 +**Cycle**: 1 +**Status**: COMPLETE (review verdict: PASS_WITH_WARNINGS, two Low findings) + +## What was done + +Added three secondary `VprStrategy` implementations for IT-12 comparative-study: `SelaVprStrategy` (D=512, 224×224 input), `EigenPlacesStrategy` (D=2048, 480×480 input) and `SaladStrategy` (D=8448, 322×322 input — DINOv2-Large backbone, heaviest in the C2 family). All run via the C7 TensorRT runtime (or ONNX-Runtime fallback), apply ImageNet mean/std preprocessing + single-stage L2 normalisation, and delegate retrieval to `FaissBridge`. All three are gated OFF for airborne and operator-tooling per ADR-002 — `BUILD_VPR_SELAVPR` / `BUILD_VPR_EIGENPLACES` / `BUILD_VPR_SALAD` ON only for the research binary and replay-cli. + +### Files added (7) + +| File | Purpose | +|------|---------| +| `src/gps_denied_onboard/components/c2_vpr/sela_vpr.py` | `SelaVprStrategy` class + `create()` factory + `_assert_engine_output_dim` helper | +| `src/gps_denied_onboard/components/c2_vpr/_preprocessor_sela_vpr.py` | `SelaVprBackbonePreprocessor` (centre-crop + 224×224 resize + ImageNet normalise + FP16 NCHW) | +| `src/gps_denied_onboard/components/c2_vpr/eigen_places.py` | `EigenPlacesStrategy` class + `create()` factory + `_assert_engine_output_dim` helper | +| `src/gps_denied_onboard/components/c2_vpr/_preprocessor_eigen_places.py` | `EigenPlacesBackbonePreprocessor` (centre-crop + 480×480 resize + ImageNet normalise + FP16 NCHW) | +| `src/gps_denied_onboard/components/c2_vpr/salad.py` | `SaladStrategy` class + `create()` factory + `_assert_engine_output_dim` helper | +| `src/gps_denied_onboard/components/c2_vpr/_preprocessor_salad.py` | `SaladBackbonePreprocessor` (centre-crop + 322×322 resize + ImageNet normalise + FP16 NCHW) | +| `tests/unit/c2_vpr/test_az340_sela_vpr_eigen_places_salad.py` | 54 parametrised AC tests across all three strategies | + +### Files changed +- _None._ The composition-root factory (`runtime_root/vpr_factory.py`) was already wired for `sela_vpr`, `eigen_places`, and `salad` strategy names at AZ-336 land time — `_STRATEGY_TO_BUILD_FLAG` and `_STRATEGY_TO_MODULE` tables already include the rows. The `KNOWN_STRATEGIES` frozenset in `c2_vpr/config.py` already includes all three. The `module-layout.md` `Component: c2_vpr` § Internal list already names `sela_vpr.py`, `eigen_places.py`, and `salad.py` (pre-declared by AZ-336). No CMake change required — `BUILD_VPR_*` gating is environment-variable-based per `_is_build_flag_on` in `vpr_factory.py`. + +## AC coverage + +All 11 ACs verified per strategy via the parametrised test suite. See `_docs/03_implementation/reviews/batch_51_review.md` § Phase 2 for the AC ↔ test mapping table. + +| AC | Status | Notes | +|----|--------|-------| +| AC-1..AC-9 + AC-11 | PASS | Each AC parametrised over all three strategies (54 test cases total) | +| AC-10 | PASS with drift | Implementation raises `StrategyNotAvailableError` (env-flag OFF path) and `ConfigError` (runtime-label mismatch path); the spec literally names `ConfigurationError`. Mirrors the established AZ-337 / AZ-338 / AZ-339 precedent. Logged as Low finding F2. | + +## Test results + +- `tests/unit/c2_vpr/test_az340_sela_vpr_eigen_places_salad.py` — **54 / 54 PASS**. +- `tests/unit/c2_vpr/test_protocol_conformance.py` — **PASS** (auto-extends across all 7 strategies after AZ-340; the three new ones are picked up by the parametrised `_STRATEGY_MODULES` table without test changes — verified by the full c2_vpr/ run below). +- `tests/unit/c2_vpr/` (full directory: faiss_bridge + net_vlad + ultra_vpr + AZ-339 + AZ-340 + protocol_conformance) — **180 / 180 PASS**. +- `tests/unit/test_az508_iso_timestamps.py` — **18 / 18 PASS** (AZ-526 regression guard confirms no new `_iso_ts_from_clock` duplicates introduced by AZ-340). +- `tests/unit/test_az270_compose_root.py` — **8 / 8 PASS**. +- `ruff check` on all 7 new files — clean (one auto-fixable `RUF022 __all__ not sorted` in `_preprocessor_eigen_places.py` was caught and fixed before commit). + +## Architectural decisions + +1. **Single parametrised test file `test_az340_sela_vpr_eigen_places_salad.py`** — rather than three near-identical files mirroring `test_ultra_vpr.py` / `test_net_vlad.py`. The three strategies share byte-identical behavioural contracts (same Protocol, same FDR record kinds, same log kinds, same error envelope) and differ only on three values (`DESCRIPTOR_DIM`, `_BACKBONE_LABEL`, preprocessor `input_shape()`). A parametrised approach keeps any future drift visible at the assertion level and reduces the test surface from ~2300 lines (three copies of test_ultra_vpr.py) to ~700 lines. Same precedent as AZ-339. +2. **Preprocessor duplication preserved** (sela_vpr vs eigen_places vs salad vs mega_loc vs mix_vpr vs ultra_vpr vs net_vlad) — per `components/02_c2_vpr/description.md` § 6 and the task spec § Constraints. Each preprocessor owns its own input-shape constants so a future code drop can change a backbone's preprocessing without coupling other strategies' weights-versions. +3. **`_assert_engine_output_dim` duplicated, NOT extracted** — see Spec Drift / Review Finding F1. The cleaner path is the dedicated AZ-527 hygiene PBI (now scoped to consolidate 7 copies, not 4). +4. **`iso_ts_from_clock` imported from the AZ-526 helper from day 1** — none of the three new strategies introduces a local `_iso_ts_from_clock` body. The AZ-526 regression guard test confirms this. +5. **Runtime-label guard placed inside `create()`** (not in `__init__`) — runtime selection is a composition-time concern; once the strategy is constructed it's expected to work. Matches the UltraVPR / NetVLAD / MegaLoc / MixVPR precedent. +6. **SALAD's high embedding dim (8448) is non-negotiable at the strategy layer** — it's the architectural output of the SALAD aggregator over DINOv2-Large patch tokens. PCA-whitening for a smaller SALAD descriptor is an operator-side decision at corpus build time (C10), gated by a future `BUILD_VPR_SALAD_PCA` build flag (out of scope here). + +## Spec drift noted (carried into review F2) + +AZ-340 § AC-10 literally specifies `ConfigurationError` for the build-flag-OFF case. The existing AZ-336 composition-root factory raises `StrategyNotAvailableError` for this case (per its own contract and test coverage at `test_protocol_conformance.py`). The strategy module's own runtime-label guard raises `ConfigError` for the related "wrong C7 runtime" case. AZ-337 / AZ-338 / AZ-339 followed this same pattern; AZ-340 mirrors them. AC-10 wording should be amended in a future shared spec-pass touching AZ-337..AZ-340; no code change required. + +## Cumulative review obligation + +This batch closes the K=3 cumulative-review window for batches 49–51 (last cumulative review covered batches 46-48). The cumulative review for batches 49–51 runs immediately after this batch report lands, before batch 52 starts. + +## Follow-on PBI + +**AZ-527** (Hygiene — consolidate `_assert_engine_output_dim` into a c2-internal helper). 2 points. Now must consolidate **7** copies (was 4 before AZ-339, became 4 again after AZ-339, now 7 after AZ-340). Depends on AZ-340. To be created and prioritised by the cumulative review for batches 49-51 (about to run). diff --git a/_docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md b/_docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md new file mode 100644 index 0000000..0792759 --- /dev/null +++ b/_docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md @@ -0,0 +1,130 @@ +# Cumulative Code Review — Batches 49-51 (Cycle 1) + +**Date**: 2026-05-14 +**Range**: batches 49 (AZ-526 `iso_ts_from_clock` consolidation), 50 (AZ-339 MegaLoc + MixVPR), 51 (AZ-340 SelaVPR + EigenPlaces + SALAD) +**Compared against**: previous cumulative review batches 46-48 +**Verdict**: **PASS_WITH_WARNINGS** + +## Scope + +21 files changed across batches 49-51 (12 src/, 4 test/, 2 docs, 3 indirect — package init + deps table + module-layout). Cumulative review focuses on cross-batch concerns that per-batch reviews cannot catch: duplicate symbols introduced across different batches, architecture drift across the trio, contract drift between producer/consumer batches, and follow-through on prior cumulative findings. + +## Carry-over closure from cumulative review 46-48 + +| Prior finding | Status | Notes | +|---------------|--------|-------| +| F1 (Medium) — `_iso_ts_from_clock` duplicated 4-way | **CLOSED by AZ-526 / Batch 49** | All 4 local copies (`net_vlad.py`, `ultra_vpr.py`, `_faiss_bridge.py`, `operator_reloc_service.py`) now import `iso_ts_from_clock` from `helpers/iso_timestamps`. Regression guard: `tests/unit/test_az508_iso_timestamps.py::test_no_local_iso_ts_from_clock_definitions_remain` (AST-walk). | +| F2 (Low) — `_assert_engine_output_dim` near-duplicated between strategies | **OPEN, escalated to current F1** | Was 2-way at B47; now **7-way** after AZ-339 (B50, +2 copies) and AZ-340 (B51, +3 copies). Hygiene PBI **AZ-527** formally created (link below). | +| F3 (Low) — `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` | **CLOSED by AZ-526 / Batch 49** | The new `iso_ts_from_clock(clock)` helper emits the `Z` suffix. All 4 callers now produce the canonical byte-shape. Verified by `test_az526_iso_ts_from_clock_emits_z_suffix`. | + +## Findings (this window) + +| # | Severity | Category | File:Line | Title | +|---|----------|----------|-----------|-------| +| F1 | Medium | Maintainability | `c2_vpr/{ultra_vpr,net_vlad,mega_loc,mix_vpr,sela_vpr,eigen_places,salad}.py` | `_assert_engine_output_dim` duplicated 7-way across c2_vpr strategy modules — **AZ-527 created** | +| F2 | Low | Spec-Gap | `_docs/02_tasks/done/AZ-{337,338,339,340}*.md` § AC-10 | C2 strategy specs literally name `ConfigurationError`; implementation raises `StrategyNotAvailableError` (env-flag OFF) and `ConfigError` (runtime-label mismatch). Documentation drift; no code change required. | + +## Finding Details + +### F1: `_assert_engine_output_dim` duplicated 7-way (Medium / Maintainability) — AZ-527 created + +- **Locations**: + - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py` (AZ-337, B47) + - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py` (AZ-338, B46) + - `src/gps_denied_onboard/components/c2_vpr/mega_loc.py` (AZ-339, B50) + - `src/gps_denied_onboard/components/c2_vpr/mix_vpr.py` (AZ-339, B50) + - `src/gps_denied_onboard/components/c2_vpr/sela_vpr.py` (AZ-340, B51 — new) + - `src/gps_denied_onboard/components/c2_vpr/eigen_places.py` (AZ-340, B51 — new) + - `src/gps_denied_onboard/components/c2_vpr/salad.py` (AZ-340, B51 — new) + +- **Description**: Each copy is the same ~30-line helper that runs a zero-init dry-run inference, asserts the engine output dict has an `"embedding"` key, and asserts the output ndarray shape is `(1, DESCRIPTOR_DIM)`. The only thing that differs across copies is the local `DESCRIPTOR_DIM` constant (512 / 4096 / 4096 / 8448 / 512 / 2048 / 8448) and the local preprocessor type-hint. The error-message strings and the shape-check logic are byte-identical. + +- **Action taken**: Hygiene PBI **AZ-527** formally opened in Jira (`Hygiene — consolidate _assert_engine_output_dim into a c2-internal helper`, 2 points, Epic AZ-255 E-C2). Scope: add `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py` exposing `assert_engine_output_dim(...)` parametrised by `descriptor_dim`, `output_key`, `input_key`; replace the 7 local definitions with imports; add a focused unit test; re-run the existing AZ-337 / 338 / 339 / 340 AC-6 tests unmodified. Link: . + +- **Why escalated from Low (B46-48 cumulative) to Medium**: at 7-way the per-edit risk of one copy drifting from the others (different error-message wording, different shape-check operator order, different early-return semantics) is non-trivial. Each future c2_vpr secondary strategy would add an 8th, 9th copy unless we close the pattern now. + +### F2: AC-10 spec drift across the C2 strategy specs (Low / Spec-Gap) + +- **Locations**: + - `_docs/02_tasks/done/AZ-337_c2_ultra_vpr_strategy.md` § AC-10 + - `_docs/02_tasks/done/AZ-338_c2_netvlad_strategy.md` § AC-10 + - `_docs/02_tasks/done/AZ-339_c2_mega_loc_mix_vpr.md` § AC-10 + - `_docs/02_tasks/done/AZ-340_c2_selavpr_eigenplaces_salad.md` § AC-10 + +- **Description**: Each spec literally names `ConfigurationError` for the "build flag OFF" case and the "runtime-label mismatch" case. The implementation (correctly, by precedent) raises `StrategyNotAvailableError` for the env-flag-OFF case (composition-root concern) and `ConfigError` for the runtime-label-mismatch case (strategy-internal concern). All four AZ-337..AZ-340 implementations made the same choice; per-batch reviews documented this consistently. + +- **Suggestion**: amend the four AC-10 wordings in a single shared spec-pass (5 minutes' work) so future batches and audit trails do not have to re-explain the divergence. Sized at <1 point — could ride along with any future c2_vpr spec touch. Optionally fold into AZ-527 if it touches the spec wording for AC-6 anyway. + +- **Defer rationale**: pure documentation drift, no code defect, no missing test coverage. Filed as F2 here so it surfaces in the audit trail without blocking. + +## Phase Summary + +### Phase 1 — Context Loading + +- 3 task specs reviewed (AZ-526, AZ-339, AZ-340). +- 4 doc inputs: `architecture.md`, `module-layout.md`, the 3 per-batch reviews (`batch_49_review.md` / `batch_50_review.md` / `batch_51_review.md`), and `_docs/02_document/components/02_c2_vpr/description.md`. +- No `_docs/02_document/architecture_compliance_baseline.md` exists yet; the **Baseline Delta** section is omitted from this report (consistent with prior cumulative reviews). + +### Phase 2 — Spec Compliance + +Per-batch reviews covered AC-by-AC compliance for each task. Cross-batch checks: + +- **AZ-339 + AZ-340 vs AZ-337 + AZ-338 pattern parity**: the five new strategies (MegaLoc, MixVPR, SelaVPR, EigenPlaces, SALAD) line up with UltraVPR's structural skeleton — `create()` factory + `__init__` + `embed_query` + `retrieve_topk` + private `_assert_engine_output_dim` + `_emit_*` FDR helpers. NetVLAD remains the lone outlier (PyTorch architecture-factory path). All five new strategies use the TRT-only path identical to UltraVPR. No accidental drift onto the NetVLAD-only intra-cluster + L2 path. +- **`_STRATEGY_TO_BUILD_FLAG` / `_STRATEGY_TO_MODULE` pre-wiring** in `runtime_root/vpr_factory.py`: the 5 rows for `mega_loc`, `mix_vpr`, `sela_vpr`, `eigen_places`, `salad` were added at AZ-336 land time and remain untouched by AZ-339 / AZ-340 — confirmed by `git log --diff-filter=M -- src/gps_denied_onboard/runtime_root/vpr_factory.py` (no commits in this window). Strategy modules now actually exist for all 5 entries — no more silent ImportError surprises if a downstream test toggles a build flag ON. +- **AC-10 spec wording drift** captured as F2 above. + +### Phase 3 — Code Quality + +- **SRP**: each new strategy and each new preprocessor has a single clear concern. Helpers separated into `_preprocessor_.py` siblings. +- **Error handling**: typed exception envelopes (`ConfigError`, `VprBackboneError`, `VprPreprocessError`, `StrategyNotAvailableError`) used consistently across all 5 new strategies; no bare `except`. +- **Naming**: aligned across all 7 c2_vpr secondary strategies; method shapes for `embed_query` / `retrieve_topk` / `_emit_*` mirror line-for-line. +- **Test quality**: AZ-339 and AZ-340 both adopted the parametrised single-file pattern (driven by `_STRATEGY_MODULES` table) instead of duplicating per-strategy test files. Reduces test surface from ~2300 lines (3 copies of `test_ultra_vpr.py`) to ~700 lines for AZ-340 with strictly equivalent AC coverage. Same precedent as the AZ-339 review noted. +- **Test-environment-mirrors-prod gate**: still satisfied — fakes mirror the production interfaces (`InferenceRuntimeCut`, `DescriptorIndexCut`, `BackbonePreprocessor` Protocols) and never live entirely in test files. + +### Phase 4 — Security Quick-Scan + +No SQL, no `subprocess`, no `eval` / `exec`, no secrets. New code is pure-Python numerical wiring + FDR record emission. n/a. + +### Phase 5 — Performance Scan + +- All 5 new strategies use FP16 inputs and pre-compiled TensorRT engines — consistent with the project's perf budget. +- Engine load + output-shape assertion happens at `create()` time (startup) not first-frame — explicit AZ-337 architectural decision; preserved across AZ-339 / AZ-340. +- SALAD's higher D=8448 will produce a larger FAISS index footprint — operator-side concern at corpus build time; flagged in the AZ-340 batch report § Architectural Decisions. +- No obvious hot-path allocations beyond what numpy / FAISS already do. + +### Phase 6 — Cross-Task Consistency + +- **Pattern parity across all 7 c2_vpr secondary strategies**: confirmed — same skeleton, same FDR record kinds, same log kinds, same error envelope. The duplication F1 is the cost of this parity until AZ-527 lands. +- **`iso_ts_from_clock` import consistency**: all c2_vpr strategy modules now import `iso_ts_from_clock` from `gps_denied_onboard.helpers.iso_timestamps` — verified by AZ-526 regression guard. +- **`intra_cluster_normalise` helper**: still used ONLY by NetVLAD as intended. Verified by spy tests in `test_net_vlad.py` and absence of import in the 6 other strategies. +- **FDR record schema**: no schema-level additions in this window beyond the records the existing strategies already emit. The 5 new strategies emit the same record kinds (`vpr.embedding_complete`, `vpr.retrieval_complete`, `vpr.query_failed`) as UltraVPR / NetVLAD / MegaLoc / MixVPR — verified by `tests/unit/test_az272_fdr_record_schema.py` (passes unmodified). +- **AZ-526 closure** of cumulative-46-48 F1 + F3: verified — zero `_iso_ts_from_clock` definitions remain anywhere under `src/`. + +### Phase 7 — Architecture Compliance + +1. **Layer direction**: c2_vpr (Layer 3) imports from `_types` (Layer 1), `helpers/*` (Layer 1), `config` (Layer 1), `logging` (Layer 1), `fdr_client` (Layer 1), `clock` (Layer 1). All downward. No upward imports detected in any of the 5 new strategy or 5 new preprocessor modules. +2. **Public API respect**: all c2_vpr → c6 / c7 communication continues to go through the structural Protocol cuts (`InferenceRuntimeCut`, `DescriptorIndexCut`). Verified by `test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies` (passes — 8/8). +3. **No new cyclic deps**: built-import graph of changed files + direct dependencies — no cycles introduced. +4. **Duplicate symbols across components**: F1 above (intra-component duplication, scoped to `c2_vpr/*.py`). No cross-component duplication detected. +5. **Cross-cutting concern not locally re-implemented**: AZ-526 correctly hoisted `_iso_ts_from_clock` out of per-component locals. The remaining open pattern is `_assert_engine_output_dim` — but this is intentionally c2-internal (engine output-shape contract is a c2 concern; C7 has its own engine-shape assertions for its own purposes inside the runtime). AZ-527 keeps it inside c2_vpr/, not in shared/helpers. +6. **Preprocessor duplication across the 7 backbones**: per `components/02_c2_vpr/description.md` § 6 and the AZ-339 / AZ-340 task specs § Constraints, this is **intentional and architectural** — each backbone owns its own input-shape constants (224×224, 322×322, 384×384, 480×480, 512×512) so a future model-weights drop can change a backbone's preprocessing without coupling other strategies' weights-versions. **Not a finding.** + +## Verdict Justification + +- Critical findings: 0 +- High findings: 0 +- Medium findings: 1 (F1 — already addressed via AZ-527 ticket creation) +- Low findings: 1 (F2 — pure documentation drift) + +**PASS_WITH_WARNINGS** — auto-chain to next batch is allowed per implement skill Step 14.5. The F1 finding has an open Jira ticket (AZ-527) ready to be sequenced into a future batch; F2 is documentation-only and does not block. + +## Recommended Follow-up + +1. **Sequence AZ-527 before any future c2_vpr secondary backbone (none planned in current scope, but defensively before any AZ-358 / AZ-389 / AZ-358 c2-side adjacent work).** 2 points, 1 new module + 7 small edits + 1 test. +2. **F2 spec-pass** (~5 minutes, <1 point): fold the AC-10 wording correction into AZ-527's spec touch if it lands first; otherwise file as a tiny standalone hygiene PBI. +3. **No other carry-over.** All previous cumulative findings are now either CLOSED or have a tracked follow-up. + +## Next Cumulative Review + +- **Trigger**: after Batch 54 (every K=3). +- **Range**: batches 52-54. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 615aeac..c62c667 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -12,5 +12,5 @@ sub_step: retry_count: 0 cycle: 1 tracker: jira -last_completed_batch: 50 -last_cumulative_review: batches_46-48 +last_completed_batch: 51 +last_cumulative_review: batches_49-51