Python 3 support

[fsteggink]

Ondersteuning in Python 2 vervalt komend jaar (in 2020): https://www.python.org/dev/peps/pep-0373/#update
Tijd om te herschrijven / testen. Ook investeren wat de stand van zaken in de verschillende distro's is. Mogelijk een goed idee om BAG 2.0 van scratch af aan met Python 3 te ontwikkelen?

Zie voor de Stetl-gebaseerde ETL's ook geopython/stetl#23

[justb4]

Ja, zeker als we BAG 2.0 in Stetl herschrijven. @borrob is al vrij ver, via PR geopython/stetl#81 maar ligt even stil, iedereen druk en dit zijn natuurlijk geen spannende functionele wijzigingen. Goed dat je eraan herinnert!

Tenminste daar ga ik nu van uit (BAG 2.0 in Stetl ipv custom program).
Dan is alle ETL in NLExtract uniform en mochten we ooit een (Web)UI maken dan hoeft die maar 1x ontwikkeld.

[fsteggink]

Nog geen activiteit op dit vlak. Misschien kunnen we beter BAG-Extract met het mininmaal noodzakelijke porten naar Python 3. Herschrijven naar Stetl is naar verwachting veel lastiger en mogelijk nog niet eens mogelijk, v.w.b. mutatiebestanden.

[borrob]

Ai, deze is ook helemaal van mij radar verdwenen, maar 2020 komt natuurlijk al in zicht. Is het de bedoeling om eerst alles om te zetten naar python3 en daarna BAG2.0 (#259) op te pakken?

Zijn er mensen die naar FOSS4G gaan, zodat er een plek te claimen is op de code sprint ? Of nog een keer een losse code dag organiseren?

[justb4]

Ik zie wel mogelijkheden om BAG met Stetl om te zetten, zelfs mutatie-bestanden. Het huidige ontwerp heeft al een redelijk strikte scheiding tussen, inlezen, datastructuur opbouwen en uitvoer. Inlezen past mooi op Stetl Inputs, datastructuur (objecten uit "BAG" classes) kan mooi in opaque Stetl "data" element en uitvoer in Stetl Outputs. Natuurlijk worden dat wel custom Input/Outputs en moet mogelijk datastructuur herzien (is nu performance/geheugen bottleneck).

Ik ben ook op FOSS4G t/m za (codesprint), hoewel rond 16:00 vliegtuig terug heb. Denk, gezien de taken, een losse dag(en), liefst natuurlijk gesponsored.

[fsteggink]

Ik denk dat we dit gescheiden moeten oppakken, zodat e.e.a. niet afhangt van elkaar. Ik wil graag zelf binnenkort helemaal overschakelen op Python 3, maar de behoefte om BAG o.b.v. Stetl te draaien is veel minder.
Helaas ben ik niet in Boekarest aanwezig.

[botenvouwer]

Momenteel ben ik bezig met het opzetten van een bgt extract met de stetl image waarin python3 gebruikt wordt. Hierdoor ontstaan een paar problemen met de python code van nlextract. Voor de bgt heb ik die opgelost. Voor de anderen ga ik dat waarschijnlijk ook doen. Als ik alles werkend en getest heb deel ik het graag :).

[borrob]

Cool! Ik heb destijds de conversie voor Stetl gemaakt en ik ben blij dat er nu ook aan NLExtract gewerkt wordt. Succes en mocht het nodig zijn, dan denk ik nog graag even mee.

[botenvouwer]

@borrob Wel allicht weet jij misschien waar deze fout vandaan kan komen, in python3 loopt het subfeaturehandler.py bij sommige batches vast. In pyhton2 gebeurt dit niet.

Zie meer info https://geoforum.nl/t/pdok-gml-naar-postgis-of-de-data-is-incompleet-of-ik-doe-iets-fout/3213/8

[borrob]

Ik heb even een gist gemaakt https://gist.github.com/borrob/6675d05f3f408601f68a6c093ff1fb6f met een opzet die bij mij werkt. Het is een minimale versie van de subfeaturehandler.py en het gebruikt als input de bgt_pand.gml dat ik van jouw (?) dropbox heb via https://www.dropbox.com/s/2co5yx06h8vra4f/bgt_pand.gml?dl=0

Op mijn Mac (python 3.7.3) heb ik in een verse virtual env pip install lxml gedraaid en daarna het script uit de gist. Ik krijg daarmee geen error.

Als alternatief kun je proberen om de xf.flush() op het einde te verwijderen en eventueel daarvoor in de plaats buffered=False gebruiken in het with statement bij het aanmaken van de temp-file.

[botenvouwer]

@borrob Ik zal het ook even zo los proberen. Wel raar dat het bij jou wel werkt dan. Het gaat (om precies te zijn) om de bgt_pand.gml uit bgt_148.zip te downloaden via: https://downloads.pdok.nl/service/extract.zip?extractname=bgt&extractset=citygml&excludedtypes=plaatsbepalingspunt&history=true&tiles=%7B%22layers%22%3A%5B%7B%22aggregateLevel%22%3A4%2C%22codes%22%3A%5B$148%5D%7D%5D%7D&enddate=23-09-2019

[borrob]

@borrob Ik zal het ook even zo los proberen. Wel raar dat het bij jou wel werkt dan. Het gaat (om precies te zijn) om de bgt_pand.gml uit bgt_148.zip te downloaden via: https://downloads.pdok.nl/service/extract.zip?extractname=bgt&extractset=citygml&excludedtypes=plaatsbepalingspunt&history=true&tiles=%7B%22layers%22%3A%5B%7B%22aggregateLevel%22%3A4%2C%22codes%22%3A%5B$148%5D%7D%5D%7D&enddate=23-09-2019

Snelle test: ook die gml geeft bij mij geen error.

[botenvouwer]

@borrob Ben er nu mee bezig, zal even verder reageren in de gist.

[justb4]

@borrob @botenvouwer benieuwd of jullie eruit komen. Ik kan inhoudelijk niet veel bijdragen, lijkt mij wel goed als jullie exact de versies (Python, lxml, libxml2 etc) en platformen doorgeven. Daar kunnen ook verschillen uit komen. Ik zie in de etree API idd iets over async en buffered=False (maar ook een flush() na elke write() ipv aan eind van with).

[justb4]

#276 is een PR hiervoor, voor BAG gedeelte althans.

[fsteggink]

Voor het issue van @botenvouwer heb ik op 16 dec een commit gedaan: b1f7793. Het bleek een "bug" (of feature?) in lxml te zijn. Als return code van een bepaalde functie wordt een signed 32 bit integer teruggegeven dat het aantal weggeschreven bytes bevat. Nu wordt er in sommige gevallen (bij grote XML bestanden) meer van 2*10^9 bytes weggeschreven, waardoor de, door de afkapping ontstane negatieve waarde, als een foutcode in libxml wordt gezien. De workaround is deze fout te negeren. Niet helemaal netjes, maar dit moet m.i. in libxml opgelost worden en ik schat in dat dat niet heel gauw gaat gebeuren, laat staan dat deze fix overal beschikbaar komt...

[fsteggink]

Dit is de betreffende melding in de bug tracker van lxml: https://bugs.launchpad.net/lxml/+bug/1570388. Ik heb hier mijn bevindingen aan toegevoegd, maar tot dusver geen reactie.

[fsteggink]

Heb apart issue aangemaakt om documentatie bij te werken: #278.
Verder moeten er, naast de Python3 upgrade, ook e.e.a. aan de repo gebeuren. In het BGT script ga ik er nog steeds van uit dat Stetl via een external Git dependency wordt benaderd, maar het lijkt me veel logischer om Stetl, evenals andere dependencies, via pip te installeren.

[justb4]

Ja volgens mij is de laatste Python 3 upgrade alleen voor de specifieke NLExtract BAG code gedaan. De overige datasets die met Stetl gedaan worden, moeten ook een upgrade krijgen. Denk dat eerst Stetl, m.n. de Docker Image naast Python 3 ook naar GDAL 3 en overige upgrades (lxml?) gemigreerd moet worden en getest in NLExtract. Heb begrepen dat verschillende partijen dat in eigen private repos al hebben gedaan, maar zie graag dat dit gedeeld wordt in zowel de Stetl repo als in NLExtract...

[fsteggink]

Gisteravond ben ik bezig geweest. Er zijn maar een paar wijzigingen nodig in de niet-BAG code. Ik ga hiervoor een PR indienen. Vraag: wat doen met de extenals? Eruit mikken? In een apart PR?

Het uitvoeren van NLExtract is een ander verhaal. Ik weet niet hoe ik het beste het stetl-script (dat ik nu met pip heb geïnstalleerd) kan uitvoeren. Ik wil niet een apart wrapper Python script maken waarmee ik stetl kan importeren. Ook wil ik niet een Docker-container hiervoor aanroepen. Dat werkt wel, maar ik wil ook direct op mijn machine Stetl kunnen aanroepen.

Momenteel gebruik ik dit:

set options=options\STEGGINK-LAPTOP.args
set pythonpath=.;..\..
python c:\Users\Frank\Envs\nlextract\Lib\site-packages\stetl\main.py -c conf\etl-imgeo-v2.1.1.cfg -a %options_file%

Ook wil ik een andere oplossing verzinnen voor pythonpath. Dat is nu nodig zodat Stetl de NLExtract-specifieke filters kan vinden (subfeaturehandler en gfspreparationfilter). Merk op dat ik voor TOP10NL een ander pythonpath nodig heb, omdat de configuratiebestanden 1 directory dieper zitten.

Ter info, de requirements in mijn virtualenv:

Deprecated==1.2.7
GDAL==2.4.1
Jinja2==2.11.1
lxml==4.5.0
MarkupSafe==1.1.1
psycopg2==2.8.4
Stetl==2.0
wrapt==1.12.1

GDAL heb ik, als Windows gebruiker, hier vandaan. Voor de rest geen verschil met een Linux omgeving 🎉

[fsteggink]

Ik heb er wel bewust voor gekozen om GDAL 2 te blijven gebruiken. Ik vind dat een upgrade hiervan later moet gebeuren. Dit in het kader van niet teveel onderdelen gelijk vervangen.

[fsteggink]

Het uitvoeren van Stetl als een script kan ik helaas alleen doen door het een .py extensie te geven. Op Windows wordt de shebang die in executable scripts staan niet uitgelezen. Ook al verwijst hij naar de Python executable van mijn virtualenv. (Heeft pip er waarschijnlijk ingezet.)
Echter, ik krijg een melding dat stetl.main niet geimporteerd kan worden. Wel als ik in mijn virtualenv een interactieve Python shell open. Na analyse van sys.path en sys.executable (even als debug statements in stetl(.py)) gezet, bleek de Python executable van Python 2.7 gebruikt te worden. Het bleek dat de py extensie hiermee werd geassocieerd. In Python 2.7 heb ik geen stetl geïnstalleerd, dus niet gek dat de import niet slaagt. Ik ga dit fixen, maar ben er niet gerust op dat dit gaat werken, omdat dan nog steeds de "generieke" python executable wordt gebruikt, terwijl voor het uitvoeren van dit script ik toch echt de python exe van mijn virtualenv nodig heb...

V.w.b. het stetl-script, is het mogelijk om hier een .py extensie aan te geven? Ik kan dit eventueel als issue opvoeren bij Stetl. Ik zag ook dat de Stetl test scripts als een aparte module "tests" geïnstalleerd werden. Dit gebeurt met Stetl 2.0.

Anyways, dit heeft niet echt met Python 3 migratie te maken, maar ik verwacht dat andere Windows gebruikers hier ook tegenaan gaan lopen, dus het is wel goed om deze kennis alvast ergens vast te leggen. T.z.t. zullen dit soort gotcha's in de documentatie moeten worden vermeld.

[fsteggink]

Installatie klaar. Blijkt wel te werken. Na verwijderen van Python 2.7 en herinstallatie van Python 3.7 en hierbij py-bestanden met de py-launcher te associeren, werkt dit ook goed voor mijn virtualenv. 🎉

Nu nog ervoor zorgen dat de NLExtract toevoegingen op sys.path terecht komen. Ik denk dat ik hiervoor het beste de etl-xxx.cmd/sh scripts kan aanpassen. Het bepalen van STETL_HOME en aanroepen als "python %STETL_HOME%\stetl\main.py" kunnen worden vervangen door simpelweg "stetl.py". Ik zou eventueel nu de extensie kunnen weglaten, maar dat werkt niet meer op Unix-gebaseerde systemen indien het stetl-script een py-extensie krijgt.

Wat mij betreft kan externals/stetl verdwijnen. Dat vond ik sowieso heel lastig te onderhouden. Door Stetl als een pip-package te gebruiken ben je, zeker als je af en toe een oude versie nodig hebt, veel flexibeler. En mocht het om wat voor reden nodig zijn (als ik zelf met Stetl aan het rommelen ben, bijv.), dan kan ik nog altijd Stetl met de hand in de site-packages van mijn virtual env kopiëren.

[fsteggink]

Nog een aanvulling: ik had ook een keer een Python 2.7 virtualenv aangemaakt. Die blijkt nog te werken, ook al heb ik Python 2.7 zelf verwijderd. Het is wel een net iets andere versie (2.7.14). Volgens mij de actuele versie toen ik virtualenv installeerde.

Ook dit is nuttige info voor de troubleshooting sectie voor Windows gebruikers.

[fsteggink]

Wat gewoon (nog steeds) werkt is de environment variabele STETL_HOME zetten en dan etl-imgeo.cmd/sh aanroepen. Alleen wanneer STETL_HOME niet is gezet, wordt de (oude) Stetl-versie uit externals gebruikt. En dan is het ook makkelijker om een andere Stetl-installatie te gebruiken. In de documentatie zouden we dit als alternatieven kunnen noemen.

We hebben dus de volgende mogelijkheden:

1. STETL_HOME niet zetten: dan moeten we externals blijven updaten. I.i.g. bij releases moeten we er scherp op zijn dat we dit doen. Voordeel is dat de etl-commando's het "out of the box" doen. Gebruikers hoeven dan niet zelf met STETL_HOME en PYTHONPATH aan de slag.
2. STETL_HOME verwijzen naar de site-packages dir van je Python-installatie of virtualenv.
3. STETL_HOME verwijzen naar een aparte Stetl-installatie (bijv. als je Stetl ook vanuit Github hebt gecloned).
4. Het Stetl-script gebruiken, mits die voor Windows-gebruikers is hernoemd naar stetl.py. In dit geval direct "stetl.py" aanroepen en niet "python stetl/main.py". En je moet ervoor zorgen dat je niet allerlei installaties door elkaar hebt staan, maar dat ligt aan de gebruiker zelf ;)

Bij gevallen 2 en 3 hoef je nog steeds geen PYTHONPATH in te stellen. En in alle gevallen zouden verwijzingen naar GDAL en andere dependencies gewoon moeten blijven werken, omdat ze vanuit je Python-installatie of virtualenv in sys.path terecht komen. Bij geval 2 en 4 moet je via pip Stetl installeren. En bij geen van de mogelijkheden is virtualenv verplicht, maar wel aan te bevelen.

Eigenlijk impliceer ik hiermee dat we de externals toch moeten blijven houden. Dat lijkt me geen probleem als we de documentatie actualiseren en ook in de readme's bij de betreffende ETLs. Wat vind jij, @justb4 ?

[justb4]

@fsteggink ja laatste optie lijkt mij beste (Stetl submodule): dan heb je altijd werkend geheel bij uitchecken en versies, kan bijv een fix in Stetl zijn op master, maar voor nu denk ik Stetl v2.

Uiteindelijk zou een pip install nlextract, die weer pip install stetl doet, handig zijn. Maar met al die native afhankelijkheden en platformen lijkt dat een brug te ver. Mijn aanbeveling en hoop is toch dat we alleen nog Docker releases hoeven te doen....

[fsteggink]

Ik heb tests gedaan met de ETL's. In de BRT ETL's heb ik nog op een aatal plekken de stetl-namespace moeten toevoegen. De ETL's lijken bij mij allemaal goed te functioneren, incl. de BAG.

[fsteggink]

Ik heb DB dumps gemaakt van de datasets en deze verder geanalyseerd met mijn eigen tooling. De enige bevinding die ik heb is dat er in de BAG geen verblijfsobjecten met vlakken zitten. Waarschijnlijk komt dit, omdat ik de Amstelveen-dataset heb gebruikt i.p.v. de volledige BAG-dump. Bij de BGT en TOP10NL t/m TOP1000NL heb ik geen fouten gevonden. Voor de DKK heb ik een andere variant gebruikt.

[fsteggink]

Zie ook de commentaren bij commit 1da951c.

[fsteggink]

Tenslotte nog nader gekeken naar de panden + nummeraanduidingen alsmede de openbareruimtelabels van de BGT. Dit i.v.m. de aanpassingen in de wijzigingen in de (BGT) subfeaturehandler. Dit ziet er allemaal goed uit in mijn QGIS-bestanden.
Wat mij betreft kunnen deze wijzigingen live, maar het is i.m.o. beter om het delete-duplicates script van de BGT in zijn geheel te verwijderen.

[justb4]

Kon al enige tijd gesloten o.a. vanwege PR #291.