From 578d2aec128775f7c101a6b381e75d647c62d1f9 Mon Sep 17 00:00:00 2001 From: Nicolas Fryder Date: Wed, 19 Aug 2026 15:10:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(groq):=20repli=20d'analyse=20JSON=20tronqu?= =?UTF-8?q?=C3=A9=20sur=20objet=20imbriqu=C3=A9=20+=20tests=20du=20worker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG — _parse_json perdait les réponses contenant un objet imbriqué Le repli utilisé quand le modèle enrobe son JSON de prose s'appuyait sur `\{[^}]+\}`, qui s'arrête au PREMIER `}`. Dès que la réponse contenait un objet imbriqué (ex. {"city": "Oslo", "meta": {"confidence": 0.9}}), la capture était tronquée donc invalide : _parse_json renvoyait None, le lieu comptait comme « aucune ville trouvée », consommait un essai LLM et finissait par basculer en 'manual'. Aucune erreur levée — défaut silencieux, sur un appel facturé. Le re.DOTALL passé en argument était de surcroît inopérant, `[^}]` matchant déjà les retours à la ligne : signe que l'intention était `.`. Corrigé en `\{.*\}` glouton, du premier `{` au dernier `}`. TESTS (23) — helpers purs de groq_worker, jusqu'ici sans couverture - _parse_json : JSON propre, enrobé de prose, imbriqué, multiligne, et tous les cas inexploitables — qui doivent rendre None sans jamais lever, sous peine d'interrompre le worker. - _input_hash : c'est la clé du cache LLM, donc ce qui décide de rappeler Groq ou non. Vérifié qu'elle normalise casse et espaces (sinon on repaie des appels déjà faits) et qu'elle distingue modèle, lieu et pays. - _cost et PRICING : un modèle absent de PRICING serait facturé au tarif de repli sans qu'on le remarque, faussant le coût affiché dans l'admin. - _apply_clean_query : appelée pour de vrai avec un curseur factice plutôt que de réécrire la concaténation dans le test. conftest reproduit le sys.path du conteneur (src/ ET racine de l'app) : worker.py et groq_worker.py importent `parser` sans préfixe parce qu'ils sont lancés en `python src/worker.py`. Vérification écartée : `COUNTRY_NAMES.get(code, iso2)` retombe sur le code brut pour un ISO2 inconnu (« Oslo, ZZ »). Mon test l'avait pris pour un bug ; c'est volontaire — mieux vaut un code qu'aucun contexte pays. Documenté. Deux tests tautologiques écrits puis supprimés avant commit : ils comparaient deux f-strings identiques et ne vérifiaient rien. Co-Authored-By: Claude --- apps/geocoder/src/groq_worker.py | 9 +- apps/geocoder/tests/conftest.py | 16 +++ apps/geocoder/tests/test_groq_worker.py | 179 ++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 apps/geocoder/tests/conftest.py create mode 100644 apps/geocoder/tests/test_groq_worker.py diff --git a/apps/geocoder/src/groq_worker.py b/apps/geocoder/src/groq_worker.py index 4f13a08..781f200 100644 --- a/apps/geocoder/src/groq_worker.py +++ b/apps/geocoder/src/groq_worker.py @@ -116,7 +116,14 @@ def _parse_json(text: str) -> dict | None: try: return json.loads(text.strip()) except json.JSONDecodeError: - m = re.search(r'\{[^}]+\}', text, re.DOTALL) + # Repli quand le modèle enrobe son JSON de texte. `[^}]+` s'arrêtait au + # PREMIER `}` : dès que la réponse contenait un objet imbriqué, la + # capture était tronquée et invalide, le lieu comptait pour un échec et + # consommait un essai LLM pour rien. Le re.DOTALL était de surcroît + # inopérant, `[^}]` matchant déjà les retours à la ligne. + # `.*` glouton va du premier `{` au dernier `}` : correct pour un objet + # unique noyé dans de la prose, ce qui est le seul cas observé. + m = re.search(r'\{.*\}', text, re.DOTALL) if m: try: return json.loads(m.group()) diff --git a/apps/geocoder/tests/conftest.py b/apps/geocoder/tests/conftest.py new file mode 100644 index 0000000..6d0de65 --- /dev/null +++ b/apps/geocoder/tests/conftest.py @@ -0,0 +1,16 @@ +import os +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + +# Deux entrées, parce que les modules du geocoder s'importent de deux façons : +# - `src.parser` depuis les tests (racine de l'app sur le path) +# - `parser` depuis worker.py et groq_worker.py, lancés en +# `python src/worker.py` dans le conteneur — sys.path[0] vaut alors src/. +# Reproduire les deux ici, c'est tester exactement ce qui s'exécute en prod. +sys.path.insert(0, str(ROOT)) +sys.path.insert(0, str(ROOT / "src")) + +# Les modules lisent DATABASE_URL au chargement ; aucune connexion n'est ouverte. +os.environ.setdefault("DATABASE_URL", "postgresql://test:test@localhost:5432/test") diff --git a/apps/geocoder/tests/test_groq_worker.py b/apps/geocoder/tests/test_groq_worker.py new file mode 100644 index 0000000..09c99a8 --- /dev/null +++ b/apps/geocoder/tests/test_groq_worker.py @@ -0,0 +1,179 @@ +""" +Helpers purs du worker Groq. + +Chaque appel Groq est facturé et compté dans un quota journalier (1 000 req/j +sur le modèle 70b). Une réponse mal analysée ne lève pas d'erreur : elle compte +comme « aucune ville trouvée », consomme un essai LLM et finit par basculer le +lieu en 'manual'. Défaut silencieux, donc coûteux. +""" + +import hashlib + +import pytest +from src.groq_worker import ( + COUNTRY_NAMES, + MODELS, + PRICING, + _cost, + _input_hash, + _parse_json, +) + + +class TestParseJson: + def test_json_propre(self): + assert _parse_json('{"city": "Oslo", "country": "NO"}') == {"city": "Oslo", "country": "NO"} + + def test_tolere_les_espaces_autour(self): + assert _parse_json(' \n {"city": "Oslo"} \n ') == {"city": "Oslo"} + + def test_repli_quand_le_modele_ajoute_du_texte(self): + assert _parse_json('Voici le résultat :\n{"city": "Bergen"}\nVoilà.')["city"] == "Bergen" + + # Régression : le repli s'arrêtait au PREMIER `}`. Un objet imbriqué + # produisait une capture tronquée, donc invalide — le lieu comptait pour un + # échec et consommait un essai LLM pour rien. + def test_repli_sur_un_json_contenant_un_objet_imbrique(self): + texte = 'Résultat: {"city": "Oslo", "meta": {"confidence": 0.9}} fin' + out = _parse_json(texte) + assert out is not None + assert out["city"] == "Oslo" + assert out["meta"]["confidence"] == 0.9 + + def test_repli_sur_json_multiligne_avec_imbrication(self): + texte = 'Bla\n{\n "city": "Bergen",\n "extra": {"x": [1, 2]}\n}\nBla' + assert _parse_json(texte)["city"] == "Bergen" + + @pytest.mark.parametrize("texte", [ + "", "aucun json ici", "{ceci n'est pas du json}", "{", "}", "null-ish", + ]) + def test_texte_inexploitable_rend_none(self, texte): + # None fait basculer sur le chemin « aucune ville », qui incrémente les + # essais : il ne doit surtout pas lever et interrompre le worker. + assert _parse_json(texte) is None + + def test_city_null_est_conserve_tel_quel(self): + # Le modèle est explicitement instruit de renvoyer city: null quand il + # ne trouve rien : ce n'est pas une erreur d'analyse. + assert _parse_json('{"city": null, "country": "NO"}') == {"city": None, "country": "NO"} + + +class TestInputHash: + def test_stable(self): + a = _input_hash("m", "Oslo", "NO") + assert a == _input_hash("m", "Oslo", "NO") + assert len(a) == 64 + + # La clé de cache détermine si on rappelle Groq : trop laxiste, on sert un + # mauvais résultat ; trop stricte, on repaie des appels déjà faits. + @pytest.mark.parametrize("a,b", [ + (("m", "Oslo", "NO"), ("m", "oslo", "NO")), # casse du lieu + (("m", "Oslo", "NO"), ("m", " Oslo ", "NO")), # espaces + (("m", "Oslo", "NO"), ("m", "Oslo", "no")), # casse du pays + ]) + def test_normalise_avant_de_hacher(self, a, b): + assert _input_hash(*a) == _input_hash(*b) + + @pytest.mark.parametrize("a,b", [ + (("m1", "Oslo", "NO"), ("m2", "Oslo", "NO")), # modèle différent + (("m", "Oslo", "NO"), ("m", "Bergen", "NO")), # lieu différent + (("m", "Oslo", "NO"), ("m", "Oslo", "SE")), # pays différent + ]) + def test_distingue_ce_qui_doit_letre(self, a, b): + assert _input_hash(*a) != _input_hash(*b) + + def test_pays_vide_et_none_donnent_la_meme_cle(self): + assert _input_hash("m", "Oslo", "") == _input_hash("m", "Oslo", None) + + def test_correspond_a_sha256(self): + attendu = hashlib.sha256(b"m|NO|oslo").hexdigest() + assert _input_hash("m", "Oslo", "NO") == attendu + + +class TestCost: + def test_calcul_par_modele(self): + pin, pout = PRICING["llama-3.3-70b-versatile"] + assert _cost("llama-3.3-70b-versatile", 1000, 100) == pytest.approx(1000 * pin + 100 * pout) + + def test_modele_inconnu_retombe_sur_le_tarif_le_plus_cher(self): + # Sous-estimer le coût d'un modèle inconnu donnerait une fausse + # assurance sur le budget. + cher = PRICING["llama-3.3-70b-versatile"] + assert _cost("modele-inconnu", 1000, 100) == pytest.approx(1000 * cher[0] + 100 * cher[1]) + + def test_zero_token_coute_zero(self): + assert _cost("llama-3.1-8b-instant", 0, 0) == 0 + + def test_croit_avec_les_tokens(self): + m = "llama-3.1-8b-instant" + assert _cost(m, 100, 0) < _cost(m, 200, 0) + + +class TestConfiguration: + def test_tous_les_modeles_ont_un_tarif(self): + # Un modèle sans tarif serait facturé au tarif de repli sans qu'on le + # remarque : le coût affiché dans l'admin deviendrait faux. + for model, _rpm, _rpd in MODELS: + assert model in PRICING, model + + def test_les_quotas_sont_positifs(self): + for model, rpm, rpd in MODELS: + assert rpm > 0 and rpd > 0, model + + def test_le_modele_de_repli_a_un_quota_journalier_superieur(self): + # L'ordre de MODELS est celui de préférence : le second sert quand le + # premier a épuisé son quota, il doit donc en avoir davantage. + assert MODELS[1][2] > MODELS[0][2] + + def test_les_noms_de_pays_couvrent_les_codes_courants(self): + for iso2 in ("FR", "NO", "DE", "SE", "GB"): + assert COUNTRY_NAMES.get(iso2), iso2 + + +class TestRequeteNettoyee: + """La requête que `_apply_clean_query` renvoie à Geoapify. + + On appelle la vraie fonction avec un curseur factice : réécrire la + concaténation dans le test ne vérifierait que le test lui-même. + """ + + class FakeCursor: + def __init__(self): + self.queries = [] + + def execute(self, sql, params=None): + self.queries.append((sql, params)) + + def _query(self, city, iso2): + from src.groq_worker import _apply_clean_query + cur = self.FakeCursor() + _apply_clean_query(cur, 1, city, iso2, 0) + return cur.queries[0][1][0] + + def test_ajoute_le_nom_du_pays_quand_il_est_connu(self): + assert self._query("Oslo", "NO") == f"Oslo, {COUNTRY_NAMES['NO']}" + + def test_iso2_inconnu_est_transmis_tel_quel(self): + # `COUNTRY_NAMES.get(code, iso2)` retombe sur le CODE plutôt que sur une + # chaîne vide : mieux vaut donner « Oslo, ZZ » à Geoapify que perdre + # tout contexte pays. Comportement volontaire, documenté ici parce qu'il + # surprend à la lecture. + assert self._query("Oslo", "ZZ") == "Oslo, ZZ" + + def test_sans_pays_la_ville_part_seule_sans_virgule_orpheline(self): + for absent in (None, ""): + requete = self._query("Oslo", absent) + assert requete == "Oslo" + assert not requete.endswith(", ") + + def test_remet_le_lieu_en_file_et_reinitialise_les_essais_geo(self): + from src.groq_worker import _apply_clean_query + cur = self.FakeCursor() + _apply_clean_query(cur, 42, "Oslo", "NO", 2) + sql, params = cur.queries[0] + assert "geocode_status='queued'" in sql + # Le géocodeur doit repartir de zéro sur la requête nettoyée, sinon il + # abandonnerait immédiatement en réutilisant ses anciens essais. + assert "geocode_tries_geo=0" in sql + assert params[1] == 3 # tries_llm incrémenté + assert params[2] == 42