basrieter
commented
5 years ago Closed basrieter closed 5 years ago
basrieter
commented
5 years ago basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Ik geef je helemaal gelijk, in een ideale wereld, maar
Dus ik zou veel liever een lightweight requests library gebruiken die gewoon efficient werkt, maar dat is er niet. En er is geen alternatief als Python libraries op ieder moment geupdate worden zonder dat er enige quality/testing wordt gedaan. Rechtstreeks naar de eindgebruiker. Miljoenen toestellen werken plots een pak trager.
Bovendien is de vorige requests library ook al een pak trager dan vroeger.
We hebben trouwens nog andere wijzigingen aangebracht aan onze codebase op basis van de analyses bij het profilen van onze plugin in Kodi. Imports van 3rd party libraries gebeuren nu niet meer op module-niveau, maar in de functie waar ze nodig is. Lijkt slecht design, maar voor Kodi addons is dat eigenlijk het meest efficiënt (code-paths lopen quasi altijd éénmalig, dus alle modules altijd laden is weinig efficiënt)
basrieter
commented
5 years ago basrieter
commented
5 years ago basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Ok. Er is meer dan 1 probleem IMO.
Updates die rechtstreeks naar eindgebruikers gaan en directe impact hebben
Voor 2a. hebben wij meteen onze code aangepast omdat het niet duidelijk was of het om een bug ging, en hoe snel er een fix zou komen. Voor 2b. idem, daar is het zelf nog twijfelachtig of het om een bug gaat, ook daar willen we niet op wachten. (Er waren al mensen die klaagden over vertragingen van nieuwere versies en al die reports zijn gewoon gesloten)
Ik kan begrijpen dat als er security problemen zijn met een oudere library dat men die wil upgraden, maar even goed moet er gekeken worden naar impact en scope van een upgrade.
Ik denk dat het governance model eens moet herbekeken worden als we een stabielere omgeving willen voor eindgebruikers. Als plugin developer bouwen we alle mogelijk resilience in in de plugin zodat de gebruiker zo weinig mogelijk impact heeft, en dan zijn we toch vatbaar voor roekeloze zaken als dit.
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
De (tijdelijke) oplossing voor bovenstaand probleem is een manuele installatie van: https://mirrors.kodi.tv/addons/krypton/script.module.urllib3/script.module.urllib3-1.24.1.zip
En vervolgens deze plugin blokkeren zodat ze niet meer geüpdate wordt.
basrieter
commented
5 years ago basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Het zou zoveel helpen als iemand (even zonder Retrospect specifiek te noemen want het geld voor alles) dit bij Team Kodi zou melden via https://github.com/xbmc/xbmc/issues/. Ik ben al in discussie over dit onderwerp, maar een fatsoenlijk bug report doet dan wonderen. Maar houd je dan wel netjes aan het issue template anders wordt ie weer gelijk gesloten.
Het zou perfect zijn als je een dummy add-on toevoegd die enkel de urllib3 model gebruikt en daarmee een simple url aanroept. Dan kunnen ze gelijk zie dat het wel heel traag is.
Een andere ding dan nog: ik heb in de addon.xml de <reuselanguageinvoker> op true staan. Helpt dat niet? Want volgens mij importeert hij dan alles maar een enkele keer.
basrieter
commented
5 years ago Original comment by Peter Notebaert (Bitbucket: peno64, GitHub: peno64).
Is een addon noodzakelijk?
Ik kan het simuleren door het volgende shell script. Ik zet dit script bvb in de temp folder en zorg dat urllib ook daar staat. Dan kan ik eenvoudig met versie 1.22 en 1.25 testen door gewoon de directory te overschrijven en dus totaal geen addon nodig…
import time
t1 = time.time()
from urllib3.util import parse_url
t2 = time.time()
print t2 - t1
When done with version 1.22 this gives
0.272784948349
When done with version 1.25.2 this gives:
3.80189013481
Indien dit ok is wil ik wel een issue aanmaken op een maagdelijke RPI.
Maar het feit dat dit ook al ligt bij urllib3, is dat niet voldoende of hoop je hiermee dat ze in kodi zouden bekijken waarom het zo traag is op RPI?
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
@Peter Notebaert maakt het reproduceren wat makkelijker. Het hoeft echt alleen een zip te zijn die je als addon kan installeren met als addon een addon.xml (met de urllib3 dependency) en een enkele Python file die hem aanroept.
basrieter
commented
5 years ago Original comment by Peter Notebaert (Bitbucket: peno64, GitHub: peno64).
Zoals je kan zien met bovenstaand shell script is een url request zelfs niet nodig. Het is gewoon de import van urllib3 welke heel erg traag is geworden. Blijkbaar omdat op dat ogenblik veel regulieren expressies worden gecompileerd.
basrieter
commented
5 years ago basrieter
commented
5 years ago basrieter
commented
5 years ago basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Ik heb ook op de PR van 1.25.2 een commentaar achtergelaten: https://github.com/xbmc/repo-scripts/pull/1052
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Ok, nu ik dan weer wakker ben:
Zouden jullie eens kunnen kijken of alpha3 goed werkt?
basrieter
commented
5 years ago Original comment by Peter Notebaert (Bitbucket: peno64, GitHub: peno64).
zowel 4.1.8.18alfa1 en 4.1.8.18alfa3 lijken te werken.
Echter ik vermoed dat er nog steeds indirecte dependencies zijn naar de urllib3 addon die niet ingesloten is in retrospect.
Namelijk als ik probeer urllib3 (v1.25.2) te uninstallen dan zegt kodi dat dit niet kan omwille van referenties naar requests (v2.21.0) en als ik requests probeer te uninstallen zegt die dat die nog gebruikt wordt door de addons InputStream Helper en Retrospect. En dat is met de alfa3 versie. Dus retrospect verwijst wel niet meer direct naar urllib3 maar via andere imports die worden gebruikt in retrospect wordt die dan toch weer aangesproken… Dus ik vrees dat urllib3 insluiten in retrospect niet veel zin heeft.
Ik zie nu dat ze bij urllib3 een versie 1.24.3 hebben gereleased na 1.25..2
Spijtig genoeg, eens je 1.25.2 hebt zal die 1.24.3 niet geinstalleerd worden door kodi en zal er telkens een update worden gevraagd…
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Fijn om te horen. De referenties blijven ook, omdat er veel meer dependecies zijn voor urllib3, die ik niet allemaal wil includen. Ik override enkel het urllib3 pad met mijn eigen versie. Dus ik sta dan los van de versie van Kodi.
Maar…..ik heb het binnen het Kodi team overlegd en we gaan kijken of we 1.25.2 kunnen reverten en vervangen door 1.24.3. Dat moeten we nog even overleggen met de persoon die deze onderhoud. Dat kan dus even duren.
basrieter
commented
5 years ago basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Please install https://bitbucket.org/basrieter/xbmc-online-tv/downloads/plugin.video.retrospect-4.1.8.18.zip to fix this issue.
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
@{557058:ca82a86d-5f96-42b9-b26a-4f3b8f256dc3} Goed nieuws. Maar je neemt nu een gelijkaardige oplossing als wij: een oplossing op het niveau van de addon. Zelfs als script.module.requests nu wordt teruggedraaid in de Kodi repository, is het geen langetermijnoplossing want ooit zal hij om security redenen toch geupgrade moeten worden. Bedankt alleszins voor al je hulp en inzet ! (v4.1.8.18 geïnstalleerd :wink:)
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
@dagwieers dat klopt! Ik heb nu even gedaan wat voor de eindgebruiker goed is en voor mijn (nagenoeg) geen impact. We gaan kijken hoe we hier een permanentere oplossing voor kunnen bedenken. Maar dat zal samen met het urllib3 team, Kodi team en de urllib3 add-on beheerder moeten moeten gaan gebeuren.
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Inderdaad. Dat gezegd, de import van requests blijft ongeveer 500ms duren, zelfs in de oude versie en dat blijft behoorlijk veel. In verhouding, de import van BeautifulSoup4 neemt 240ms in beslag (die hebben we bijna volledig verwijderd), die van dateutil 120ms. Dus als je die drie importeert in je addon weet je waar je aan toe bent (minstens 1sec laadtijd voor er iets is gebeurd). Daarom dat we dat dus niet meer importeren standaard (enkel wanneer hij gebruikt wordt) en requests eruit hebben gegooid in het voordeel van urllib2 (voor Python 2) die slechts 240ms duurt. Onze addon vliegt (nu ook op Raspberry Pi).
Naar urllib3 gaan (zonder requests) neemt 290ms, maar heeft dus het risico dat we opnieuw in de problemen komen. Al is het misschien een optie om urllib3 te gebruiken zonder extraatjes zoals requests doet. Dat gaan we nog moeten bespreken.
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Ik snap het, maar die 250ms die het extra kost, t.o.v. urllib2 neem ik voor lief. Ik ben echt afgestapt van de custom code om dingen te doen. Daarnaast bestaat urllib2 niet voor Python 3, en Python 3 is voor Leia en voor Matrix verplicht om in die repository te komen. Hebben jullie de <reuselanguageinvoker> aan staan, want die zou onnodige re-imports en reloads van modules moeten voorkomen?
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Voor Python 3 gebruiken we urllib, en hopelijk kunnen we afstappen van urllib2 als Python 2 gedaan is. Dus echt custom code is het niet, integendeel, eigenlijk zijn requests en urllib3 meer “custom” dan the standaard python libraries.
Die reuselanguageinvoker moet ik zelf eens bekijken, dat lijkt inderdaad handig, doch de imports doen waar ze worden gebruikt is eigenlijk niet zo’n groot probleem voor ons. De impact op leesbaarheid is beperkt, onze code is redelijk beperkt op dat vlak, ik kan me best voorstellen dat in Retrospect de impact een pak groter is.
We hebben in het verleden wel wat problemen gehad omdat Web scraping niet altijd even betrouwbaar was, en dat heeft altijd een effect op de eindgebruiker, en we willen nu echt naar een addon met zo weinig mogelijk afhankelijkheden. Dat doen we nu met fallback code als scraping niet werkt, door meer defensief te werken, alles in vraag stellen, extra unit tests, CI integratie, etc.
basrieter
commented
5 years ago Original comment by Bas Rieter (Bitbucket: basrieter, GitHub: basrieter).
Wat ik met custom code bedoel is zelf wrappers gaan rondom andere standard libraries, zoals urllib2. Dat had ik eerst namelijk. De requests module is toch echt wel erg basic naar mijn idee en wordt echt veel gebruikt. Met de reuselanguageinvoker wordt een dergelijke import maar 1x uitgevoerd en zou het daarna geen probleem meer moeten zijn.
Maar goed, als het moet dan kan het altijd nog urllib worden voor Python 3. Ik heb alles een eigen UrlHandler staan die enkel de implementatie van requests abstract maakt. Zolang ik de signatures niet verander, zou alles goed moeten gaan.
BTW: Als jullie er behoefte aan hebben zouden jullie op de Retrospect Slack workspace kunnen komen, dan praat het wat sneller en makkelijker. Mocht je interesse hebben hoor ik het wel via uitzendinggemist.vx [@] gmail [dot] com.
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Ik heb ook altijd gedacht dat requests erg basic was, en het is zeker handig en wordt veel gebruik. Maar het is niet echt basic als je ziet wat ze allemaal hebben toegevoegd wat standaard geladen wordt. En voor normaal gebruik is dat wellicht niet zo belangrijk, maar voor ons specifiek gebruik waar (1) laadtijd dus heel belangrijk is en (2) we weinig toeters of bellen gebruiken, en belangrijker, waar (3) we geen grote veranderingen willen met potentiële breakages of performance regressies waar we geen controle over hebben, ben ik dus recent van gedacht veranderd. (Het moment dat ik van gedacht ben veranderd is mooi gedocumenteerd in ons GitHub issue :slight_smile:) We veranderen misschien nog wel eens van gedacht.
Het helpt natuurlijk ook als iemand de PR al had geschreven toen de beslissing werd genomen :smiley: (Bedankt @{5c978dd892a55a222a81d4d3} !)
Ik zou graag wat meer meehelpen met Retrospect voor de Belgische zenders, ook als alternatief voor onze eigen VRT NU en VRT Radio plugin. Ik denk dat het goed is dat gebruikers alternatieven hebben voor het geval dat een addon een probleem heeft. Extra resilience :wink:
Ik ben zelf geïnteresseerd om de nieuwe VTM GO in Kodi te zien. Lijkt me een leuke aanwinst voor de waaier van mogelijkheden die we vandaag al hebben. (Ik ben zelf niet zo tevreden van de Vier en Vijf integratie, zowel naar content als de kwaliteit van hun infrastructuur, ligt niet aan Retrospect)
(Over en uit!)
basrieter
commented
5 years ago Original comment by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
BTW Net getest met reuselanguageinvoker en voor onze nieuwste addon (urllib2 + selective imports) merk ik geen verschil, maar als ik het toepas op onze oudere addons (requests) met laatste urllib3 is het verschil enorm. De eerste laadtijd is nog steeds traag, maar nadien merk je nauwelijks een verschil tussen gebruik van requests en urllib2. Quasi instant menus, sneller als ooit voorheen.
Misschien moeten we onze mening toch eens opnieuw herzien. De combinatie requests en selective imports lijkt mij de ideale keuze. Eerste laadtijd zal nog steeds snel zijn, bij de eerste HTTP access moet je dan eventueel wat langer wachten. Best of both worlds. Mijn enige bezwaar voor requests is dan nog de impact en scope van library updates.
Original report by Dag Wieers (Bitbucket: dagwieers, GitHub: dagwieers).
Door een recente update van script.module.urllib3 naar v1.25 neemt een import van de requests library standaard 4.2 seconden in beslag. Dus iedere actie in Retrospect wordt vertraagd met bijna 4 seconden, wat alles in Kodi op Raspberry Pi enorm vertraagd.
Meer informatie in: https://github.com/pietje666/plugin.video.vrt.nu/issues/169
Voor onze plugin plugin.video.vrt.nu hebben we besloten om niet langer requests/urllib3 te gebruiken omdat dit het tweede incident is in korte tijd met grote impact op onze gebruikers.