etalab / transport-site

Rendre disponible, valoriser et améliorer les données transports
https://transport.data.gouv.fr
194 stars 30 forks source link

Mise à jour job import IRVE vers geo_data #3794

Closed AntoineAugusti closed 7 months ago

AntoineAugusti commented 8 months ago

Il me semble que la ressource stable a été modifiée et qu'il faudrait répercuter ce changement dans le job d'import vers la carte d'exploration.

https://github.com/etalab/transport-site/blob/8fae723e8f52cace2e17cc30fe39094f0a967920/apps/transport/lib/jobs/geo_data/irve_to_geo_data.ex#L10

En plus je constate une erreur dans le job d'import actuellement dont voici la stracktrace

** (FunctionClauseError) no function clause matching in String.trim/1
The following arguments were given to String.trim/1:
# 1
nil

Attempted function clauses (showing 1 out of 1):
def trim(string) when -is_binary(string)-
(elixir 1.15.5) String.trim/1\n (transport 0.0.1) lib/jobs/geo_data/base.ex:79: Transport.Jobs.BaseGeoData.string_to_float/1
(transport 0.0.1) lib/jobs/geo_data/base.ex:58: Transport.Jobs.BaseGeoData.parse_coordinate/1
(transport 0.0.1) lib/jobs/geo_data/irve_to_geo_data.ex:30: anonymous fn/2 in Transport.Jobs.IRVEToGeoData.prepare_data_for_insert/2
(elixir 1.15.5) lib/stream.ex:613: anonymous fn/4 in Stream.map/2
(elixir 1.15.5) lib/enum.ex:4830: Enumerable.List.reduce/3
(elixir 1.15.5) lib/stream.ex:1027: Stream.do_transform_inner_list/7
(elixir 1.15.5) lib/stream.ex:1828: Enumerable.Stream.do_each/4\n"}, %{"at" => "2024-03-05T12:30:22.990358Z

cc @thbar @cyrilmorin

thbar commented 8 months ago

Merci @AntoineAugusti ! Je vais aller voir.

Si je comprends bien, l'impact c'est la carte d'exploration qui n'est plus rafraîchie, mais reste disponible.

Donc du coup urgence modérée, mais je vais aller voir à un moment. Merci !

ptitfred commented 7 months ago

De ce que j'intuite de ma navigation dans data.gouv.fr et l'explorateur, le uuid est désormais eb76d20a-8501-400e-b336-d85724de5435. Ceci correspond à la dernière version du schéma, la 2.3.1:

2024-04-02_184859

Avant de faire quoi que ce soit, j'ai donc les questions suivantes :

AntoineAugusti commented 7 months ago

Il faudrait utiliser la ressource qui utilise toujours la dernière version du schéma, eb76d20a-8501-400e-b336-d85724de5435, et qui est mise à jour de manière quotidienne. "Consolidation de la dernière version à date du schéma (v2.3.1)".

L'objectif est d'utiliser la dernière version du schéma et de disposer de données fraiches.

Il faudrait de plus comprendre pourquoi on avait eu une erreur d'import, qui semble avoir été causée par des coordonnées géographiques nil alors que les données consolidées sont censées être valides et donc exclure ces lignes.

thbar commented 7 months ago

Je partage quelques infos utiles (et Pierlou va m'aider à préciser ça, je l'ai contacté par DM).

La consolidation est actuellement configurée ici: https://github.com/datagouv/schema.data.gouv.fr/blob/916d572e2b55856db31aac7bfccd70f6d3d98dfb/config_consolidation.yml#L84-L116

On gagnera à expliciter et lisser ce fonctionnement car ce n'est pas tout le temps très clair pour le grand public (enfin les ré utilisateurs), voir par exemple: https://github.com/etalab/schema-irve/issues/57#issuecomment-1938260734.

Certains anciens fichiers restent mais ne sont plus mis à jour etc, il est "facile" de croire qu'on utilise quelque chose de mis à jour alors que ça n'est plus le cas, on va retravailler ça.

thbar commented 7 months ago

J'ai vérifié avec @Pierlou le fonctionnement actuel car il y avait eu des changements pour nous assister.

Normalement (la reformulation étant la mienne je précise):

Je pense que ma question a été répondue dans le sens où je n'avais pas vu que la 2.2.0 était toujours exposée mais ne tournait plus.

Voilà c'est un peu un work-in-progress, donc n'hésite pas si c'est pas clair !

ptitfred commented 7 months ago

Merci pour toutes ces précisions.

il y a une ressource stable qui correspond à la latest du schéma, quelle qu'elle soit, et qui pointera dessus toute seule

Un changement de schéma implique pourrait casser notre code d'import. Devrait-on utiliser le table-schema pour rendre le parsing plus solide et alerter en cas de "breaking change" ?


Par ailleurs, j'ai consulté le table-schema et les dernières colonnes du dataset de prod n'y sont pas spécifiées. J'ai l'impression qu'elles sont implicites parce qu'il s'agit d'un jeu de données data gouv (metadata) et parce qu'il inclut des données géographiques. Est-ce le cas ? En voici la liste pour contexte:

Pierlou commented 7 months ago

La ressource stable qui correspond toujours à la consolidation de la dernière version devrait faire l'affaire pour vos imports non ? Vous serez sûrs (et les autres utilisateurs aussi) qu'il n'y a pas de question à se poser en cas de montée de version. Pour les colonnes additionnelles, elles sont ajoutées lors de la consolidation par nos scripts afin d'enrichir le fichier consolidé

ptitfred commented 7 months ago

Pour les colonnes additionnelles, elles sont ajoutées lors de la consolidation par nos scripts afin d'enrichir le fichier consolidé

Merci @Pierlou :bow:

Par curiosité, cette consolidation est-elle documentée ?

Pierlou commented 7 months ago

Yes, le code est ouvert et disponible ici

thbar commented 7 months ago

La ressource stable qui correspond toujours à la consolidation de la dernière version devrait faire l'affaire pour vos imports non ? Vous serez sûrs (et les autres utilisateurs aussi) qu'il n'y a pas de question à se poser en cas de montée de version.

Merci @Pierlou! Oui ça me paraît très bien. D'ailleurs je vais m'en servir ailleurs dans le code à terme (pour la consolidation dynamique #3839). Ça serait top qu'on s'assure qu'il ne reste rien d'autre à côté car c'est "confusant", et peut-être qu'on peut communiquer dessus aussi (dans notre newsletter par exemple).

Pierlou commented 7 months ago

Une question d'ailleurs : est-ce que vous prévoyez de décommissionner les anciennes versions (aka les exclure de la config de consolidation) ? De notre côté, plus on a de versions à consolider, plus le script prend de temps à tourner, vu qu'on appelle Validata sur chaque fichier pour chaque version non-exclue (donc du temps/des ressources)

thbar commented 7 months ago

est-ce que vous prévoyez de décommissionner les anciennes versions (aka les exclure de la config de consolidation) ?

@Pierlou pour moi je ne vois aujourd'hui aucun intérêt à avoir plusieurs consolidations en parallèle (c'est pas lisible pour l'extérieur et même pour nous, cf la discussion courante). Ça pourrait avoir un intérêt ponctuel lors d'une mise à jour majeure du schéma, non rétro-compatible, mais jusque là et depuis la v2, le cas ne s'est pas présenté si je me souviens bien.

J'irais jusqu'à dire que si on pouvait auto-décommissionner les anciennes versions en systématique, ça m'irait très bien!

Je copie-colle l'état actuel d'ailleurs ici:

CleanShot 2024-04-03 at 14 06 27@2x

Un changement de schéma implique pourrait casser notre code d'import. Devrait-on utiliser le table-schema pour rendre le parsing plus solide et alerter en cas de "breaking change" ?

@ptitfred à court terme ça me paraît overkill car le schéma bouge pour l'instant très doucement et c'est nous qui contrôlons le schéma par ailleurs. Une note dans le code devrait suffire à court terme, et du code défensif par ailleurs.

Par ailleurs, j'ai consulté le table-schema et les dernières colonnes du dataset de prod n'y sont pas spécifiées. J'ai l'impression qu'elles sont implicites parce qu'il s'agit d'un jeu de données data gouv (metadata) et parce qu'il inclut des données géographiques. Est-ce le cas ? En voici la liste pour contexte:

Bonne remarque - je n'ai pas regardé ces colonnes extra récemment, il faudra qu'on en reparle et voir ce qu'on s'autorise dessus.

J'ai un peu une question similaire avec #3839 où j'aimerais avoir la possibilité d'intégrer une colonne supplémentaire pour tracer le jeu d'origine, et que le résultat reste valide !

Pierlou commented 7 months ago

J'irais jusqu'à dire que si on pouvait auto-décommissionner les anciennes versions en systématique, ça m'irait très bien!

Une Github action qui, après release d'une nouvelle version IRVE, ajoute l'ancienne version à la liste des exclusions ? 🤔

ptitfred commented 7 months ago

Bonne remarque - je n'ai pas regardé ces colonnes extra récemment, il faudra qu'on en reparle et voir ce qu'on s'autorise dessus.

(Pierlou y a répondu.)

thbar commented 7 months ago

J'irais jusqu'à dire que si on pouvait auto-décommissionner les anciennes versions en systématique, ça m'irait très bien!

Une Github action qui, après release d'une nouvelle version IRVE, ajoute l'ancienne version à la liste des exclusions ? 🤔

C'est une idée, on va y réfléchir ; pour l'instant on va juste mettre une TODO dans notre processus de release et on verra à quelle fréquence ça évolue !

Pierlou commented 7 months ago

OK, du coup en parallèle si vous voulez on peut aussi exclure les anciennes versions, vous me dites

thbar commented 7 months ago

OK, du coup en parallèle si vous voulez on peut aussi exclure les anciennes versions, vous me dites

Go pour moi ! Ça va supprimer les ressources pré-existantes c'est bien ça ? Poke @stephane-pignal on aura peut-être un peu de contacts support suite à ça mais ça nous va.

Pierlou commented 7 months ago

Non c'est indépendant, les ressources sur data.gouv correspondant à des versions exclues ne sont juste plus mises à jour (vous pourrez les supprimer si vous le souhaitez, ce sera à votre main)

thbar commented 7 months ago

Ok ! On va faire le ménage de notre côté, je crée un ticket spécifique. Merci !

thbar commented 7 months ago

Ticket pour le ménage :

@Pierlou merci pour tes interventions, ça va aider à rendre la situation plus claire !

ptitfred commented 7 months ago

En plus je constate une erreur dans le job d'import actuellement dont voici la stracktrace

Ceci semble se manifester uniquement si consolidated_longitude ou consolidated_latitude sont manquants.

A date du 8 avril 2024, le fichier contient bien ces colonnes (le contraire signalerait un défaut du script de consolidation). Pour s'en assurer, en utiliser csvgrep:

$ csvgrep -c consolidated_longitude,consolidated_latitude -r . -i \
   path/to/consolidation-etalab-schema-irve-statique-v-2.3.1-20240402.csv | wc -l
1 # 1 seul ligne car csvgrep rappelle les headers, mais aucune ligne ne présente ces colonnes comme vide
ptitfred commented 7 months ago

Et effectivement les données de 2.2.0 ne contiennent pas les colonnes "consolidated".