VibecoderMcSwaggins commited on
Commit
0a03ac3
·
1 Parent(s): 1d4962c

docs(critical): upgrade NEXT-CONCERNS with validated architecture audit

Browse files

VALIDATED FROM FIRST PRINCIPLES - All claims verified by reading source code.

Priority upgraded from P2/P3 to P0/P1:

CONFIG DRIFT (BUG-009) - CRITICAL:
- Two parallel config systems: core/config.py vs api/config.py
- Different values for same concept (./results vs /tmp/stroke-results)
- Env var fragmentation (STROKE_DEMO_* vs raw FRONTEND_ORIGIN)
- Recommendation: Pydantic Settings as SSOT

DEPENDENCY PINNING (BUG-012) - CRITICAL:
- uv.lock committed but IGNORED by production Docker
- Dockerfile:37 uses pip install -r requirements.txt (loose pins)
- Base image unpinned (isleschallenge/deepisles:latest)
- Production builds are NOT reproducible
- Recommendation: Make Docker use uv sync --frozen + pin base image

Includes concrete migration plan with phases.

Files changed (1) hide show
  1. NEXT-CONCERNS.md +185 -46
NEXT-CONCERNS.md CHANGED
@@ -1,77 +1,216 @@
1
- # Next Concerns - To Investigate
2
 
3
- Deferred items from BUGS-AUDIT-2024-12-12.md that need deeper investigation.
 
4
 
5
  ---
6
 
7
- ## BUG-009: Results Directory Configuration Drift
8
 
9
- **Priority:** P2 (Medium)
10
 
11
- ### Current State
12
 
13
- Two files define where results go:
14
- - `core/config.py:93` → `results_dir: Path = Path("./results")`
15
- - `api/config.py:10` `RESULTS_DIR = Path("/tmp/stroke-results")`
 
16
 
17
- ### Investigation Needed
 
 
18
 
19
- 1. **Does anything actually use `core/config.py`'s `results_dir`?**
20
- - Grep codebase for imports from `core/config`
21
- - If nothing uses it, delete the duplicate
22
- - If something uses it, consolidate to single source of truth
23
 
24
- 2. **Gold standard pattern:**
25
- ```python
26
- # api/config.py (SSOT)
27
- RESULTS_DIR = Path("/tmp/stroke-results")
28
 
29
- # core/config.py (if needed)
30
- from stroke_deepisles_demo.api.config import RESULTS_DIR
31
- ```
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
32
 
33
- ### Action
34
 
35
- Run investigation, then either:
36
- - Delete unused `core/config.py` results_dir, OR
37
- - Make it import from `api/config.py`
38
 
39
  ---
40
 
41
- ## BUG-012: Loose Dependency Pinning
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
42
 
43
- **Priority:** P3 (Low)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
44
 
45
  ### Current State
46
 
47
- - `pyproject.toml` uses `>=` ranges (e.g., `fastapi>=0.115.0`)
48
- - `package.json` uses `^` ranges (e.g., `"react": "^19.2.0"`)
 
 
49
 
50
- ### Reality Check
51
 
52
- Lock files exist and are committed:
53
- - `uv.lock` → Backend installs are deterministic
54
- - `package-lock.json` → Frontend installs are deterministic
55
 
56
- **Builds ARE reproducible** if you use the lock files.
 
 
57
 
58
- ### Investigation Needed
59
 
60
- 1. Verify `uv.lock` is committed: `git ls-files uv.lock`
61
- 2. Verify Docker uses `uv sync` (respects lock) not `uv pip install` (ignores lock)
62
- 3. Verify CI uses lock files
63
 
64
- ### Action
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
65
 
66
- If lock files are committed and used correctly:
67
- - Close as "mitigated by lock files"
68
- - Optionally add README note about reproducible builds
69
 
70
- If lock files are NOT being used:
71
- - Fix CI/Docker to use them
72
- - Document requirement
 
 
73
 
74
  ---
75
 
76
- **Created:** 2024-12-12
77
- **Status:** Pending investigation
 
1
+ # Next Concerns - Critical Architecture Debt
2
 
3
+ **Status:** VALIDATED - All claims verified from first principles
4
+ **Priority:** P0/P1 - Production reproducibility and config integrity at risk
5
 
6
  ---
7
 
8
+ ## PART 1: CONFIG DRIFT (BUG-009) - CRITICAL
9
 
10
+ ### Current State: Two Parallel Config Systems
11
 
12
+ **System A: Pydantic Settings** (`core/config.py`)
13
 
14
+ ```python
15
+ # Line 93
16
+ results_dir: Path = Path("./results")
17
+ ```
18
 
19
+ - Uses `STROKE_DEMO_*` env prefix
20
+ - Supports `.env` file loading
21
+ - Used by: `inference/deepisles.py`, `ui/components.py`, `ui/app.py`
22
 
23
+ **System B: Module Constants** (`api/config.py`)
 
 
 
24
 
25
+ ```python
26
+ # Line 10
27
+ RESULTS_DIR = Path("/tmp/stroke-results")
28
+ ```
29
 
30
+ - Raw constants, no env var support
31
+ - Used by: `routes.py`, `files.py`, `main.py`, `job_store.py`
32
+
33
+ ### The Problem
34
+
35
+ These define **DIFFERENT VALUES** for the same concept:
36
+
37
+ - `./results` vs `/tmp/stroke-results`
38
+
39
+ Production uses System B (API), but System A exists and could cause confusion.
40
+
41
+ ### Env Var Fragmentation
42
+
43
+ | Where | Var Name | System |
44
+ |-------|----------|--------|
45
+ | Dockerfile:79 | `FRONTEND_ORIGIN` | Raw env var |
46
+ | Dockerfile:83 | `BACKEND_PUBLIC_URL` | Raw env var |
47
+ | core/config.py:68 | `STROKE_DEMO_*` | Pydantic Settings |
48
+
49
+ **These don't talk to each other.** The Dockerfile sets raw vars that Settings doesn't read.
50
+
51
+ ### Recommended Fix
52
+
53
+ **Option B from audit: Pydantic Settings as SSOT**
54
+
55
+ 1. Add API-relevant settings to `core/config.py`:
56
+ - `results_dir` (already exists, fix default to `/tmp/stroke-results`)
57
+ - `max_concurrent_jobs`
58
+ - `frontend_origin`
59
+ - `backend_public_url`
60
+
61
+ 2. Delete or convert `api/config.py` to thin shim that imports from Settings
62
 
63
+ 3. Update Dockerfile env vars to use `STROKE_DEMO_*` prefix
64
 
65
+ 4. Update all API modules to import from `core/config.py`
 
 
66
 
67
  ---
68
 
69
+ ## PART 2: DEPENDENCY PINNING (BUG-012) - CRITICAL
70
+
71
+ ### Current State: Production Build is NOT Reproducible
72
+
73
+ **What we have:**
74
+
75
+ - `uv.lock` ✅ Committed and used by CI
76
+ - `requirements.txt` ⚠️ Has loose `>=` ranges
77
+ - Dockerfile uses `pip install -r requirements.txt` ❌ **IGNORES uv.lock**
78
+
79
+ **Evidence:**
80
+
81
+ ```dockerfile
82
+ # Dockerfile:37 - IGNORES uv.lock!
83
+ RUN pip install --no-cache-dir -r requirements.txt
84
+ ```
85
+
86
+ ```txt
87
+ # requirements.txt - LOOSE PINS
88
+ fastapi>=0.115.0
89
+ pydantic>=2.5.0
90
+ uvicorn[standard]>=0.32.0
91
+ ```
92
+
93
+ ### Base Image Unpinned
94
+
95
+ ```dockerfile
96
+ # Dockerfile:16 - NO SHA DIGEST
97
+ FROM isleschallenge/deepisles:latest
98
+ ```
99
+
100
+ `:latest` can change anytime. A rebuild tomorrow could get different dependencies.
101
+
102
+ ### The Problem
103
+
104
+ | Environment | Install Method | Reproducible? |
105
+ |-------------|----------------|---------------|
106
+ | CI | `uv sync` | ✅ Yes |
107
+ | Local dev | `uv sync` | ✅ Yes |
108
+ | **Production Docker** | `pip install -r requirements.txt` | ❌ **NO** |
109
+
110
+ ### Recommended Fix
111
+
112
+ **Option A: Make Docker use uv.lock**
113
+
114
+ ```dockerfile
115
+ # Install uv
116
+ RUN pip install uv
117
 
118
+ # Copy lock file
119
+ COPY uv.lock pyproject.toml ./
120
+
121
+ # Install from lock (frozen = fail if lock is stale)
122
+ RUN uv sync --frozen --no-dev
123
+ ```
124
+
125
+ **Option B: Pin requirements.txt exactly**
126
+
127
+ ```txt
128
+ # requirements.txt - EXACT PINS
129
+ fastapi==0.115.6
130
+ pydantic==2.10.3
131
+ uvicorn[standard]==0.32.1
132
+ ```
133
+
134
+ **Either way: Pin the base image**
135
+
136
+ ```dockerfile
137
+ # Get digest from: docker pull isleschallenge/deepisles:latest && docker images --digests
138
+ FROM isleschallenge/deepisles@sha256:<actual-digest>
139
+ ```
140
+
141
+ ---
142
+
143
+ ## PART 3: FRONTEND CONFIG
144
 
145
  ### Current State
146
 
147
+ - `frontend/.env.production` hardcodes API URL at build time
148
+ - Works but not flexible for different deployments
149
+
150
+ ### HF Static Spaces Alternative
151
 
152
+ HF exposes runtime variables via `window.huggingface.variables`. Currently unused.
153
 
154
+ ### Recommendation
 
 
155
 
156
+ Keep current approach (build-time `.env.production`) unless multi-deployment flexibility needed.
157
+
158
+ ---
159
 
160
+ ## MIGRATION PLAN
161
 
162
+ ### Phase 1: Config Consolidation (BUG-009)
 
 
163
 
164
+ 1. Add to `core/config.py` Settings class:
165
+
166
+ ```python
167
+ # API settings
168
+ results_dir: Path = Path("/tmp/stroke-results") # Fix default
169
+ max_concurrent_jobs: int = 10
170
+ frontend_origins: list[str] = ["http://localhost:5173"]
171
+ backend_public_url: str | None = None
172
+ ```
173
+
174
+ 2. Update Dockerfile env vars:
175
+
176
+ ```dockerfile
177
+ ENV STROKE_DEMO_FRONTEND_ORIGINS='["https://...hf.space"]'
178
+ ENV STROKE_DEMO_BACKEND_PUBLIC_URL=https://...hf.space
179
+ ```
180
+
181
+ 3. Update imports in API modules:
182
+ - `routes.py`, `files.py`, `main.py`, `job_store.py`
183
+ - Change from `api.config` to `core.config.get_settings()`
184
+
185
+ 4. Delete `api/config.py` or convert to compatibility shim
186
+
187
+ ### Phase 2: Dependency Reproducibility (BUG-012)
188
+
189
+ 1. Pin base image with SHA digest in Dockerfile
190
+
191
+ 2. Choose ONE lock strategy:
192
+ - **Recommended:** Make Dockerfile use `uv sync --frozen`
193
+ - Alternative: Generate pinned requirements.txt from uv.lock
194
+
195
+ 3. Update CI to use `--frozen` flag (fail if lock stale)
196
+
197
+ ### Phase 3: Validation
198
+
199
+ 1. Build Docker image locally
200
+ 2. Verify all settings load correctly
201
+ 3. Verify reproducible builds (rebuild should get same versions)
202
+
203
+ ---
204
 
205
+ ## SOURCES
 
 
206
 
207
+ - FastAPI Settings: https://fastapi.tiangolo.com/advanced/settings/
208
+ - Pydantic Settings: https://docs.pydantic.dev/latest/concepts/pydantic_settings/
209
+ - uv sync docs: https://docs.astral.sh/uv/concepts/projects/sync/
210
+ - Docker pinning: https://docs.docker.com/build/building/best-practices/
211
+ - 12-Factor Config: https://12factor.net/config
212
 
213
  ---
214
 
215
+ **Validated:** 2024-12-12
216
+ **Status:** Ready for implementation