Skip to content

Commit 008aaac

Browse files
w7-learnclaude
andcommitted
fix: code improvements and documentation fixes
- Add date range filter to SalesDaily cleanup in ingest tests - Enforce artifact_hash presence before verification in registry routes - Compute SHA256 from saved file instead of source in storage - Fix override_get_db to mirror production transaction semantics - Filter DeploymentAlias cleanup to only test runs - Update database port to 5433 in config and .env.example - Add language identifiers to fenced code blocks (MD040) - Fix table formatting for markdownlint MD060 - Update PR reference in PHASE/6-MODEL_REGISTRY.md - Convert bare URLs to markdown links in INITIAL-7.md - Wrap __init__.py in backticks in PRP-7 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 902f331 commit 008aaac

12 files changed

Lines changed: 204 additions & 185 deletions

File tree

.env.example

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
# Copy this file to .env and adjust values as needed
33

44
# Database connection (PostgreSQL + pgvector via Docker Compose)
5-
DATABASE_URL=postgresql+asyncpg://forecastlab:forecastlab@localhost:5432/forecastlab
5+
DATABASE_URL=postgresql+asyncpg://forecastlab:forecastlab@localhost:5433/forecastlab
66

77
# Application settings
88
APP_NAME=ForecastLabAI

INITIAL-7.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@
3232
## DOCUMENTATION:
3333
- Postgres JSONB patterns
3434
- Artifact integrity (hashing) best practices
35-
- https://scalegrid.io/blog/using-jsonb-in-postgresql-how-to-effectively-store-index-json-data-in-postgresql/
36-
- https://www.fortra.com/blog/supply-chain-vulnerability
35+
- [Using JSONB in PostgreSQL](https://scalegrid.io/blog/using-jsonb-in-postgresql-how-to-effectively-store-index-json-data-in-postgresql/)
36+
- [Supply Chain Vulnerability](https://www.fortra.com/blog/supply-chain-vulnerability)
3737

3838
## OTHER CONSIDERATIONS:
3939
- No hardcoded artifact paths: derived from `ARTIFACT_ROOT` + run_id.

PRPs/PRP-7-model-registry.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,14 +1050,14 @@ IMPLEMENT:
10501050
- compare_runs.py: Compare two runs, show config/metrics diff
10511051
```
10521052
1053-
### Task 18: Update module __init__.py exports
1053+
### Task 18: Update module `__init__.py` exports
10541054

10551055
```yaml
10561056
FILE: app/features/registry/__init__.py
10571057
ACTION: MODIFY
10581058
IMPLEMENT:
10591059
- Export all public classes
1060-
- __all__ list (sorted alphabetically)
1060+
- `__all__` list (sorted alphabetically)
10611061
VALIDATION:
10621062
- uv run python -c "from app.features.registry import *; print('OK')"
10631063
```

app/core/config.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ class Settings(BaseSettings):
2121
debug: bool = False
2222

2323
# Database
24-
database_url: str = "postgresql+asyncpg://forecastlab:forecastlab@localhost:5432/forecastlab"
24+
database_url: str = "postgresql+asyncpg://forecastlab:forecastlab@localhost:5433/forecastlab"
2525

2626
# Logging
2727
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO"

app/features/ingest/tests/test_routes.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,11 @@ async def db_session():
4646
async with async_session_maker() as cleanup_session:
4747
with suppress(Exception):
4848
# Clean up test data (delete in correct order due to FK constraints)
49-
await cleanup_session.execute(delete(SalesDaily))
49+
await cleanup_session.execute(
50+
delete(SalesDaily).where(
51+
(SalesDaily.date >= date(2024, 1, 1)) & (SalesDaily.date <= date(2024, 12, 31))
52+
)
53+
)
5054
await cleanup_session.execute(delete(Product).where(Product.sku.like("SKU-%")))
5155
await cleanup_session.execute(delete(Store).where(Store.code.like("S00%")))
5256
await cleanup_session.execute(

app/features/registry/routes.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,12 @@ async def verify_artifact(
349349
detail="Run has no associated artifact",
350350
)
351351

352+
if run.artifact_hash is None:
353+
raise HTTPException(
354+
status_code=status.HTTP_400_BAD_REQUEST,
355+
detail="Run has no stored artifact hash",
356+
)
357+
352358
storage = LocalFSProvider()
353359

354360
try:

app/features/registry/storage.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,13 @@ def save(self, source_path: Path, artifact_uri: str) -> tuple[str, int]:
182182
dest_path = self._resolve_path(artifact_uri)
183183
dest_path.parent.mkdir(parents=True, exist_ok=True)
184184

185-
# Compute hash before copy
186-
file_hash = self.compute_hash(source_path)
187-
file_size = source_path.stat().st_size
188-
189-
# Copy file
185+
# Copy file first
190186
shutil.copy2(source_path, dest_path)
191187

188+
# Compute hash and size from the saved file
189+
file_hash = self.compute_hash(dest_path)
190+
file_size = dest_path.stat().st_size
191+
192192
logger.info(
193193
"registry.artifact_saved",
194194
artifact_uri=artifact_uri,

app/features/registry/tests/conftest.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import pytest
1010
from httpx import ASGITransport, AsyncClient
11-
from sqlalchemy import delete
11+
from sqlalchemy import delete, select
1212
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
1313

1414
from app.core.config import get_settings
@@ -45,7 +45,11 @@ async def db_session() -> AsyncGenerator[AsyncSession, None]:
4545
yield session
4646
finally:
4747
# Clean up test data (delete in correct order due to FK constraints)
48-
await session.execute(delete(DeploymentAlias))
48+
# Only delete aliases for test runs (those with model_type.like("test-%"))
49+
test_run_ids = select(ModelRun.id).where(ModelRun.model_type.like("test-%"))
50+
await session.execute(
51+
delete(DeploymentAlias).where(DeploymentAlias.run_id.in_(test_run_ids))
52+
)
4953
await session.execute(delete(ModelRun).where(ModelRun.model_type.like("test-%")))
5054
await session.commit()
5155

@@ -57,7 +61,12 @@ async def client(db_session: AsyncSession) -> AsyncGenerator[AsyncClient, None]:
5761
"""Create test client with database dependency override."""
5862

5963
async def override_get_db() -> AsyncGenerator[AsyncSession, None]:
60-
yield db_session
64+
try:
65+
yield db_session
66+
await db_session.commit()
67+
except Exception:
68+
await db_session.rollback()
69+
raise
6170

6271
app.dependency_overrides[get_db] = override_get_db
6372

docs/ARCHITECTURE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,7 @@ forecast_enable_lightgbm: bool = False
321321
- `DeploymentAlias` - Mutable pointers to successful runs for deployment
322322

323323
**Run Lifecycle (State Machine):**
324-
```
324+
```text
325325
PENDING → RUNNING → SUCCESS/FAILED → ARCHIVED
326326
```
327327
- Validated transitions prevent invalid state changes

docs/PHASE/4-FORECASTING.md

Lines changed: 45 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ class BaseForecaster(ABC):
3838

3939
**Model Types Implemented**:
4040

41-
| Model | Class | Description | Key Parameter |
41+
|Model|Class|Description|Key Parameter|
4242
|-------|-------|-------------|---------------|
43-
| `naive` | `NaiveForecaster` | Predicts last observed value for all horizons | None |
44-
| `seasonal_naive` | `SeasonalNaiveForecaster` | Predicts value from same season in previous cycle | `season_length` (default: 7) |
45-
| `moving_average` | `MovingAverageForecaster` | Predicts mean of last N observations | `window_size` (default: 7) |
46-
| `lightgbm` | (Placeholder) | LightGBM regressor (feature-flagged) | `n_estimators`, `max_depth`, `learning_rate` |
43+
|`naive`|`NaiveForecaster`|Predicts last observed value for all horizons|None|
44+
|`seasonal_naive`|`SeasonalNaiveForecaster`|Predicts value from same season in previous cycle|`season_length` (default: 7)|
45+
|`moving_average`|`MovingAverageForecaster`|Predicts mean of last N observations|`window_size` (default: 7)|
46+
|`lightgbm`|(Placeholder)|LightGBM regressor (feature-flagged)|`n_estimators`, `max_depth`, `learning_rate`|
4747

4848
**FitResult Dataclass**:
4949
```python
@@ -62,18 +62,18 @@ class FitResult:
6262

6363
Pydantic v2 schemas with frozen configs for reproducibility:
6464

65-
| Schema | Purpose |
65+
|Schema|Purpose|
6666
|--------|---------|
67-
| `ModelConfigBase` | Base with `schema_version` and `config_hash()` |
68-
| `NaiveModelConfig` | Config for naive forecaster |
69-
| `SeasonalNaiveModelConfig` | Config with `season_length` (1-365) |
70-
| `MovingAverageModelConfig` | Config with `window_size` (1-90) |
71-
| `LightGBMModelConfig` | Config for LightGBM (n_estimators, max_depth, learning_rate) |
72-
| `TrainRequest` | API request with store_id, product_id, date range, config |
73-
| `TrainResponse` | Response with model_path, n_observations, duration_ms |
74-
| `PredictRequest` | Request with horizon (1-90), model_path |
75-
| `PredictResponse` | Response with forecast points |
76-
| `ForecastPoint` | Single forecast with date, value, optional bounds |
67+
|`ModelConfigBase`|Base with `schema_version` and `config_hash()`|
68+
|`NaiveModelConfig`|Config for naive forecaster|
69+
|`SeasonalNaiveModelConfig`|Config with `season_length` (1-365)|
70+
|`MovingAverageModelConfig`|Config with `window_size` (1-90)|
71+
|`LightGBMModelConfig`|Config for LightGBM (n_estimators, max_depth, learning_rate)|
72+
|`TrainRequest`|API request with store_id, product_id, date range, config|
73+
|`TrainResponse`|Response with model_path, n_observations, duration_ms|
74+
|`PredictRequest`|Request with horizon (1-90), model_path|
75+
|`PredictResponse`|Response with forecast points|
76+
|`ForecastPoint`|Single forecast with date, value, optional bounds|
7777

7878
**Key Features**:
7979
- Frozen models (`frozen=True`) for immutability
@@ -150,10 +150,10 @@ class ForecastingService:
150150

151151
**File**: `app/features/forecasting/routes.py`
152152

153-
| Endpoint | Method | Description |
153+
|Endpoint|Method|Description|
154154
|----------|--------|-------------|
155-
| `/forecasting/train` | POST | Train a forecasting model |
156-
| `/forecasting/predict` | POST | Generate forecasts using trained model |
155+
|`/forecasting/train`|POST|Train a forecasting model|
156+
|`/forecasting/predict`|POST|Generate forecasts using trained model|
157157

158158
**Train Request Example**:
159159
```json
@@ -204,12 +204,12 @@ class ForecastingService:
204204

205205
**Directory**: `app/features/forecasting/tests/`
206206

207-
| File | Tests | Coverage |
207+
|File|Tests|Coverage|
208208
|------|-------|----------|
209-
| `test_schemas.py` | 20 | Schema validation, config hash, frozen models |
210-
| `test_models.py` | 24 | Model fit/predict, edge cases, params |
211-
| `test_persistence.py` | 15 | Save/load bundles, version compatibility |
212-
| `test_service.py` | 20 | Service integration, validation, logging |
209+
|`test_schemas.py`|20|Schema validation, config hash, frozen models|
210+
|`test_models.py`|24|Model fit/predict, edge cases, params|
211+
|`test_persistence.py`|15|Save/load bundles, version compatibility|
212+
|`test_service.py`|20|Service integration, validation, logging|
213213

214214
**Total**: 79 tests
215215

@@ -223,11 +223,11 @@ class ForecastingService:
223223

224224
**Directory**: `examples/models/`
225225

226-
| File | Description |
226+
|File|Description|
227227
|------|-------------|
228-
| `baseline_naive.py` | Naive forecaster demo |
229-
| `baseline_seasonal.py` | Seasonal naive with weekly seasonality |
230-
| `baseline_mavg.py` | Moving average with configurable window |
228+
|`baseline_naive.py`|Naive forecaster demo|
229+
|`baseline_seasonal.py`|Seasonal naive with weekly seasonality|
230+
|`baseline_mavg.py`|Moving average with configurable window|
231231

232232
---
233233

@@ -246,19 +246,19 @@ forecast_model_artifacts_dir: str = "./artifacts/models"
246246
forecast_enable_lightgbm: bool = False
247247
```
248248

249-
| Setting | Default | Description |
249+
|Setting|Default|Description|
250250
|---------|---------|-------------|
251-
| `forecast_random_seed` | 42 | Random seed for reproducibility |
252-
| `forecast_default_horizon` | 14 | Default forecast horizon in days |
253-
| `forecast_max_horizon` | 90 | Maximum allowed horizon |
254-
| `forecast_model_artifacts_dir` | `./artifacts/models` | Directory for saved models |
255-
| `forecast_enable_lightgbm` | False | Feature flag for LightGBM models |
251+
|`forecast_random_seed`|42|Random seed for reproducibility|
252+
|`forecast_default_horizon`|14|Default forecast horizon in days|
253+
|`forecast_max_horizon`|90|Maximum allowed horizon|
254+
|`forecast_model_artifacts_dir`|`./artifacts/models`|Directory for saved models|
255+
|`forecast_enable_lightgbm`|False|Feature flag for LightGBM models|
256256

257257
---
258258

259259
## Directory Structure
260260

261-
```
261+
```text
262262
app/features/forecasting/
263263
├── __init__.py # Module exports
264264
├── models.py # BaseForecaster + implementations
@@ -284,7 +284,7 @@ examples/models/
284284

285285
## Validation Results
286286

287-
```
287+
```bash
288288
$ uv run ruff check app/features/forecasting/
289289
All checks passed!
290290

@@ -302,16 +302,16 @@ $ uv run pytest app/features/forecasting/tests/ -v
302302

303303
## Logging Events
304304

305-
| Event | Description |
305+
|Event|Description|
306306
|-------|-------------|
307-
| `forecasting.train_request_received` | Train request received |
308-
| `forecasting.train_request_completed` | Training completed successfully |
309-
| `forecasting.train_request_failed` | Training failed |
310-
| `forecasting.predict_request_received` | Prediction request received |
311-
| `forecasting.predict_request_completed` | Prediction completed |
312-
| `forecasting.predict_request_failed` | Prediction failed |
313-
| `forecasting.model_saved` | Model bundle saved to disk |
314-
| `forecasting.model_loaded` | Model bundle loaded from disk |
307+
|`forecasting.train_request_received`|Train request received|
308+
|`forecasting.train_request_completed`|Training completed successfully|
309+
|`forecasting.train_request_failed`|Training failed|
310+
|`forecasting.predict_request_received`|Prediction request received|
311+
|`forecasting.predict_request_completed`|Prediction completed|
312+
|`forecasting.predict_request_failed`|Prediction failed|
313+
|`forecasting.model_saved`|Model bundle saved to disk|
314+
|`forecasting.model_loaded`|Model bundle loaded from disk|
315315

316316
---
317317

0 commit comments

Comments
 (0)