From 690125bec36e488038c0b12c46cf7755ebbb1bc1 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Tue, 25 Aug 2026 20:23:35 +0200 Subject: [PATCH] fix(recipes): corrige les matches dupliques et le timeout de warm-up des tests CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deux bugs reels trouves par la premiere execution CI de la migration node-nlp -> tech-step-intent-service : 1. PhraseMatcher retourne tous les matches y compris chevauchants — un synonyme comme "fondre" litteralement contenu dans "faire fondre" (tous deux synonymes de `melt`) produisait deux candidats separes pour la meme technique, dupliquant son techStepId dans le resultat final. Fixe avec spacy.util.filter_spans (garde le plus long match par position) dans LocalePipeline.process. Test de non-regression ajoute. 2. La suite Mocha construit `app` directement via createApp(), sans jamais passer par server.ts — le warm-up (POST /v1/train fr+en sur le corpus complet) se declenchait donc paresseusement dans le premier test qui appelait le classifieur, depassant le timeout Mocha de 10s par test. Fixe par un root hook plugin Mocha (test-support/mocha-root-hooks.ts, .mocharc.json) qui reset la DB et warm up le classifieur une seule fois avant toute suite, avec son propre timeout de 60s. Verifie : 27/27 tests pytest du service (dont le nouveau test de non-regression), lint + build complets du monorepo. La suite Mocha elle-meme n'a toujours pas pu etre executee dans cet environnement (pas de Postgres disponible ici) — a confirmer via la CI. Co-Authored-By: Claude Sonnet 5 --- apps/api/.mocharc.json | 3 +- apps/api/test-support/mocha-root-hooks.ts | 40 +++++++++++++++++++ .../intent_service/locale_pipeline.py | 35 +++++++++++----- .../tests/test_locale_pipeline_entities.py | 24 ++++++++++- 4 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 apps/api/test-support/mocha-root-hooks.ts diff --git a/apps/api/.mocharc.json b/apps/api/.mocharc.json index 193fdb9..2a00c64 100644 --- a/apps/api/.mocharc.json +++ b/apps/api/.mocharc.json @@ -2,5 +2,6 @@ "extension": ["ts"], "spec": "test/**/*.test.ts", "node-option": ["import=tsx"], - "timeout": 10000 + "timeout": 10000, + "require": ["test-support/mocha-root-hooks.ts"] } diff --git a/apps/api/test-support/mocha-root-hooks.ts b/apps/api/test-support/mocha-root-hooks.ts new file mode 100644 index 0000000..016869d --- /dev/null +++ b/apps/api/test-support/mocha-root-hooks.ts @@ -0,0 +1,40 @@ +import { techStepClassifier } from "../src/lib/recipe-matching/tech-step-matcher.js"; +import { resetDatabase } from "./reset-db.js"; + +/** + * Mocha root hook plugin (see `.mocharc.json`'s `require`) — runs once + * before every test file's own suites, regardless of load order. + * + * Warms up `techStepClassifier` here, with its own generous timeout, + * instead of leaving it to happen lazily on whichever test file Mocha + * happens to load first. In production this one-time cost (a `POST + * /v1/train` round-trip per locale to `services/tech-step-intent-service`, + * training a real `textcat` on the full `TECH_STEP_TRAINING_DATA` corpus) + * is paid by `server.ts`'s own `techStepClassifier.warmUp()` before the + * server ever accepts traffic — but this test suite builds its `app` + * directly via `createApp()` (see e.g. `tech-step-worker.routes.test.ts`), + * never running `server.ts` at all. Without this hook, that cost instead + * landed inside whichever test's own call happened to trigger + * `_ensureTrained()` first — found the hard way in CI, where training the + * full corpus took longer than a single test's default 10s timeout + * (`.mocharc.json`) and failed an otherwise-unrelated test purely because + * Mocha loaded its file first alphabetically. + * + * `resetDatabase()` runs first, deliberately: `_train()` + * (`tech-step-matcher.ts`) resolves `TechStep.key -> id` from the database + * alongside training, and a freshly-migrated (never-seeded) test database + * has no `TechStep` rows yet — every per-test `beforeEach` in this suite + * already calls `resetDatabase()` again before its own test, which is a + * no-op duplication of effort but not a correctness problem: `TRUNCATE ... + * RESTART IDENTITY` plus deterministic re-seeding (`seedReferenceData`) + * assigns the exact same ids every time, so the `uid -> id` map memoized + * here from this first reset stays valid for every reset after it. + */ +export const mochaHooks = { + // biome-ignore lint/suspicious/noExplicitAny: Mocha's root hook `this` (a Context with `.timeout()`) isn't typed without @types/mocha (not a dependency here) — same untyped-`this` shape already used in tech-step-worker.routes.test.ts. + async beforeAll(this: any): Promise { + this.timeout(60000); + await resetDatabase(); + await techStepClassifier.warmUp(); + }, +}; diff --git a/services/tech-step-intent-service/intent_service/locale_pipeline.py b/services/tech-step-intent-service/intent_service/locale_pipeline.py index 0643df7..5a0c790 100644 --- a/services/tech-step-intent-service/intent_service/locale_pipeline.py +++ b/services/tech-step-intent-service/intent_service/locale_pipeline.py @@ -25,9 +25,9 @@ from dataclasses import dataclass, field import spacy from spacy.language import Language from spacy.matcher import PhraseMatcher -from spacy.tokens import Doc +from spacy.tokens import Doc, Span from spacy.training import Example -from spacy.util import minibatch +from spacy.util import filter_spans, minibatch from .text_normalization import normalize_text @@ -272,14 +272,31 @@ class LocalePipeline: doc = self._base_nlp(text) - entities = [ - Entity( - uid=self._base_nlp.vocab.strings[match_id], - start=doc[start].idx, - end=doc[end - 1].idx + len(doc[end - 1].text), - ) - for match_id, start, end in self._matcher(doc) + # A technique's own synonym list can legitimately contain one phrase + # nested inside another (`melt`'s "fondre" is a literal substring of + # its own "faire fondre") — the `PhraseMatcher` reports *both* as + # separate matches at overlapping positions, which without + # resolution would hand `splitIntoClauses` (apps/api) two candidates + # for what a human reads as one mention, producing the same + # techStepId twice in the final result. `filter_spans` keeps only + # the longest match at each position (so "faire fondre" wins over + # the "fondre" it contains) — found by a real regression in + # `tech-step-matcher.test.ts`'s "detects several distinct + # techniques..." case once this service replaced node-nlp (which + # apparently resolved this internally; nothing here recreates that + # by choice, `filter_spans` is spaCy's own documented tool for + # exactly this "one span per position" problem, e.g. as used for + # NER-style outputs). + matched_spans = [ + Span(doc, start, end, label=match_id) for match_id, start, end in self._matcher(doc) ] + entities = sorted( + ( + Entity(uid=self._base_nlp.vocab.strings[span.label], start=span.start_char, end=span.end_char) + for span in filter_spans(matched_spans) + ), + key=lambda entity: entity.start, + ) cats = doc.cats if not cats: diff --git a/services/tech-step-intent-service/tests/test_locale_pipeline_entities.py b/services/tech-step-intent-service/tests/test_locale_pipeline_entities.py index f108abd..eb2169f 100644 --- a/services/tech-step-intent-service/tests/test_locale_pipeline_entities.py +++ b/services/tech-step-intent-service/tests/test_locale_pipeline_entities.py @@ -24,8 +24,15 @@ _FR_ENTRIES = [ utterances=["préchauffer le four à 180 degrés", "mettre la poêle sur feu vif"], ), TrainEntry( + # `synonyms` deliberately includes both "fondre" (standalone) and + # "faire fondre" (containing it) — mirrors the real corpus + # (`tech-step-training-data.ts`) exactly, and is what + # `test_does_not_double_match_a_synonym_nested_in_a_longer_one` + # below exists to guard: the `PhraseMatcher` reports both as + # separate overlapping matches, `LocalePipeline.process` must + # collapse them into one. uid="melt", - synonyms=["faire fondre", "faire chauffer"], + synonyms=["fondre", "fondu", "faire fondre", "faire chauffer"], utterances=["faire fondre le beurre", "faire chauffer une noix de beurre"], ), TrainEntry( @@ -89,6 +96,21 @@ def test_detects_two_techniques_with_exact_tight_spans_reading_order(fr_pipeline assert text[melt_entity.start : melt_entity.end].lower() == "faire fondre" +def test_does_not_double_match_a_synonym_nested_in_a_longer_one(fr_pipeline: LocalePipeline): + # Regression: "fondre" is itself a substring of "faire fondre" — both + # are registered as `melt` synonyms (like the real corpus). Without + # `filter_spans` in `LocalePipeline.process`, the `PhraseMatcher` + # reports *both* overlapping matches, producing `melt` twice in + # apps/api's final `matchTechSteps` output instead of once (caught by a + # real CI failure in `tech-step-matcher.test.ts` once this service + # replaced node-nlp). + text = "faire fondre le beurre" + result = fr_pipeline.process(text) + assert [entity.uid for entity in result.entities] == ["melt"] + entity = result.entities[0] + assert text[entity.start : entity.end] == "faire fondre" + + def test_matches_the_classic_poele_chaude_example_with_exact_offsets(fr_pipeline: LocalePipeline): # Le cas motivant les context spans côté apps/api (tech-step-matcher.test.ts) : # le mot-clé de `preheat` est un groupe nominal ("poêle chaude"), pas un