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

[AZ-527] Consolidate _assert_engine_output_dim into c2-internal helper

Browse Source 
Closes cumulative review batches 49-51 Finding F1 (Medium /
Maintainability) -- the 7-way duplication of _assert_engine_output_dim
across c2_vpr secondary VPR strategy modules.

Add c2-internal helper assert_engine_output_dim(inference_runtime,
handle, preprocessor, descriptor_dim, *, output_key='embedding',
input_key='input') in src/gps_denied_onboard/components/c2_vpr/
_engine_dim_assertion.py. The helper runs a zero-init dry-run
inference at preprocessor.input_shape() and asserts the engine output
dict carries (1, descriptor_dim) under output_key. Raises
gps_denied_onboard.config.schema.ConfigError on mismatch (preserving
the prior error envelope and message wording byte-identically).

Migrate 7 strategy modules (ultra_vpr, net_vlad, mega_loc, mix_vpr,
sela_vpr, eigen_places, salad) to import the helper and delete the
local _assert_engine_output_dim definitions + their inline
'AZ-527 (planned)' comments. NetVLAD is the only call site that
overrides output_key='vlad_descriptor'; the other 6 explicitly pass
output_key=_OUTPUT_KEY + input_key=_ENGINE_INPUT_KEY (matching helper
defaults but documenting strategy contract at the call site).

Add tests/unit/c2_vpr/test_az527_engine_dim_assertion.py (14 tests,
AAA pattern, Protocol-conforming fakes) covering AC-1..AC-4: helper
signature; wrong shape raises ConfigError naming both dims; missing
output key raises ConfigError naming the missing key; AST-walk
regression guard for stray definitions outside the helper module
(modeled on AZ-526's test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper);
import-grep regression guard verifying all 7 strategy modules import
the helper.

AC-5 (existing AZ-337/338/339/340 AC-6 sub-tests pass unmodified) is
exercised transitively: c2_vpr/ full directory 230/230 PASS, no test
file modified outside the new test_az527_*. AC-6 (AZ-270 + AZ-507
layer lints) verified by tests/unit/test_az270_compose_root.py
8/8 PASS.

Code-review verdict: PASS (zero findings). Ruff clean.

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-14 00:50:17 +03:00
parent f6a180e5df
commit 235eb4549e
10 changed files with 615 additions and 191 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/reviews/batch_52_review.md
+91
View File
@@ -0,0 +1,91 @@
# Code Review Report — Batch 52
**Batch**: 52 (AZ-527 — Hygiene: consolidate `_assert_engine_output_dim` into a c2-internal helper)
**Date**: 2026-05-14
**Verdict**: **PASS**
## Findings
_None._
## Phase Summary
### Phase 1 — Context Loading
Inputs read:
- `_docs/02_tasks/todo/AZ-527_hygiene_engine_dim_assertion_consolidation.md` — task spec.
- `_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 internal-helper boundary.
- `_docs/02_document/architecture.md` + `_docs/02_document/module-layout.md` — for layer direction + Public API rules.
- The 7 modified strategy modules (`ultra_vpr.py`, `net_vlad.py`, `mega_loc.py`, `mix_vpr.py`, `sela_vpr.py`, `eigen_places.py`, `salad.py`).
- The new `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py`.
- The new `tests/unit/c2_vpr/test_az527_engine_dim_assertion.py`.
### Phase 2 — Spec Compliance
| AC | Status | Evidence |
|----|--------|----------|
| AC-1 (helper exists with expected signature) | PASS | `test_ac1_helper_callable_with_default_keys`. The signature is `(inference_runtime, handle, preprocessor, descriptor_dim, *, output_key="embedding", input_key="input") -> None`. |
| AC-2 (wrong shape raises `ConfigError` with both dims named) | PASS | `test_ac2_wrong_descriptor_width_raises_config_error` (3 parametrised) + `test_ac2_wrong_ndim_or_batch_raises_config_error` (4 parametrised). Error envelope is `gps_denied_onboard.config.schema.ConfigError`. |
| AC-3 (missing output key raises `ConfigError` naming the missing key) | PASS | `test_ac3_missing_default_output_key_raises_config_error` + `test_ac3_missing_overridden_output_key_raises_config_error`. Both default `"embedding"` and NetVLAD override `"vlad_descriptor"` paths covered. |
| AC-4 (no stray local definitions remain) | PASS | `test_ac4_no_stray_engine_dim_assertion_definitions_outside_helper` (AST walk, modeled on AZ-526's `test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper`) + `test_ac4_seven_strategy_modules_import_the_helper` (import grep on each of the 7 strategy modules). |
| AC-5 (AZ-337/338/339/340 AC-6 tests pass unmodified) | PASS | Full c2_vpr test directory: **230 / 230 PASS** (no test file modified outside the new `test_az527_*`). |
| AC-6 (AZ-270 + AZ-507 layer lints pass) | PASS | `tests/unit/test_az270_compose_root.py` + `tests/unit/test_az508_iso_timestamps.py` (combined): **26 / 26 PASS**. Helper is `_engine_dim_assertion.py` (underscore = c2-internal), not in the strategy resolution table. |
Constraints (from § Constraints): all satisfied.
- Helper module is c2-internal (`_engine_dim_assertion.py` underscore prefix; not exported via `c2_vpr/__init__.py`).
- `descriptor_dim` / `output_key` / `input_key` are parameters; helper has zero per-backbone module constants.
- Error envelope preserved: `gps_denied_onboard.config.schema.ConfigError` (NOT `ValueError`, NOT a custom new error type).
- Probe input shape comes from `preprocessor.input_shape()` (covered by `test_helper_propagates_preprocessor_input_shape_to_probe_tensor`).
- Test additions go into new `test_az527_engine_dim_assertion.py`; the AZ-337/338/339/340 test files are untouched.
### Phase 3 — Code Quality
- **SRP**: helper has a single concern — verify the loaded engine emits `(1, descriptor_dim)` under `output_key`. Nothing else.
- **Error handling**: raises typed `ConfigError` with full context (expected + actual dims; missing key name); no bare `except`; no silent suppression.
- **Naming**: `assert_engine_output_dim`, `descriptor_dim`, `output_key`, `input_key` — all precise, no vague terms.
- **DRY**: 7 copies → 1 source of truth (the entire point of this PBI).
- **Test quality**: Arrange / Act / Assert pattern; parametrised over the realistic dim values (512 / 2048 / 8448) + degenerate shapes (`(512,)`, `(2,1,512)`, `(1,1,512)`, `(1,512,1)`); regression guards via AST walk + import grep.
- **Dead code**: 7 local `_assert_engine_output_dim` definitions + their inline `AZ-527 (planned)` comments deleted. `EngineHandle` and `InferenceRuntimeCut` imports remain referenced elsewhere in each strategy (verified by ripgrep).
- **Comment hygiene**: helper docstring describes intent + parameters + raises; no narrating comments. Test file uses minimal section headers per the existing test_az340_* / test_az526_* precedent.
- **Annotation form**: `from __future__ import annotations` + bare type names (no quoted strings) — Ruff `UP037` clean.
### Phase 4 — Security Quick-Scan
No SQL, no `subprocess`, no `eval` / `exec`, no secrets. The helper is pure-Python numerical wiring (allocate a zero-init `(1,3,h,w)` ndarray, call `infer()`, assert dict shape). n/a.
### Phase 5 — Performance Scan
- The helper allocates one zero-init `(1, 3, H, W)` `np.float16` ndarray and runs one `infer()` call. Both happen at `create()` time (startup), NOT per frame.
- No hot-path impact. The migration replaced 7 byte-identical helpers with 1 — wall-clock startup time is unchanged within measurement noise.
### Phase 6 — Cross-Task Consistency
- All 7 strategy modules consistently route through the helper. The 6 default-key backbones (UltraVPR / MegaLoc / MixVPR / SelaVPR / EigenPlaces / SALAD) explicitly pass `output_key=_OUTPUT_KEY` + `input_key=_ENGINE_INPUT_KEY` (matching the helper defaults but making the strategy contract explicit at the call site). NetVLAD overrides `output_key="vlad_descriptor"` only. This is the only intentional behavioural difference between the strategies' use of the helper.
- The helper signature uses keyword-only args for `output_key` / `input_key` (the `*` separator after `descriptor_dim`). This forbids positional misuse — a future strategy author cannot accidentally swap them.
### Phase 7 — Architecture Compliance
1. **Layer direction**: `_engine_dim_assertion.py` (c2_vpr, Layer 3) imports `ConfigError` from `gps_denied_onboard.config.schema` (Layer 1) and `numpy` (stdlib-equivalent). Type-only imports under `TYPE_CHECKING` are `EngineHandle` (`_types.inference`, Layer 1), `BackbonePreprocessor` (`c2_vpr/_preprocessor`, Layer 3 — same component), `InferenceRuntimeCut` (`c2_vpr/inference_runtime_cut`, Layer 3 — same component). All imports are downward or intra-component. No upward imports.
2. **Public API respect**: the helper is c2-internal — underscore-prefixed module name (`_engine_dim_assertion.py`) and NOT re-exported from `c2_vpr/__init__.py`. The 7 strategy modules import it as a sibling internal module, not via a Public API. No cross-component import added.
3. **No new cyclic deps**: built-import graph of changed files + direct dependencies. The helper has zero c2_vpr → c2_vpr cycle risk because it imports its dependencies under `TYPE_CHECKING` only (no runtime back-edge to the strategy modules). Strategy modules import the helper as a leaf — no cycle.
4. **Duplicate symbols**: 7-way `_assert_engine_output_dim` duplication is now zero-way duplicated. AC-4 regression guard prevents recurrence.
5. **Cross-cutting concerns not locally re-implemented**: the helper stays inside `c2_vpr/`, not in `shared/helpers/`. This is correct per AZ-527 § Out of scope: the engine output-shape contract is a c2 internal concern (C7 owns its own engine-shape assertions inside the runtime). The reasoning is explicit in `_engine_dim_assertion.py`'s module docstring.
### Phase 8 — Test infrastructure
- `tests/unit/c2_vpr/test_az527_engine_dim_assertion.py` — 14 tests, all PASS.
- `tests/unit/c2_vpr/` (full directory) — 230 / 230 PASS (UltraVPR / NetVLAD / MegaLoc / MixVPR / SelaVPR / EigenPlaces / SALAD AC-6 sub-tests all green; FaissBridge / protocol_conformance / new AZ-527 file all green).
- `tests/unit/test_az270_compose_root.py` (composition-root layer lint) — 8 / 8 PASS.
- `tests/unit/test_az508_iso_timestamps.py` (helpers AST-walk regression guards from AZ-526 — included to verify the ASYL pattern still holds in the helpers/ namespace; doesn't directly involve AZ-527 but sanity-checks neighbouring lint infrastructure) — 18 / 18 PASS.
## Verdict
- Critical findings: 0
- High findings: 0
- Medium findings: 0
- Low findings: 0
**PASS** — clean batch, no findings, all ACs covered, all tests green, full Ruff + lint clean.