Repository navigation
Adaptar las lecturas de los datos estadísticos de la CNMC de año de retribución a giscedata_cups_estadistiques y no directamente el cups - #733
Conversation
giscebot
left a comment
There was a problem hiding this comment.
Solicito cambios porque la adaptación sigue siendo incompatible con la retirada de los campos legacy en gisce/erp#28653.
He revisado el diff completo en b635e15463bc, los consumidores de estadísticas del repositorio, el contrato de la PR ERP dependiente y el estado de CI. git diff --check pasa, pero los tests de libCNMC están omitidos y quedan estos bloqueos:
UpdateCNMCStats.__init__usaself.connection.GiscedataCupsEstadistques(falta la segundaideEstadistiques). El resto del repositorio usaGiscedataCupsEstadistiques; por tanto,update_cnmc_statsfallará antes de procesar el CSV.- El propio FA1 todavía incluye
cnmc_numero_lecturesen elreaddeGiscedataCupsPsy construyeo_num_lecturesdesdecups. Ese campo también se mueve al modelo anual, así que el generador seguirá fallando tras desplegar la PR ERP. - La migración no cubre todos los consumidores activos:
cir_4_2014/F1.py,F1bis.pyyF11.py;cir_4_2015/F1.py,F1bis.pyyF11.py; ycir_8_2021/FA2.pyaún leen del CUPS camposcne_anual_*,cnmc_potencia_facturadaocnmc_numero_lecturesque gisce/erp#28653 elimina. No basta con adaptar FA1: esos informes quedarán rotos con la versión mínima enlazada desde ERP. - No hay regresiones para el nuevo loader ni para la selección por
(cups_id, year). Hace falta demostrar al menos que el import actualiza el año objetivo (sin tocar el adyacente) y que los generadores ya no solicitan campos eliminados al CUPS.
Además, el año del loader queda fijado internamente a datetime.today().year - 1; conviene exponer/pasar el ejercicio solicitado, igual que hacen los generadores, para que una recarga histórica no escriba sobre otro año.
Antes de aprobar: corregir el nombre del proxy, mover todas las lecturas restantes al registro anual, trasladar también cnmc_numero_lectures en FA1 y añadir pruebas que fallen si vuelve cualquiera de estas rutas legacy.
giscebot
left a comment
There was a problem hiding this comment.
Solicito cambios. La segunda iteración corrige la migración de las lecturas en F1/F1bis/F11/FA1/FA2 y expone --year, pero el loader sigue sin poder ejecutar correctamente:
- En
libcnmc/core/__init__.py:328el proxy continúa escrito comoGiscedataCupsEstadistques. Debe serGiscedataCupsEstadistiques; con el nombre actual falla al construirUpdateCNMCStats, antes de leer el CSV. - En
libcnmc/core/__init__.py:329se sigue fijandoself.year = datetime.today().year - 1. El nuevoyearque pasa el CLI queda ignorado, por lo que una carga histórica escribe en el ejercicio equivocado. El constructor debe tomar el valor recibido enkwargs(manteniendo, si se desea, el mismo valor por defecto en un único sitio).
Además, el diff aún no añade pruebas. Hace falta al menos una regresión del loader que instancie el proxy correcto y demuestre que year=2024 busca/crea/escribe exclusivamente (cups_id, 2024); así ambos fallos anteriores dejarían de poder pasar inadvertidos.
He revisado los tres commits, el diff completo, las lecturas restantes de los campos migrados y git diff --check. Este último pasa. La CI de tests continúa omitida porque falta la etiqueta de merge, así que no aporta validación funcional en el estado actual.
giscebot
left a comment
There was a problem hiding this comment.
La última corrección resuelve los dos fallos funcionales señalados: el proxy ya es GiscedataCupsEstadistiques y UpdateCNMCStats respeta el year recibido (con el mismo valor por defecto para llamadas no CLI). También he comprobado el diff completo, que las lecturas migradas usan el registro anual del CUPS, que ya no quedan lecturas legacy de los campos retirados, el contrato de ensure_year(year, cups_ids=...) en gisce/erp#28653 y git diff --check.
Mantengo la solicitud de cambios por el bloqueo pendiente de la revisión anterior: el PR sigue sin añadir ninguna regresión para UpdateCNMCStats. Hace falta una prueba que demuestre que year=2024 busca el CUPS, asegura y actualiza únicamente (cups_id, 2024) —sin tocar otro ejercicio— y que, por tanto, falla si vuelve a ignorarse el año o se usa un proxy incorrecto. No es una petición cosmética: ambos defectos existieron en iteraciones de esta misma PR y actualmente la CI de tests está omitida por falta de la etiqueta de merge, así que no hay validación funcional ejecutada.
La ejecución local disponible tampoco sustituye esa cobertura: el descubrimiento con Python 3 no es válido para esta suite legacy (hay sintaxis Python 2 y faltan dependencias). Con la regresión específica añadida y la CI Python 2 pasando, no veo otro bloqueo en el cambio actual.
giscebot
left a comment
There was a problem hiding this comment.
Solicito cambios. La regresión añadida en 528e8bb resuelve correctamente el bloqueo de la revisión anterior: comprueba el proxy GiscedataCupsEstadistiques, respeta year=2024, ejecuta ensure_year, escribe únicamente el registro de 2024 y deja intacto 2023. También he verificado el diff completo, las lecturas de los campos migrados, git diff --check y la CI Python 2, que pasa.
El bloqueo actual es de compatibilidad y despliegue. La PR ERP relacionada gisce/erp#28653 fue cerrada sin merge porque la migración monolítica se fragmentó en verticales por campo; ya existen, entre otras, #28963 para la infraestructura y #28966 para cne_anual_reactiva_generada. Sin embargo, esta PR sigue cambiando de una vez los diez campos y deja de leerlos del CUPS sin fallback.
Así no hay un orden de despliegue seguro: al publicar esta versión para el primer vertical ERP, los campos todavía no migrados seguirán en giscedata.cups.ps, mientras F1/F1bis/F11/FA1/FA2 ya los buscarán en giscedata.cups.estadistiques; los formularios producirán ceros o vacíos aunque existan datos legacy. Esperar a los diez verticales tampoco permite que cada PR ERP lleve su consumidor completo ni desplegar los cortes de forma independiente.
Hay que alinear libCNMC con la estrategia campo a campo: separar cambios publicables por vertical, o introducir compatibilidad transitoria por campo (estadística anual cuando el campo/modelo esté disponible y fallback al CUPS mientras no lo esté), con pruebas que cubran el estado mixto. La prueba del loader añadida puede conservarse y reutilizarse en el vertical correspondiente.
giscebot
left a comment
There was a problem hiding this comment.
Solicito cambios. El nuevo review request apunta al mismo head 528e8bb53d3b revisado ayer; no hay cambios de código que resuelvan el bloqueo de despliegue.
He revalidado el contexto externo: la PR monolítica de ERP gisce/erp#28653 continúa cerrada sin merge, la infraestructura #28963 sigue abierta y el primer vertical de campo #28966 sigue abierto en draft. Por tanto, ERP continúa migrando campo a campo, mientras esta PR cambia a la vez los diez consumidores de libCNMC y no ofrece compatibilidad con el estado mixto.
El test del loader y la CI Python 2 siguen correctos, pero no eliminan este riesgo: publicar este head durante la secuencia de verticales haría que F1/F1bis/F11/FA1/FA2 leyesen del modelo anual campos que todavía permanecen únicamente en giscedata.cups.ps, produciendo ceros o vacíos.
Sigue siendo necesario separar los cambios de libCNMC por vertical o implementar un fallback transitorio por campo, con cobertura del esquema mixto. No detecto ningún bloqueo adicional sobre los ya indicados.
giscebot
left a comment
There was a problem hiding this comment.
Solicito cambios. La nueva iteración acierta al limitar la migración a FA1/FA2, restaurar los formularios antiguos y plantear compatibilidad transitoria por campo, pero el helper que implementa esa compatibilidad no puede funcionar contra el esquema real ni durante los despliegues verticales.
He revisado el diff completo en a0a0a835c0a2, el contrato del modelo en gisce/erp#28963, el primer vertical gisce/erp#28966, git diff --check y la CI Python 2 (verde). Dejo los detalles en línea:
- La búsqueda usa
any, pero el modelo anual defineyear; el RPC fallará por dominio sobre un campo inexistente antes de aplicar ningún fallback. - El helper lee primero todos los campos solicitados desde
giscedata.cups.ps. En cuanto un vertical retire uno de esos campos del CUPS, esereadfallará antes de consultar estadísticas. Hay que descubrir/separar qué campos existen en cada modelo y leer cada subconjunto de su origen disponible, preservando fallback por campo. - Las pruebas redefinen una copia local de
get_stats_valuesen vez de importarlibcnmc.utils.get_stats_values; por eso la suite verde no valida el código de producción y repite incluso el dominioany.
Con el helper corregido y pruebas que importen la función real y simulen un esquema mixto —un campo ya retirado del CUPS y disponible en estadísticas, y otro todavía legacy— no veo otro bloqueo.
giscebot
left a comment
There was a problem hiding this comment.
Apruebo esta iteración. Los bloqueos de la revisión anterior quedan resueltos:
get_stats_valuesbusca poryear, conforme al contrato degiscedata.cups.estadistiques.- Las regresiones importan y ejercitan la implementación real de
libcnmc.utils.get_stats_values. - El comportamiento mixto queda preservado campo a campo: el ORM omite del
readlos campos inexistentes en cada modelo, se mantienen los valores todavía presentes en CUPS y solo se superponen los valores devueltos por estadísticas anuales. - Los formularios antiguos quedan fuera del cambio y FA1/FA2 mantienen compatibilidad durante el despliegue vertical.
He revisado el diff completo en 302b3c2, el contexto de las PR ERP #28963 y #28966, los comentarios anteriores y el contrato del loader por año. También he ejecutado con Python 2 los 5 tests de test_get_stats_values.py y los 3 de test_update_cnmc_stats.py; todos pasan. git diff --check también pasa y el worktree queda limpio.
La ejecución completa de CI del head actual aparece omitida al no estar activa la etiqueta requerida; la última ejecución completa anterior estaba verde. Dado que los cambios posteriores son precisamente la corrección del dominio y la conexión de los tests a la función real, y esa cobertura focal pasa en el head actual, no veo un bloqueo funcional pendiente.
|
Post-merge review: PR #733 està mergejada a |
Descripcion
Ficheros modificados
Relacionado:
NOTA: