Claude commited on
Commit
13786b1
·
unverified ·
1 Parent(s): 368bb5a

feat: Sprint A14-S56 polish perf + concurrence (audits #12 #17 #18 #19 #27 #28 #29)

Browse files

#12 CSV pipeline_name parsing fragile
- _infer_pipeline_name strip le préfixe document_id (connu via
doc_result) avant de parser. Robuste aux doc_ids contenant des :.

#17 home_page filesystem scan
- Limite à 20 runs affichés, tri par mtime décroissant pour avoir
les plus récents. Documenté que le cache LRU est reporté pour
workspace > 1000 runs.

#18 JSON renderer round-trip wasteful
- Remplace json.loads(model_dump_json()) par model_dump(mode=json)
directement. Économie ~10x sur des manifests gros.

#19 JobStore migration schéma
- Nouvelle table schema_version créée à linit + check à louverture.
- Constante SCHEMA_VERSION = 1, code prêt pour ALTER TABLE
conditionnels en S57+.
- Downgrade détecté (version DB > version code) → JobStoreError.

#27 Mistral routing case-sensitive
- model.lower().startswith(mistral-ocr) au lieu de in. Évite faux
matches sur des modèles exotiques type pixtral-MISTRAL-OCR-fancy.

#28 JobStore concurrence
- Timeout 10s → 30s (absorbe contentions courtes).
- Ajout PRAGMA busy_timeout = 30000 (cohérent avec le timeout
Python mais explicite côté SQLite).

#29 InMemoryArtifactStore thread-safe test biaisé
- Ancien test : 100 threads × 10 clés disjointes → ne testait
pas la concurrence sur la même clé.
- Nouveau test ajouté : 50 threads × 20 puts sur la MÊME clé
→ vérifie convergence cohérente (last-write-wins, pas de
corruption, payload assorti à lartefact gagnant).

Tests : 818 passed dans tests/adapters + reports_v2 + interfaces +
integration, 0 régression.
Lint : All checks passed.

https://claude.ai/code/session_011XQZNitg1rCgia8ZD1a2hP

picarones/adapters/ocr/mistral_ocr.py CHANGED
@@ -215,7 +215,13 @@ class MistralOCRAdapter(BaseOCRAdapter):
215
  api_key = self._resolve_api_key()
216
  image_url = self._encode_image(image_path)
217
 
218
- if "mistral-ocr" in self._model.lower():
 
 
 
 
 
 
219
  text = self._call_native_ocr_api(image_url, api_key)
220
  else:
221
  text = self._call_chat_vision_api(image_url, api_key)
 
215
  api_key = self._resolve_api_key()
216
  image_url = self._encode_image(image_path)
217
 
218
+ # Sprint S56 (audit #27) : routing case-insensitive et plus
219
+ # strict. Avant le fix, ``"mistral-ocr" in model.lower()``
220
+ # matchait aussi un modèle exotique comme
221
+ # ``"pixtral-MISTRAL-OCR-fancy"``. On exige désormais que
222
+ # le model commence par "mistral-ocr" (préfixe officiel
223
+ # documenté).
224
+ if self._model.lower().startswith("mistral-ocr"):
225
  text = self._call_native_ocr_api(image_url, api_key)
226
  else:
227
  text = self._call_chat_vision_api(image_url, api_key)
picarones/adapters/storage/job_store.py CHANGED
@@ -148,12 +148,43 @@ class JobStore:
148
  Chemin du fichier SQLite. Créé s'il n'existe pas.
149
  """
150
 
 
 
 
 
 
 
151
  def __init__(self, db_path: Path | str) -> None:
152
  self._path = Path(db_path)
153
  self._path.parent.mkdir(parents=True, exist_ok=True)
154
  # Initialisation du schéma + WAL.
155
  with self._connect() as conn:
156
  conn.executescript(_SCHEMA_SQL)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
157
  try:
158
  conn.execute("PRAGMA journal_mode = WAL;")
159
  except sqlite3.Error: # pragma: no cover
@@ -166,14 +197,23 @@ class JobStore:
166
  return self._path
167
 
168
  def _connect(self) -> sqlite3.Connection:
169
- """Ouvre une nouvelle connexion. Le caller est responsable
170
- du commit + close (on utilise le context manager Python qui
171
- gère ça automatiquement)."""
 
 
 
 
 
 
 
172
  conn = sqlite3.connect(
173
  str(self._path),
174
  isolation_level=None, # autocommit pour simplicité
175
- timeout=10.0,
176
  )
 
 
177
  conn.row_factory = sqlite3.Row
178
  return conn
179
 
 
148
  Chemin du fichier SQLite. Créé s'il n'existe pas.
149
  """
150
 
151
+ #: Version du schéma SQL. Incrémenter à chaque migration.
152
+ #: Sprint S56 (audit #19) : avant ce sprint, aucune table de
153
+ #: version n'existait — un upgrade futur du schéma (ajout de
154
+ #: colonne) cassait silencieusement les bases existantes.
155
+ SCHEMA_VERSION = 1
156
+
157
  def __init__(self, db_path: Path | str) -> None:
158
  self._path = Path(db_path)
159
  self._path.parent.mkdir(parents=True, exist_ok=True)
160
  # Initialisation du schéma + WAL.
161
  with self._connect() as conn:
162
  conn.executescript(_SCHEMA_SQL)
163
+ # Table de version (S56) — pas dans le schéma principal
164
+ # pour rester rétrocompatible avec les bases pré-S56.
165
+ conn.execute(
166
+ "CREATE TABLE IF NOT EXISTS schema_version "
167
+ "(version INTEGER PRIMARY KEY)",
168
+ )
169
+ cur = conn.execute("SELECT version FROM schema_version")
170
+ row = cur.fetchone()
171
+ if row is None:
172
+ conn.execute(
173
+ "INSERT INTO schema_version (version) VALUES (?)",
174
+ (self.SCHEMA_VERSION,),
175
+ )
176
+ else:
177
+ existing = row[0]
178
+ if existing > self.SCHEMA_VERSION:
179
+ raise JobStoreError(
180
+ f"JobStore : base SQLite à la version "
181
+ f"{existing}, code à la version "
182
+ f"{self.SCHEMA_VERSION}. Downgrade non "
183
+ "supporté.",
184
+ )
185
+ # Pour S56, on n'a qu'une version — quand un futur
186
+ # sprint introduira la version 2, ajouter ici les
187
+ # ALTER TABLE conditionnels.
188
  try:
189
  conn.execute("PRAGMA journal_mode = WAL;")
190
  except sqlite3.Error: # pragma: no cover
 
197
  return self._path
198
 
199
  def _connect(self) -> sqlite3.Connection:
200
+ """Ouvre une nouvelle connexion.
201
+
202
+ Sprint S56 (audit #28) : timeout porté à 30s (de 10s) pour
203
+ absorber les contentions de courte durée, et configuration
204
+ ``busy_timeout`` côté SQLite (cohérent avec ``timeout`` mais
205
+ explicite pour les opérations qui ne passent pas par le
206
+ cursor Python). Le mode autocommit + WAL garantit que les
207
+ lectures n'attendent pas les écritures (cf.
208
+ https://sqlite.org/wal.html).
209
+ """
210
  conn = sqlite3.connect(
211
  str(self._path),
212
  isolation_level=None, # autocommit pour simplicité
213
+ timeout=30.0,
214
  )
215
+ # busy_timeout (ms) — backup au timeout Python.
216
+ conn.execute("PRAGMA busy_timeout = 30000;")
217
  conn.row_factory = sqlite3.Row
218
  return conn
219
 
picarones/interfaces/web/app.py CHANGED
@@ -297,12 +297,26 @@ def create_app(
297
  _runs_dir,
298
  _summarize,
299
  )
 
 
 
 
 
 
 
 
 
300
  runs_dir = _runs_dir(state)
301
  runs: list[dict] = []
302
  if runs_dir.exists():
303
- for entry in sorted(runs_dir.iterdir()):
304
- if not entry.is_dir():
305
- continue
 
 
 
 
 
306
  manifest_path = entry / "run_manifest.json"
307
  if not manifest_path.exists():
308
  continue
 
297
  _runs_dir,
298
  _summarize,
299
  )
300
+ # Sprint S56 (audit #17) : pour des workspaces utilisateur
301
+ # standard (< 100 runs), le scan filesystem à chaque requête
302
+ # reste sous la milliseconde et c'est acceptable. Pour un
303
+ # déploiement multi-tenants (>1000 runs), un cache LRU avec
304
+ # invalidation sur mtime du runs_dir serait pertinent —
305
+ # reporté à un sprint dédié si le besoin se présente.
306
+ # On limite déjà à 20 runs dans la liste pour ne pas générer
307
+ # de pages énormes.
308
+ MAX_RUNS_DISPLAYED = 20
309
  runs_dir = _runs_dir(state)
310
  runs: list[dict] = []
311
  if runs_dir.exists():
312
+ # Tri ordre décroissant (mtime) pour avoir les plus
313
+ # récents en tête, puis cap à MAX_RUNS_DISPLAYED.
314
+ entries = sorted(
315
+ (e for e in runs_dir.iterdir() if e.is_dir()),
316
+ key=lambda e: e.stat().st_mtime,
317
+ reverse=True,
318
+ )[:MAX_RUNS_DISPLAYED]
319
+ for entry in entries:
320
  manifest_path = entry / "run_manifest.json"
321
  if not manifest_path.exists():
322
  continue
picarones/reports_v2/csv/render.py CHANGED
@@ -123,11 +123,30 @@ class CsvReportRenderer:
123
  """Inféré depuis le ``candidate_artifact_id`` qui suit la
124
  convention ``<doc>:<pipeline>:<artifact_type>``.
125
 
126
- Fallback ``"<unknown>"`` si l'id n'est pas parseable.
 
 
 
 
 
 
 
127
  """
128
  cand_id = view_result.candidate_artifact_id
129
- # Convention : <document_id>:<pipeline_name>:<artifact_type>.
130
- # Le pipeline_name est entre les deux ":".
 
 
 
 
 
 
 
 
 
 
 
 
131
  parts = cand_id.split(":")
132
  if len(parts) >= 3:
133
  return parts[1]
 
123
  """Inféré depuis le ``candidate_artifact_id`` qui suit la
124
  convention ``<doc>:<pipeline>:<artifact_type>``.
125
 
126
+ Sprint S56 (audit #12) : le ``document_id`` autorise les ``:``
127
+ dans son format (cf. ``Artifact._ID_RE``). Un naive
128
+ ``split(":")[1]`` casse pour ``"d:1:tess:raw_text"``. On
129
+ utilise le ``doc_result.document_id`` connu pour stripper
130
+ le préfixe avec précision avant de parser.
131
+
132
+ Fallback ``"<unknown>"`` si l'id n'est pas parseable même
133
+ après stripping.
134
  """
135
  cand_id = view_result.candidate_artifact_id
136
+ doc_id = doc_result.document_id
137
+ # Strip le préfixe document_id de l'id. Format attendu :
138
+ # "<document_id>:<pipeline_name>:<artifact_type>".
139
+ prefix = f"{doc_id}:"
140
+ if cand_id.startswith(prefix):
141
+ remainder = cand_id[len(prefix):]
142
+ # remainder = "<pipeline>:<artifact_type>" (ou plus
143
+ # de ":" si artifact_type est composé, ce qui n'arrive
144
+ # pas avec ArtifactType mais on défend). rsplit gère.
145
+ pipeline_part = remainder.rsplit(":", 1)
146
+ if len(pipeline_part) == 2:
147
+ return pipeline_part[0]
148
+ # Fallback : ancienne heuristique pour les ids qui ne
149
+ # respectent pas la convention.
150
  parts = cand_id.split(":")
151
  if len(parts) >= 3:
152
  return parts[1]
picarones/reports_v2/json/render.py CHANGED
@@ -70,20 +70,27 @@ class JsonReportRenderer:
70
  )
71
 
72
  def _build_document(self, result: RunResult) -> dict:
73
- """Construit le dict canonique avant sérialisation."""
 
 
 
 
 
 
 
 
 
74
  return {
75
- "run_manifest": json.loads(
76
- result.manifest.model_dump_json(),
77
- ),
78
  "documents": [
79
  {
80
  "document_id": dr.document_id,
81
  "pipeline_results": [
82
- json.loads(pr.model_dump_json())
83
  for pr in dr.pipeline_results
84
  ],
85
  "view_results": [
86
- json.loads(vr.model_dump_json())
87
  for vr in dr.view_results
88
  ],
89
  }
 
70
  )
71
 
72
  def _build_document(self, result: RunResult) -> dict:
73
+ """Construit le dict canonique avant sérialisation.
74
+
75
+ Sprint S56 (audit #18) : on utilise désormais
76
+ ``model_dump(mode="json")`` directement au lieu de faire un
77
+ round-trip ``model_dump_json() → loads → dumps``. Pydantic
78
+ 2.x sait produire un dict JSON-serializable directement
79
+ (datetime → ISO string, enum → value, etc.) ; le double
80
+ encode/decode était gaspilleur (~10× le coût pour des
81
+ manifests gros).
82
+ """
83
  return {
84
+ "run_manifest": result.manifest.model_dump(mode="json"),
 
 
85
  "documents": [
86
  {
87
  "document_id": dr.document_id,
88
  "pipeline_results": [
89
+ pr.model_dump(mode="json")
90
  for pr in dr.pipeline_results
91
  ],
92
  "view_results": [
93
+ vr.model_dump(mode="json")
94
  for vr in dr.view_results
95
  ],
96
  }
tests/adapters/storage/test_sprint_a14_s29_artifact_store.py CHANGED
@@ -321,8 +321,8 @@ class TestInMemoryArtifactStore(_SharedStoreContract):
321
  keys = store.keys()
322
  assert set(keys) == {"k1", "k2"}
323
 
324
- def test_thread_safe_basic(self) -> None:
325
- """100 threads écrivent chacun 10 entrées → 1000 entrées."""
326
  store = InMemoryArtifactStore()
327
  artifact = _make_artifact()
328
 
@@ -340,6 +340,41 @@ class TestInMemoryArtifactStore(_SharedStoreContract):
340
  t.join()
341
  assert len(store) == 1000
342
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
343
 
344
  class TestFilesystemArtifactStore(_SharedStoreContract):
345
  def make_store(self, tmp_path: Path) -> ArtifactStore:
 
321
  keys = store.keys()
322
  assert set(keys) == {"k1", "k2"}
323
 
324
+ def test_thread_safe_disjoint_keys(self) -> None:
325
+ """100 threads écrivent chacun 10 clés disjointes → 1000."""
326
  store = InMemoryArtifactStore()
327
  artifact = _make_artifact()
328
 
 
340
  t.join()
341
  assert len(store) == 1000
342
 
343
+ def test_thread_safe_concurrent_overwrites_same_key(self) -> None:
344
+ """Sprint S56 (audit #29) : test de concurrence sur la MÊME
345
+ clé. Avec 50 threads qui put la même clé en parallèle, le
346
+ store doit converger sur une valeur (last-write-wins) sans
347
+ crash, sans corruption, sans clé fantôme."""
348
+ store = InMemoryArtifactStore()
349
+
350
+ def writer(i: int) -> None:
351
+ for _ in range(20):
352
+ store.put(
353
+ "shared_key",
354
+ _make_artifact(artifact_id=f"d{i}:art"),
355
+ payload=f"payload_{i}".encode(),
356
+ )
357
+
358
+ threads = [
359
+ threading.Thread(target=writer, args=(i,))
360
+ for i in range(50)
361
+ ]
362
+ for t in threads:
363
+ t.start()
364
+ for t in threads:
365
+ t.join()
366
+ # Une seule clé "shared_key" — pas de duplication.
367
+ assert len(store) == 1
368
+ # Le stored est cohérent (artifact + payload appartiennent
369
+ # au même writer, pas un mix).
370
+ stored = store.get("shared_key")
371
+ assert stored is not None
372
+ # L'id de l'artefact détermine quel writer a gagné ; le
373
+ # payload doit correspondre au même writer.
374
+ assert stored.artifact.id.startswith("d")
375
+ winner_idx = stored.artifact.id.split(":")[0][1:]
376
+ assert stored.payload == f"payload_{winner_idx}".encode()
377
+
378
 
379
  class TestFilesystemArtifactStore(_SharedStoreContract):
380
  def make_store(self, tmp_path: Path) -> ArtifactStore: