azaion/gps-denied-onboard
1
0
Fork 0
mirror of https://github.com/azaion/gps-denied-onboard.git synced 2026-06-21 22:31:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-340] [AZ-527] Archive AZ-340 + batch 51 report + cumulative review 49-51

Browse Source 
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 <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-14 00:39:29 +03:00
parent 87909cce9f
commit f6a180e5df
6 changed files with 320 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/batch_51_cycle1_report.md
+64
View File
@@ -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).
_docs/03_implementation/cumulative_review_batches_49-51_cycle1_report.md
+130
View File
@@ -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: <https://denyspopov.atlassian.net/browse/AZ-527>.
- **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_<backbone>.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.