Premiers extracteurs NeTEx (StopPlaces) #4026
Conversation
thbar
changed the title
|
Ca faisait 15 ans que j'avais pas vu un parser Sax et je suis déçu de voir que c'est toujours aussi pénible à review et maintenir :/ |
|
@ptitfred je pense avoir des idées pour rendre le tout plus lisible dans un prochain tour (ou avec Saxy, ou avec d’autres librairies), donc ne perdons pas espoir 😊 |
| :zip.create( | ||
| zip_filename, | ||
| file_data | ||
| |> Enum.map(fn {name, content} -> {name |> to_charlist(), content} end) | ||
| ) |
There was a problem hiding this comment.
Peut-être stocker en mémoire et non sur le disque ? Semble possible en lisant https://www.erlang.org/doc/apps/stdlib/zip.html#zip/2
Permettra de ne pas laisser des fichiers dans le tmp ou de devoir penser à nettoyer
There was a problem hiding this comment.
Peut-être stocker en mémoire et non sur le disque ? Semble possible en lisant https://www.erlang.org/doc/apps/stdlib/zip.html#zip/2
Merci pour la suggestion. En fait, notre "traversal de zip" est conçu pour travailler avec des fichiers sur disque uniquement, afin d'éviter une surcharge de la RAM en production (certains fichiers dépassent 500 MB). Le stockage en mémoire n'est donc pas adapté ici.
La codebase inclut plusieurs tests qui ne nettoient pas automatiquement les fichiers tmp. Un helper de nettoyage pourrait être utile à l'avenir (comme Dir.mktmpdir en Ruby), c'est une idée qu'on peut conserver !
Pour éviter tout souci, j'ai toutefois ajouté de l'unicité dans le commit e2ff1c4.
apps/transport/test/netex/stop_places_streaming_parser_test.exs
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| def handle_event(:characters, chars, state) | ||
| when state.current_tree == ["StopPlace", "Centroid", "Location", "Latitude"] do |
There was a problem hiding this comment.
On ne gère pas le cas d'une frame englobante ? Me semble qu'on n'aurait pas exactement ce tree alors
There was a problem hiding this comment.
Merci pour la question ; l'implémentation actuelle va bien gérer correctement tout ce qui est frame englobante au dessus (le StopPlace peut être à n'importe quel niveau des profondeur dans le XML), pas de souci du coup.
Pour mettre ce point plus clairement en évidence (et éviter les régressions), j'ai modulé un peu le test ici 8dba0bf.
| tmp_file = System.tmp_dir!() |> Path.join("temp-netex.zip") | ||
| ZipCreator.create!(tmp_file, [{"arrets.xml", some_netex_content()}]) | ||
|
|
||
| # given a zip netex archive containing 2 files, I want the output I expected |
There was a problem hiding this comment.
Seulement 1 fichier il me semble ?
There was a problem hiding this comment.
Tout à fait, merci pour le catch. Corrigé dans 5858db6
| extension = Path.extname(file_name) | ||
|
|
||
| cond do | ||
| # Entry names ending with a slash `/` are directories. Skip them. | ||
| # https://github.com/akash-akya/unzip/blob/689a1ca7a134ab2aeb79c8c4f8492d61fa3e09a0/lib/unzip.ex#L69 | ||
| String.ends_with?(file_name, "/") -> | ||
| [] | ||
|
|
||
| extension |> String.downcase() == ".zip" -> | ||
| raise "Insupported zip inside zip for file #{file_name}" | ||
|
|
||
| extension |> String.downcase() != ".xml" -> | ||
| raise "Insupported file extension (#{extension}) for file #{file_name}" |
There was a problem hiding this comment.
Peut-être déplacer ceci ailleurs, après Unzip.list_entries ? On risque d'avoir cette logique à d'autres endroits et ceci ne semble pas spécifique aux stop places
There was a problem hiding this comment.
Merci pour la suggestion. J'envisagerai ça quand on généralisera le code (pour l'instant très spécifique stop places), j'aurai davantage de recul à ce moment là.
| @@ -0,0 +1,46 @@ | |||
| defmodule Transport.NeTEx.ArchiveParserTest do | |||
There was a problem hiding this comment.
J'aurais tendance à mettre les "helper methods" après les tests pour mettre en évidence le fonctionnement testé
There was a problem hiding this comment.
Je me souviens que tu as levé ce point quelques fois.
Le lien que tu as partagé montre que c'est une question de préférence personnelle.
(d'ailleurs personnellement je trouve ça plus simple cognitivement, dans les tests, où ce code n'est pas réutilisé ailleurs, d'avoir les helpers défini avant leur utilisation, ou encore mieux, dans des modules externes si on peut, plutôt qu'en bas).
Si ce point t'embête et vu que tu l'as évoqué quelques fois déjà, parlons-en tranquille à un point dév pour voir si ça vaut le coup d'homogénéiser les tests là dessus, si tu le souhaites.
|
|
||
| netex = | ||
| df | ||
| |> Task.async_stream( |
There was a problem hiding this comment.
Il semble qu'on avait un disk cache helper lors de précédentes PRs pour le script. Je confonds ?
There was a problem hiding this comment.
Il y a bien un helper de ce type (ici: https://github.com/etalab/transport-site/blob/master/apps/shared/lib/req_custom_cache.ex) mais il n'est pas adapté à l'usage ici. Le cache évoqué dans ce lien stocke la totalité de l'objet réponse Elixir Req sur le disque, ce qui empêche d'avoir "juste le body zip" à part.
Pour faire plus simple pour le moment et reporter ce refactoring intéressant, j'ai préféré ne pas modifier le cache, et réimplémenter, le temps d'avoir + de recul et de pouvoir généraliser et modifier le cache (éventuellement).
| IO.puts("Processing file #{r.id}") | ||
|
|
||
| try do | ||
| count = | ||
| Transport.NeTEx.read_all_stop_places(r.local_path) | ||
| |> Enum.flat_map(fn {_file, stops} -> stops end) | ||
| # some stop places have no latitude in NeTEx | ||
| |> Enum.reject(fn p -> is_nil(p[:latitude]) end) | ||
| |> Enum.count() | ||
|
|
||
| IO.puts("#{count} StopPlaces detected") | ||
| rescue | ||
| e -> IO.puts("Som'thing bad happened") |
There was a problem hiding this comment.
Tu aurais un output à partager ?
There was a problem hiding this comment.
Il y a des cas variés, je préfère ne pas faire de détail pour l'instant car ça prendrait un brin de temps, une prochaine itération capturera ça en détail et j'aurai alors l'opportunité de partager ça (notamment pour améliorer le parser, certains fichiers ne passant pas).
Je contribuerai sur:
et j'en profiterai alors pour ou bien traiter ces cas, ou bien les afficher clairement.
Co-authored-by: Antoine Augusti <[email protected]>
Co-authored-by: Antoine Augusti <[email protected]>
There was a problem hiding this comment.
Merci pour le retour détaillé @AntoineAugusti ; j'ai pris en compte ou répondu !
| extension = Path.extname(file_name) | ||
|
|
||
| cond do | ||
| # Entry names ending with a slash `/` are directories. Skip them. | ||
| # https://github.com/akash-akya/unzip/blob/689a1ca7a134ab2aeb79c8c4f8492d61fa3e09a0/lib/unzip.ex#L69 | ||
| String.ends_with?(file_name, "/") -> | ||
| [] | ||
|
|
||
| extension |> String.downcase() == ".zip" -> | ||
| raise "Insupported zip inside zip for file #{file_name}" | ||
|
|
||
| extension |> String.downcase() != ".xml" -> | ||
| raise "Insupported file extension (#{extension}) for file #{file_name}" |
There was a problem hiding this comment.
Merci pour la suggestion. J'envisagerai ça quand on généralisera le code (pour l'instant très spécifique stop places), j'aurai davantage de recul à ce moment là.
| end | ||
|
|
||
| def handle_event(:characters, chars, state) | ||
| when state.current_tree == ["StopPlace", "Centroid", "Location", "Latitude"] do |
There was a problem hiding this comment.
Merci pour la question ; l'implémentation actuelle va bien gérer correctement tout ce qui est frame englobante au dessus (le StopPlace peut être à n'importe quel niveau des profondeur dans le XML), pas de souci du coup.
Pour mettre ce point plus clairement en évidence (et éviter les régressions), j'ai modulé un peu le test ici 8dba0bf.
| @@ -0,0 +1,46 @@ | |||
| defmodule Transport.NeTEx.ArchiveParserTest do | |||
There was a problem hiding this comment.
Je me souviens que tu as levé ce point quelques fois.
Le lien que tu as partagé montre que c'est une question de préférence personnelle.
(d'ailleurs personnellement je trouve ça plus simple cognitivement, dans les tests, où ce code n'est pas réutilisé ailleurs, d'avoir les helpers défini avant leur utilisation, ou encore mieux, dans des modules externes si on peut, plutôt qu'en bas).
Si ce point t'embête et vu que tu l'as évoqué quelques fois déjà, parlons-en tranquille à un point dév pour voir si ça vaut le coup d'homogénéiser les tests là dessus, si tu le souhaites.
| tmp_file = System.tmp_dir!() |> Path.join("temp-netex.zip") | ||
| ZipCreator.create!(tmp_file, [{"arrets.xml", some_netex_content()}]) | ||
|
|
||
| # given a zip netex archive containing 2 files, I want the output I expected |
There was a problem hiding this comment.
Tout à fait, merci pour le catch. Corrigé dans 5858db6
| IO.puts("Processing file #{r.id}") | ||
|
|
||
| try do | ||
| count = | ||
| Transport.NeTEx.read_all_stop_places(r.local_path) | ||
| |> Enum.flat_map(fn {_file, stops} -> stops end) | ||
| # some stop places have no latitude in NeTEx | ||
| |> Enum.reject(fn p -> is_nil(p[:latitude]) end) | ||
| |> Enum.count() | ||
|
|
||
| IO.puts("#{count} StopPlaces detected") | ||
| rescue | ||
| e -> IO.puts("Som'thing bad happened") |
There was a problem hiding this comment.
Il y a des cas variés, je préfère ne pas faire de détail pour l'instant car ça prendrait un brin de temps, une prochaine itération capturera ça en détail et j'aurai alors l'opportunité de partager ça (notamment pour améliorer le parser, certains fichiers ne passant pas).
Je contribuerai sur:
et j'en profiterai alors pour ou bien traiter ces cas, ou bien les afficher clairement.
|
|
||
| netex = | ||
| df | ||
| |> Task.async_stream( |
There was a problem hiding this comment.
Il y a bien un helper de ce type (ici: https://github.com/etalab/transport-site/blob/master/apps/shared/lib/req_custom_cache.ex) mais il n'est pas adapté à l'usage ici. Le cache évoqué dans ce lien stocke la totalité de l'objet réponse Elixir Req sur le disque, ce qui empêche d'avoir "juste le body zip" à part.
Pour faire plus simple pour le moment et reporter ce refactoring intéressant, j'ai préféré ne pas modifier le cache, et réimplémenter, le temps d'avoir + de recul et de pouvoir généraliser et modifier le cache (éventuellement).
| :zip.create( | ||
| zip_filename, | ||
| file_data | ||
| |> Enum.map(fn {name, content} -> {name |> to_charlist(), content} end) | ||
| ) |
There was a problem hiding this comment.
Peut-être stocker en mémoire et non sur le disque ? Semble possible en lisant https://www.erlang.org/doc/apps/stdlib/zip.html#zip/2
Merci pour la suggestion. En fait, notre "traversal de zip" est conçu pour travailler avec des fichiers sur disque uniquement, afin d'éviter une surcharge de la RAM en production (certains fichiers dépassent 500 MB). Le stockage en mémoire n'est donc pas adapté ici.
La codebase inclut plusieurs tests qui ne nettoient pas automatiquement les fichiers tmp. Un helper de nettoyage pourrait être utile à l'avenir (comme Dir.mktmpdir en Ruby), c'est une idée qu'on peut conserver !
Pour éviter tout souci, j'ai toutefois ajouté de l'unicité dans le commit e2ff1c4.
|
Merci @AntoineAugusti pour la review ! |
Cette PR introduit une couche fondamentale pour réaliser différentes choses plus tard:
StopPlaceouQuayetc dans un NeTEx...)Les limitations sont documentées sur les modules directement, et le code est testé pour sa plus grande partie.
J'ai intégré un script qui me sert à parcourir la totalité des NeTEx du PAN, que je ferai évoluer.
TODOs
Code de traversal d'archive à rendre moins "soudé" au use-case stop places- ça attendra un prochain herenetex_archive_parser.ex(actuellement 0%, embêtant pour refactorer)