Je rebondis sur ce sujet du coup, j’ai terminé la vérification pour la PR sur l’amélioration du recalcul du suivi de l’énergie. Je pense que tu peux review :
S’ensuivra la PR suivante qui ajoute du logging plus claire pour l’utilisateur, notamment pour voir ou en étaient les dernière tache lancées si jamais un redémarrage Gladys ou un problème était survenu pendant un recalcul (évitant de devoir refaire tout le recalcul …) :
Est-ce que tu pourrais décrire plus en détail ce que fait cette PR ?
Comme cela touche au core, ce sont des changements assez sensibles.
J’aimerais bien comprendre précisément le périmètre pour pouvoir évaluer correctement l’impact.
Lors du développement j’ai passé plusieurs mois sur la fiabilisation des calculs, et c’est vital pour moi que le calcul ne soit pas impacté 
Tu as raison de demander le détail, je comprends tout à fait. Et si possible, je préférerais que tu le test également, en le laissant tourner à côté de ta prod por bien vérifier. C’est ce que j’ai fais pour bien évaluer le bon fonctionnement. Je n’ai pas trouvé de cas de figure où ça plante. Mais par exemple, je n’ai pas de zigbee énergie par exemple. Même si il n’y a aucune raison que ce soit différent, ça couvrirait l’ensemble.
Le diff global est gros (+3203 / -428), mais le périmètre fonctionnel que je vise dans cette PR est surtout celui-ci :
calculate-cost-range)calculate-consumption-from-index-range)feature_selectors).YYYY-MM-DD côté controller (avec BadParameters si format invalide).destroyStatesBetween) avant recomputation.destroyStatesFrom).ENERGY_INDEX_LAST_PROCESSED en fin de traitement sur les recalculs partiels/sélectionnés pour éviter d’abîmer le curseur global.wrapperDetached pour les recalculs longs (from beginning / range), pour renvoyer immédiatement un job_id.job_id est bien retourné.range si une date est fournie, sinon des endpoints from-beginning.contracts.calculateCost reste le même).Je n’ai :
J’ai « seulement » modifié le périmètre d’entrée (features sélectionnées, plage de dates) et l’encadrement technique des recalculs (purge ciblée, jobs, API/front).
Et pour tout te dire, en début de semaine, j’ai faillis fermer les PR de ce sujet et t’écrire pour te dire que je te laissais la main sur ce sujet.
J’avais peur que ça te prenne plus de temps à review qu’à le faire toi même. Qu’en plus visuellement ça ne te convienne pas. Et que du coup tu aurais sûrement été mieux à le faire tel que tu le penses.
Mais je balance entre cette question et le fait qu’on soit quand meme quelques uns à être en attente. Et du fait que je vois bien que tu es occupé dans bien d’autres choses.
Alors je suis parti du posta de proposer, et que toi tu disposes. Et tu me donneras ton retour / ressenti. 
Merci pour les précisions, je te tiens au courant quand j’ai pu regarder
J’ai regardé rapidement la PR et j’ai l’impression qu’elle touche à plus de choses que le simple re-calcul avec les dates.
Par exemple, les quatre premiers fichiers semblent être un reliquat d’une autre PR :
Pour le code métier, la PR est assez massive et touche à tellement d’éléments qu’il est difficile d’être certain qu’elle ne modifie pas l’algorithme de calcul.
Il y a même une nouvelle logique introduite avec des notions nouvelles (comme shouldRestoreLastProcessed), donc ce n’est clairement pas une PR triviale.
Pourtant intuitivement, si je réfléchis 2 minutes à l’idée, j’ai du mal à comprendre pourquoi c’est aussi complexe.
Je me demande si ton intuition n’est pas bonne malheureusement :
C’est vraiment dommage, car je suis plutôt d’accord avec le résultat et l’UX me paraît bonne. Le problème, c’est que le code est énorme : je ne peux pas merger sans le lire entièrement, comprendre les nouveaux concepts et effectuer tous les tests nécessaires… 
Qu’en penses-tu ?
Bonjour @pierre-gilles
Merci d’avoir commencé à regarder.
La branche de la PR partait de la branche Tasmota, qui as eu pas mal de changement suite à nos discussions.
Après l’update de la PR une fois Tasmota mergée, j’ai refais une passe, mais je suis passé à côté du fichier ‹ front/src/components/device/index.js › qui intègre toujours la création des device conso/coût => Je ne comprend pas comment je suis passé à côté, c’est le 1er fichier et c’est un pavé. C’est retiré
front/src/components/device/UpdateDevice.jsx et front/src/components/device/UpdateDeviceFeature.jsx c’était l’ajout d’un raccourci depuis les features qui avaient des consos / coût associés pour aller directement sur la page de suivi de l’énergie. Ca pourra etre proposé plus tard, j’ai retiré.
front/src/components/drag-and-drop/DeviceListWithDragAndDrop.jsx : le composant est utilisé dans la sélection des devices à recalculer. Il y avait besoin d’ajouter une impossibilité de changer le nom pour cet usage.
Je suis d’accord, ce n’est pas une PR triviale sur ce point.
De base, j’ai envisagé de dupliquer la logique dans un moteur de recalcul séparé pour ne pas dépendre de ENERGY_INDEX_LAST_PROCESSED.
Mais j’ai volontairement évité ça: on aurait eu 2 implémentations parallèles (incrémental vs recalcul), donc plus de complexité avec un risque de divergence dans le temps.
Aujourd’hui j’ai ajouté calculateConsumptionFromIndexRange en tant qu’orchestrateur (bornes de dates, nettoyage, boucle des fenêtres).
J’ai voulu conserver le calcul réel de chaque fenêtre en passant toujours par calculateConsumptionFromIndex, qui lui utilise ENERGY_INDEX_LAST_PROCESSED.
Du coup, shouldRestoreLastProcessed sert à protéger le curseur incrémental live lors des recalculs partiels/périodes passées. Parce qu’avant, le calcul allait toujours « jusqu’au bout ». Mais là le but c’est de pouvoir ajouter un prix de contrat ancien et de lancer le recalcul sur cette plage seulement. Si c’était il y a 1 an, sans ça le prochain calcul 30 minutes aurait repris sur une année complete au lieu de redémarrer au dernier vrai run de suivi.
Sans restauration:
Avec restauration:
Parce que ce curseur est utilisé par le calcul de consommation index.
En bref, c’est un garde-fou pour garder un moteur unique maintenable, sans effet de bord sur les runs incrémentaux suivants.
Bien entendu je me trompe peut-etre sur la logique, mais notamment sur ce point, j’ai travaillé avec l’IA et tenter de tourner ca dans tous les sens pour être certains que ce soit une bonne manière de faire (pas forcément la meilleur, la review sert à ça !)
Je suis bien conscient de cela, et je t’en ai parlé dès le début de cette PR. Ca touche un élément sensible, et j’esperais que justement tu puisses le faire tourner sur une instance à côté de ta prod pour être certains que les calculs (qui n’ont pas été touchés) sont bons meme après recalcul sur plages, etc.
Pour ce qui est du code, pour info ce sont les tests les plus gros :
Bon week-end à toi et bonne course
Salut @pierre-gilles,
Ne sachant pas si tu es en cours ou quand tu passera en review sur cette PR, j’ai pensé bon de relancer une review complète de @coderabbitai car je m’aperçois qu’il n’en refait plus après quelques passes.
Et au vue du nombre de commit fait depuis sa 1ère review je me suis dit qu’il serait bien d’en refaire une pleine sur la PR complète : https://github.com/GladysAssistant/Gladys/pull/2413#pullrequestreview-3943430667
Je pense qu’il a ressortis de bonne chose. Si tu n’as pas commencé ta review, puis-je push les changements ? Sinon que préfères-tu ?
Je n’ai pas encore regardé, tu peux modifier la PR
Ok, c’est fait.
Salut @pierre-gilles,
Je reviens vers toi sur la PR #2413 (recalcul énergie).
Toujours en attente de ce recalcul pour pouvoir pleinement utiliser l’intégration de suivi de l’énergie, j’ai repris la PR proprement (merge master, correctifs CodeRabbit, retests sur ma prod sur près de 2 mois sans aucune divergence de calcul avec la version officielle). Je l’ai réouverte.
Pour rappel du fil de discussion : tu m’avais indiqué début avril préférer refaire le sujet toi-même parce que la PR te semblait trop grosse et trop sensible. J’avais accepté et mis la PR en pause.
Pour avancer concrètement, je te propose de redécouper la PR en plusieurs petites PRs indépendantes, mergeables une par une. Tu pourrais relire par petits morceaux, tester chaque étape sur ta prod, et revenir en arrière facilement si besoin :
destroyStatesBetween + ses tests (utilitaire pur, isolé, ~100-200 lignes).Tu pourrais merger PR1 et PR2 sans engager les concepts les plus sensibles (shouldRestoreLastProcessed n’apparaît qu’en PR3).
Si tu valides ce découpage, je m’y mets cette semaine. Si tu préfères reprendre toi-même avec un horizon clair, dis-le et je refermerai la PR. Ce qui m’ennuie, c’est l’absence de décision et de ne pas pouvoir pleinement utiliser cette intégration, il me semble que d’autres utilisateurs étaient également concernés.
Merci pour ton retour.
Salut,
Le sujet est surtout une question de confiance 
Ce qui me pose problème avec la PR dans son état actuel, c’est qu’elle modifie en profondeur la logique de calcul. L’année dernière, cette partie m’a pris plusieurs mois à fiabiliser pour les trois types de contrats gérés (base, heures pleines/heures creuses et Tempo), avec beaucoup de tests sur des données réelles provenant de dizaines d’utilisateurs.
Normalement, les tests unitaires servent justement à détecter les régressions. Mais aujourd’hui, avec l’IA, les tests eux-mêmes peuvent être massivement modifiés automatiquement, ce qui est le cas ici. Du coup, je ne peux plus considérer les tests comme une garantie fiable. L’IA peut très bien avoir introduit des régressions puis adapté les tests pour les valider.
Pour merger cette PR telle quelle, il faudrait donc que je relise et comprenne l’intégralité du code modifié, puis que je refasse toute la phase de validation réalisée l’année dernière. Et malheureusement, je n’ai même plus les jeux de données de test que certains utilisateurs m’avaient envoyés à l’époque. C’est donc un chantier quasiment équivalent au développement initial, pas une simple itération.
À l’inverse, si tu proposes une PR “front only” qui ajoute simplement la possibilité de recalculer sur le dernier mois / 3 derniers mois / 6 derniers mois / 12 derniers mois, elle pourrait probablement être mergée en moins d’une heure, sans phase de validation lourde, puisque le code métier ne serait pas modifié.
Le découpage en petites PR peut aider à rendre les changements plus lisibles, mais dès qu’on touche à la logique métier ici, il y aura de toute façon une grosse phase de validation incompressible.
Qu’en penses-tu ?
Et désolé si ça peut sembler dur, ce n’est vraiment pas contre ton travail. C’est surtout que, avec mon planning actuel, des chantiers de cette ampleur sont très compliqués à absorber J’aimerais avoir plus de temps pour ce genre de revue approfondie, mais le projet n’en est pas encore à ce stade.
Tout d’abord merci pour cette réponse, qui a le mérite d’être complète et honnête sur tes contraintes.
Pour pouvoir te répondre pleinement, et pas juste « je suis sûr de mon code », j’ai pris le temps de mener un audit rigoureux de la PR cet après-midi. J’ai cadré la demande avec une consigne explicite : vérifier que chaque changement est nécessaire, solide, bien pensé, sans régression, et surtout que les tests ne sont pas dénaturés — y compris en cassant volontairement le code pour vérifier que les tests détectent bien les régressions (mutation testing réel). C’est précisément le doute légitime que tu soulèves sur l’IA et les tests, et je voulais y apporter une réponse vérifiable, pas une affirmation.
Je te livre ci-dessous le résultat de cet audit, factuel et reproductible chez toi.
Je maintiens ma réponse précédente, mais cette fois avec la vérification ligne à ligne. Les deux fichiers où la logique métier vit sont :
server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndex.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.jsJ’ai relu chaque ligne modifiée. Ce qui est strictement inchangé :
convertEnergyUnit(...) — conversions d’unitécontracts[contract](...) — toutes les formules de coût (base, HP/HC, Tempo)subtract(30, 'minutes')saveMultipleHistoricalStates / saveHistoricalStateLes seuls ajouts dans calculateConsumptionFromIndex.js sont encapsulés dans un if (selectorSet.size > 0) (filtrage par whitelist) : désactivés en l’absence de whitelist, donc comportement legacy strictement préservé pour le calcul incrémental live qui tourne toutes les 30 minutes.
Dans calculateCostFrom.js, les ajouts sont :
if (selectorSet.size > 0))destroyStatesBetween si endAt fourni, sinon destroyStatesFrom (comportement legacy)try/catch par feature de coût (robustesse : une feature qui plante ne bloque pas les autres)La formule de coût elle-même — celle qui t’a pris des mois à fiabiliser sur base/HP/HC/Tempo — n’a pas bougé d’un caractère. C’est vérifiable en quelques minutes avec git diff master HEAD -- server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js et un Ctrl+F sur contracts[contract], convertEnergyUnit, energyPricesForDate.
C’est une crainte parfaitement légitime, et c’est précisément pour ça que j’ai fait du mutation testing réel : j’ai cassé temporairement 3 points critiques du code et relancé toute la suite. Le but est de vérifier que les tests détectent la régression, pas qu’ils passent avec la régression.
Voici les 3 mutations testées et le résultat :
| Mutation appliquée | Tests qui cassent |
|---|---|
| Validation des dates retirée du controller | reject invalid start date format + reject invalid end date format (2 tests) |
Filtre whitelist du moteur live calculateConsumptionFromIndex retiré |
skip consumption features not in whitelist selectors (1 test) |
Bloc finally de restauration du curseur ENERGY_INDEX_LAST_PROCESSED retiré |
restore last processed value on selector-based recalculation + restore last processed value on window error (2 tests) |
Total : 5 tests cassent précisément sur les bonnes assertions. Les tests ne sont pas du green-washing IA, ils détectent vraiment les régressions sur les protections critiques. Reproductible chez toi : il suffit de commenter ces 3 blocs et relancer npm run test-service --service=energy-monitoring. Fichiers restaurés après vérification, j’ai confirmé avec git status qu’il n’y a aucune modification résiduelle.
J’ai aussi vérifié la non-régression des tests existants :
calculateConsumptionFromIndex.test.js) ont uniquement un ajout de undefined en deuxième paramètre (pour s’aligner sur la nouvelle signature). Aucune assertion n’a été affaiblie ou supprimée.calculateCostFromYesterday.test.js est en réalité renforcé : avant il vérifiait juste calledOnce, maintenant il vérifie la signature exacte des arguments passés.try/catch + expect.fail est remplacé par next(error) — c’est le pattern Express correct.Je comprends la logique : éviter la phase de validation lourde sur le métier en ne touchant pas au backend. Mais cette proposition ne couvre pas le besoin réel des utilisateurs, et c’est important que je l’explique :
Cas d’usage n°1 — Le recalcul historique sur appareils existants ajoutés au suivi a posteriori
C’est la promesse du suivi de l’énergie dans Gladys. Beaucoup d’utilisateurs (dont moi) ont des prises Tasmota ou Zigbee2mqtt depuis plusieurs années dans leur instance Gladys, avec parfois 4 ans de données d’index stockées. Quand on ajoute l’intégration suivi d’énergie sur ces appareils, on doit pouvoir recalculer la consommation et le coût sur toute l’historique disponible, et pas seulement sur les 12 derniers mois.
Aujourd’hui, le seul moyen est le from-beginning global, qui :
Un preset « 12 derniers mois » ne résout aucun de ces trois problèmes. Une sélection par feature + plage de dates les résout tous les trois.
Cas d’usage n°2 — Le tarif rétroactif
Un utilisateur qui s’aperçoit qu’il a changé de tarif il y a 18 mois et qui veut recalculer uniquement cette plage est bloqué avec un preset 12 mois.
Cas d’usage n°3 — La granularité
Quand on a 30 appareils suivis et qu’on veut juste corriger une donnée sur un seul appareil sur une semaine, lancer un recalcul global sur 1/3/6/12 mois est disproportionné et risqué pour les autres appareils.
Je voudrais aussi mentionner deux points où la PR rend le code plus sûr que master :
wrapperDetached (nouvelle primitive job) découple la requête HTTP du calcul long. Avant, le from-beginning bloquait la requête HTTP jusqu’à la fin du calcul, ce qui faisait timeout sur les grosses installs. C’est probablement la cause silencieuse des « ça ne marche pas » qu’on remonte régulièrement.try/finally autour de la restauration du curseur ENERGY_INDEX_LAST_PROCESSED protège l’instance d’un état corrompu si une window échoue en plein recalcul. Sur master, si une window plante, le curseur reste détruit et le calcul incrémental live démarre depuis zéro à la prochaine itération.Pour ne pas te raconter que tout est parfait, l’audit a aussi remonté 3 cleanups mineurs (non bloquants, sans risque de régression) :
calculateCostFrom qui n’a plus d’utilité (tous les callers internes passent déjà la nouvelle signature).FromBeginning et Range que je peux factoriser.calculateCostFrom.test.js qui ne testent pas vraiment ce qu’ils prétendent (à durcir ou retirer — les vrais tests métier restent dans les tests originaux non modifiés).Je m’engage à corriger ces 3 points avant le découpage.
Compte tenu de ce qui précède, je te propose toujours le découpage en sous-PRs, mais en aménageant ton souci de « phase de validation incompressible » :
destroyStatesBetween + wrapperDetached + leurs tests. ~150-200 lignes. Aucune logique métier, juste des utilitaires. Mergeable rapidement, low-risk.if (selectorSet.size > 0), donc le comportement legacy reste strictement préservé.shouldRestoreLastProcessed. C’est là que se concentre la « vraie » review métier.Sur PR3 — celle qui te demande le plus de validation — je suis prêt à :
shouldRestoreLastProcessedSi tu valides ce découpage, je commence par PR1 cette semaine. Si tu préfères que je reste sur l’attente, dis-le-moi avec un horizon — même large — et je fermerai à nouveau la PR sereinement.
Merci pour ton temps, et désolé si la réponse est dense — j’ai préféré te donner du factuel plutôt que de l’affirmation.
@pierre-gilles, complément à mon message précédent.
Pour appuyer concrètement ce que j’écrivais sur l’audit, j’ai fait les cleanups que j’avais annoncés.
calculateCostFrom.test.jsEn auditant les tests ajoutés par cette PR sur ce fichier (les tests d’origine sur master n’ont pas été touchés), j’ai trouvé 5 tests problématiques sur 6 :
should handle case where no energy price is found for the device at the given date)device.create() + db.duckDbBatchInsertState() + energyPrice.create()), comme le font tous les tests originaux du fichier sur master. Les versions précédentes utilisaient des stubs sinon qui rendaient les tests moins fiables et incohérents avec le reste du fichier.Pendant l’audit, j’ai aussi découvert que le test should handle case where no energy price is found for the device at the given date qui est sur master ne couvre pas réellement la branche qu’il prétend tester. Raison technique : son energy_parent_id sur le cost feature pointe vers l’index feature au lieu de la consumption feature, donc le matching échoue plus tôt dans le code et la branche if (energyPricesForDate.length === 0) n’est jamais atteinte. Cette branche était en réalité couverte uniquement par un de mes tests stubbés que je voulais retirer.
J’ai donc ajouté un test propre en DB réelle pour préserver cette couverture. Je n’ai pas modifié le test master qui présente le bug — je me suis interdit de toucher au legacy.
J’ai fais une review de la PR 1 !
Merci à toi,
Je t’ai répondu et j’ai corrigé.
J’ai également ouvert la PR2 (ajout des recalcul sur feature seulement, sans les plages de dates qui interviendront en PR3) pour référence.
Je n’ai pas encore tout retester. Je ferais ca demain soir.
En tout cas, à mon niveau, je suis assez impressionné par la rigueur de Terdious dans la description de ce qu’il a été préparé. J’espère que vous trouverez ensemble la façon d’emmener cette évolution jusqu’à une mise en prod, parce que ça va vraiment être utile de pouvoir faire ces recalculé ciblés.
Salut @StephaneB,
Merci pour ton message, ça me touche.
Je me permets quand même, par honnêteté et transparence envers Pierre-Gilles, de nuancer un point :
Je ne suis pas développeur métier, je suis autodidacte. J’ai des idées claires et j’arrive généralement à coder ce dont j’ai besoin, mais sur la rigueur d’architecture, je m’appuie beaucoup sur les retours en review pour progresser — et c’est tout à fait normal que ça demande plus de cycles à Pierre-Gilles avec moi qu’avec d’autres contributeurs.
D’ailleurs, dès la PR1, j’avais fait le choix de dupliquer deux fonctions plutôt que d’étendre les existantes — par excès de prudence sur le core de Gladys, que je sais sensible. C’était finalement le mauvais choix dans ce contexte, et Pierre-Gilles a eu raison de me le faire remarquer. Je l’ai refactoré dans la bonne direction.
Donc oui, je tiens à porter ce développement parce que je crois qu’il sert un vrai besoin partagé — mais pas en me prêtant des qualités que je n’ai pas. Le sérieux que je peux mettre dans la description et les tests vient surtout du temps que j’y passe et des outils que j’utilise, pas d’un niveau dev pro.
Merci en tout cas pour ton soutien.
Salut @pierre-gilles,
Pour info, j’ai posté un retour de test sur la PR1 (#2528). J’ai laissé tourner une image Docker terdious/gladys:energy-monitoring-pr1 sur une copie isolée de ma DB de prod depuis hier soir, lancé un recalcul complet des coûts (1h30, logs clean), et comparé les valeurs recalculées à ma prod mois par mois et année par année — ça matche.
Je peux laisser tourner encore un ou deux jours pour valider l’incrémental sur la durée, ou enchaîner directement sur une image de test PR2 si tu préfères. Dis-moi.
Top merci, je te dis dès que j’ai pu regarder !