Opened 7 years ago
Closed 7 years ago
Last modified 7 years ago
#13084 closed Bug Report - General (fixed)
ttvdb.py will cease to function - TTVDB will discontinue support for V1 XML API in 3 months time on October 1st, 2017
| Reported by: | Owned by: | Mark Spieth | |
|---|---|---|---|
| Priority: | minor | Milestone: | 29.0 |
| Component: | MythTV - Mythmetadatalookup | Version: | Unspecified |
| Severity: | medium | Keywords: | |
| Cc: | Ticket locked: | no |
Description
The current ttvdb.py metadata script appears to be using the TV DB version 1 XML API. It was made public on April 2nd, 2017 that on October 1st, 2017 that support for the version 1 TTVDB API will be discontinued.
This means that the current ttvdb.py script will no longer be functional to retreive TV metadata from thetvdb.com. I have not specified a version of MythTV for this ticket as this issue affects all currently supported versions.
The new version 2 TTVDB JSON API was made public on April 25, 2016.
I have also filled an issue with the tvnamer script that that the ttvdb.py was based on to alert them too.
Attachments (8)
Change History (83)
comment:1 Changed 7 years ago by
| Milestone: | needs_triage → 29.0 |
|---|---|
| Owner: | changed from JYA to Mark Spieth |
| Status: | new → assigned |
comment:2 Changed 7 years ago by
Update: I just found this on the tvdb_api github page. Note: Please try to use the Lighthouse project instead of Github Issues
So it appears that the developers prefer to use the ticket system at Lighthouseapp.com. However, the activity in the issue queue there is not promising. The last resolved ticket was 11 months ago.
There is a ticket asking about an ETA on the API V 2.0 dating back two months ago but it was never answered.
dbr/Ben does show activity on Github as recently as July 8, 2017.
comment:3 Changed 7 years ago by
A pull request has been added to github https://github.com/MythTV/mythtv/pull/141
please test and see how it goes. The output is not quite the same but its close. I have not been able to test on python2.6 but it woks with 2.7.13 and 3.5.4rc1 (debian testing)
It uses an update of the original tvdb_api module from There are some extra python modules required.
Also a good chunk of python 3 compatability has also been done with the other modules. Untested with other scripts.
Also ttvdb.py --doctest runs a set of unit tests which requires tvdb_test.conf in the same directory as the script.
comment:4 Changed 7 years ago by
I do not have much success with this - I am afraid I have little knowledge of python, so I don't know where it went wrong. I installed my test version of MythTV in /home/peter/proj/build/mythtv/master and set my paths to point appropriately. This works with other scripts and the fixes/29 version of ttvdb.py. My Python version is 2.7.12. I ran ttvdb.py with no parameters.
tmdb3.py now also fails with "ImportError?: No module named builtins"
+ export PATH=/home/peter/proj/build/mythtv/master/usr/bin:/home/peter/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/opt/mythtv/bin
+ PATH=/home/peter/proj/build/mythtv/master/usr/bin:/home/peter/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/opt/mythtv/bin
+ export MYTHTVDIR=/home/peter/proj/build/mythtv/master/usr
+ MYTHTVDIR=/home/peter/proj/build/mythtv/master/usr
+ export LD_LIBRARY_PATH=/home/peter/proj/build/mythtv/master/usr/lib:/home/peter/proj/build/mythtv/master/usr/share/mythtv/lib:
+ LD_LIBRARY_PATH=/home/peter/proj/build/mythtv/master/usr/lib:/home/peter/proj/build/mythtv/master/usr/share/mythtv/lib:
+ export MYTHCONFDIR=/home/peter/.mythtv-msttst
+ MYTHCONFDIR=/home/peter/.mythtv-msttst
+ export PYTHONPATH=/home/peter/proj/build/mythtv/master/usr/local/lib/python2.7/dist-packages
+ PYTHONPATH=/home/peter/proj/build/mythtv/master/usr/local/lib/python2.7/dist-packages
++ ls -d /home/peter/proj/build/mythtv/master/usr/local/share/perl/5.22.1
+ export PERL5LIB=/home/peter/proj/build/mythtv/master/usr/local/share/perl/5.22.1
+ PERL5LIB=/home/peter/proj/build/mythtv/master/usr/local/share/perl/5.22.1
+ exec /srv/ahome/peter/proj/build/mythtv/master/usr/share/mythtv/metadata/Television/ttvdb.py
The modules tvdb_api.py (v2.0 or greater).
They should have been installed along with the MythTV python bindings.
Error:(%s)
Traceback (most recent call last):
File "/srv/ahome/peter/proj/build/mythtv/master/usr/share/mythtv/metadata/Television/ttvdb.py", line 765, in <module>
''') % e
TypeError: unsupported operand type(s) for %: 'NoneType' and 'exceptions.ImportError'
comment:5 Changed 7 years ago by
I've got the patch installed on a test host, in the 'normal' place and got the error at line 765 too. For *buntu, this allowed ttvdb.py --help to work:
sudo apt-get install python-future python-requests-cache
For anyone else testing, install the patch with:
git remote add mspieth https://github.com/mspieth/mythtv.git git fetch mspieth ttvdb-api-v2 git merge remotes/mspieth/ttvdb-api-v2 build as normal
comment:6 Changed 7 years ago by
apologies for not being specific with the extra py mods
as bill said
apt-get install python-requests-cache python-future
do same for python3 if testing with that (future is part of py3)
apt-get install python3-requests-cache
to test
cd ..../mythtv/programs/scripts/metadata/Television PYTHONPATH=..../mythtv/bindings/python ./ttvdb.py --doctest PYTHONPATH=..../mythtv/bindings/python3 ./ttvdb.py --doctest
should give no errors if the output matches the doctests near the top of the file. PYTHONPATH is required if you dev tree is not in the default PYTHONPATH otherwise it will use your installed libs. Current path is viatal too for tvdb_test.conf for the tests.
Dev was done with pycharm (my favourite ide for python ATM). Be careful with project paths in this too.
comment:7 Changed 7 years ago by
Help!
What am I doing wrong now? Also I get the same error if I run it with a show name as parameter.
+ export PYTHONPATH=/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages
+ PYTHONPATH=/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages
+ exec /home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py --doctest
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 42, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[1]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 46, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[3]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 105, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[5]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 115, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[7]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 121, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[9]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 128, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[11]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 152, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[13]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 159, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[15]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 215, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[17]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1820, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 351, in __main__
Failed example:
main()
Exception raised:
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 1315, in __run
compileflags, 1) in test.globs
File "<doctest __main__[19]>", line 1, in <module>
main()
File "/home/peter/proj/build/mythtv/ttvdb/usr/share/mythtv/metadata/Television/ttvdb.py", line 1844, in main
userkey=tvdb_account.account_identifier)
File "/home/peter/proj/build/mythtv/ttvdb/usr/local/lib/python2.7/dist-packages/MythTV/ttvdb/tvdb_api.py", line 715, in __init__
self.session.remove_expired_responses()
AttributeError: 'CachedSession' object has no attribute 'remove_expired_responses'
**********************************************************************
1 items had failures:
10 of 20 in __main__
***Test Failed*** 10 failures.
TestResults(failed=10, attempted=22)
comment:8 Changed 7 years ago by
looks like you may not have a recent enough requests-cache
# dpkg -l python-requests-cache ii python-requests-cache 0.4.13-1 all persistent cache for requests library
if not that version, try
pip install requests-cache
otherwise just comment out the remove_expired_responses line, works just as well most of the time except for the pre auth test whether to auth or not.
comment:9 Changed 7 years ago by
Mark,
Pardon my lack of Python skills, but it looks like version 2.x supports two print %<x> syntaxes but 3.x only supports one:
$ python2
>>> print('''Some String: %s''') % 'something else'
Some String: something else
>>> print('''Some String: %s''' % 'something else')
Some String: something else
$ python3
>>> print('''Some String: %s''' % 'something else')
Some String: something else
>>> print('''Some String: %s''') % 'something else'
Some String: %s
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for %: 'NoneType' and 'str'
Not a big deal, but for any missing module, the error identifying the module doesn't print out. For example:
Traceback (most recent call last):
File "./ttvdb.py", line 765, in <module>
''') % e
TypeError: unsupported operand type(s) for %: 'NoneType' and 'ModuleNotFoundError'
I can make a patch if you want.
comment:10 Changed 7 years ago by
print is a function in py 2 with future and 3. You have identified a bug. % should be inside the () in all cases. py2 original syntax allowed many things including no (). note that % is a format operator on a string. works in py2 since () returns the string and in py3 its part of the function. bugger that I didnt notice this before. legacy I missed. updated pull request
comment:11 Changed 7 years ago by
NOTE that I have added a mythtv user account to thetvdb.com. assuming the original has been lost over time.
The registered email is currently mine and should change to a project based one for password recovery.
I will forward the password securely on request.
comment:12 Changed 7 years ago by
The first 2 are correct, the 2nd 2 are ineffective. There are no formatting directives in '\n' to which the % operator is applied. The string being updated is the correct one to use as the first operand of the % operator. If the print function has 1 arg, then things are usually fine and these 2 are (fine I mean).
Already applied the first 2 anyway.
comment:13 Changed 7 years ago by
Another testing issue is that python3 cache cannot be read by python2. I tried to fix this but no luck so far but sould not be an issue in practice.
remove the cache dir when running doctest with py2 after a py3 run.
comment:14 Changed 7 years ago by
The only doctest failure is caused by a change in a lastupdated line and this fixes it for me:
diff --git a/mythtv/programs/scripts/metadata/Television/ttvdb.py b/mythtv/programs/scripts/metadata/Television/ttvdb.py
index 6453e33425..5969a01ce1 100755
--- a/mythtv/programs/scripts/metadata/Television/ttvdb.py
+++ b/mythtv/programs/scripts/metadata/Television/ttvdb.py
@@ -371,7 +371,7 @@ Banner:http://thetvdb.com/banners/graphical/73739-g4.jpg,http://thetvdb.com/bann
<ratingcount>168</ratingcount>
<year>2007</year>
<releasedate>2007-03-14</releasedate>
- <lastupdated>Sat, 29 Jul 2017 07:24:53 GMT</lastupdated>
+ <lastupdated>Fri, 28 Jul 2017 16:24:53 GMT</lastupdated>
<status>Ended</status>
<images>
<image type="fanart" url="http://www.thetvdb.com/banners/fanart/original/80159-11.jpg" thumb="http://www.thetvdb.com/banners/_cache/fanart/original/80159-11.jpg"/>
comment:15 Changed 7 years ago by
Aftyer commenting out remove_expired_responses there was another item that was not found.
if self._ignored_parameters:
AttributeError: 'DbCache' object has no attribute '_ignored_parameters'
I commented out the test for _ignored_parameters as well.
Now I am getting a 403 error. with -d I see the below error. I get the same error with --doctest or a series name.
# ..got tvdb mirrors # Start to process series or series_season_ep #################### DEBUG:tvdb_api:Getting show Hannibal DEBUG:tvdb_api:Searching for show Hannibal DEBUG:tvdb_api:auth INFO:requests.packages.urllib3.connectionpool:Starting new HTTPS connection (1): api.thetvdb.com DEBUG:requests.packages.urllib3.connectionpool:"POST /login HTTP/1.1" 403 None ... various traceback messages ... simplejson.scanner.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
Did the user id get locked out or disabled?
comment:16 Changed 7 years ago by
Peter, I've done several doctests as well as letting my production backend retrieve metadata (e.g. when the recording completes) after your last post.
I don't have the Python bindings installed for version 3.x, so have only been testing on version 2.7
This is my test script:
#!/bin/sh rm -rf ~/.mythtv/cache/tvdb_api/* cd ~/source/mythtv/mythtv/programs/scripts/metadata/Television PYTHONPATH=~/source/mythtv/mythtv/bindings/python python2 ./ttvdb.py --doctest
For the record, my production box is still on 14.04, ENOTIME, but it's on 30-Pre. I had to do these to get things running:
# apt can't find these 2 on 14.04: sudo pip install future requests_cache # apt has python-requests, but it's old, so: sudo pip install --upgrade requests
comment:17 Changed 7 years ago by
Thanks Bill I ran those pip installs, also I reverted my changes where I had commented some things out. Now it it working much better. --doctest gives 3 failures out of 22, and running with a series name is giving back good results. Next I need to test it with the backend.
comment:18 Changed 7 years ago by
another update to the pull request
- Upstream support for partial language data
- Upstream handle loadurl errors by callers
- Upstream support no cache and non-conforming cache
- Upstream tolerate missing cache _ignored_parameters attribute
- Remove username and userkey from auth info, not required, less brittle
- python 3.6 support for num-seasons output order
comment:19 Changed 7 years ago by
Mark, I fetched the above, especially because I was having trouble with a language issue. Please try this:
$ /usr/local/share/mythtv/metadata/Television/ttvdb.py -l en -a US -D 281053
and see if it results in:
File "/usr/local/share/mythtv/metadata/Television/ttvdb.py", line 1194, in change_amp
text = text.replace(""", "'").replace("\r\n", " ")
AttributeError: 'dict' object has no attribute 'replace'
Thanks for all your hard work on this!!!
comment:20 Changed 7 years ago by
updated pull request
fixed 2 more modes
- -l de -m -a US -D 72449 1 10
- -l en -a US -D 281053
comment:21 Changed 7 years ago by
Here's one that has no actors (per http://thetvdb.com/?tab=series&id=283661&lid=7):
/usr/local/share/mythtv/metadata/Television/ttvdb.py -l en -a US -D 283661
This works for me:
diff --git a/mythtv/programs/scripts/metadata/Television/ttvdb.py b/mythtv/programs/scripts/metadata/Television/ttvdb.py
index 2356bad0e3..c8f8bcb138 100755
--- a/mythtv/programs/scripts/metadata/Television/ttvdb.py
+++ b/mythtv/programs/scripts/metadata/Television/ttvdb.py
@@ -1263,6 +1263,7 @@ def Getseries_episode_data(t, opts, series_season_ep, language = None):
# Get Cast members
cast_members=''
+ tmp_cast = ''
try:
series_data = search_for_series(t, series_name, opts.language)
tmp_cast = series_data['_actors']
comment:23 Changed 7 years ago by
Mark - thank you for all the hard work on this!
It is working now, but is not retrieving fanart or banners, so the background in the watch recording screen is not shown.
This is the command sent by MythTV
ttvdb.py -l en -a US -N 72108 Pyramid
Attached are the response from the old version of ttvdb.py and the new version. You can see there are several fanart and banners returned by the old, that are not present in the new.
Changed 7 years ago by
| Attachment: | doctest-update-2017-08-14.patch added |
|---|
These changes allow --doctest to pass, not sure if changing the expected is the solution
comment:24 Changed 7 years ago by
another update.
fixes peters issue with less data than before I hope.
break away!
comment:25 Changed 7 years ago by
Mark, great! Had two shows last night that didn't get cover art, but re-running with today's fetch took care of both.
283661 from comment 21 is still acting up. Try this version of a previous test:
/usr/local/share/mythtv/metadata/Television/ttvdb.py -l en -a US -N 283661 "Egg Hunt"
it should drop a Traceback and this fixed it for me and doctest is still perfect.
diff --git a/mythtv/bindings/python/MythTV/ttvdb/tvdb_api.py b/mythtv/bindings/python/MythTV/ttvdb/tvdb_api.py
index 80df1d3..8b64fad 100644
--- a/mythtv/bindings/python/MythTV/ttvdb/tvdb_api.py
+++ b/mythtv/bindings/python/MythTV/ttvdb/tvdb_api.py
@@ -1033,6 +1033,12 @@ class Tvdb:
actorsEt = self._getetsrc(self.config['url_actorsInfo'] % (sid))
cur_actors = Actors()
+
+ if actorsEt is None:
+ self.config['actors_enabled'] = False
+ self._setShowData(sid, '_actors', cur_actors)
+ # cur_actors is just an empty list() ^^^
+ return
+
for curActorItem in actorsEt:
curActor = Actor()
for curInfo in curActorItem.keys():
comment:26 Changed 7 years ago by
Did it a bit differently but good catch.
Pull request updated.
break away!
comment:27 Changed 7 years ago by
It is looking good now. I can get metadata for TV series in both recordings and videos.
comment:28 Changed 7 years ago by
The versions of the various python modules these changes are requiring is a problem for a lot of people, least of all our downstream packagers.
We need to have this work with the packages that are shipped in Ubuntu 16.04 (LTS)
Regards Stuart
comment:29 Changed 7 years ago by
I should add that this is because using pip to pull in these modules doesn't work for downstream packaging. It's a case of install a shipped module or nothing.
comment:30 follow-up: 44 Changed 7 years ago by
I finally had a little time (while a compile was running) to look over the pull request.
First, let me thank Mark for the raising of the hand to do this work. While I personally do not use ttvdb, I know that it is used by many, and keeping it working is important to those people.
Secondly, I am not a dev, and my opinions (AFAIK) carry no weight with anyone that actually matters, but I do have opinions, and so I will share them, but one should feel free to ignore them in preference to official responses.
Now on to the review. Note that this is based only looking at the pull request and the code. I have not actually tested it.
While there is no formal review process in the project, I have a couple of concerns about the current pull request.
First, while I would rarely use quite the flowery language used by Linus on the LKML, I do agree with him that mixing new features with bug fixes should result in the entire patch being rejected with prejudice. While changing the bindings to support python3 is goodness in the long run, that has nothing to do with fixing the ttvdb script which (as has been stated elsewhere?) runs with python2. A python3 port of the bindings should be a separable (developer task?) ticket, with a separate pull request, likely targeted just to the master branch. Separating the python3 patches will also reduce the required new dependencies to (I think) just python2-requests. Again, simpler is better (especially for something that needs back-porting). Also, depending on distro/packging, python3 modules can have packaging requirements (different library locations) that can be problematic and has additional dependencies for packagers. Again, if not needed, not appropriate for backporting. I will also note that one of the devs, I think Mr. Wagner (I think it was Mr. Wagner, but it has been long enough that my pointer may be pointing to previously freed and now overwritten memory) suggested he had issues with using the various python compatibility import libraries (six and/or future?), so it may be good to ask for an opinion on the use of those compatibility imports (I do not remember if the issue(s) were philosophical, or known incompatibility, or what (and whether any of those concerns are still operational)) in the separate developer task ticket for python3 binding update.
Secondly, there is a (legacy, but now identified) issue with the use of an embedded apikey that cannot be (easily) modified. There were known to be individuals using MythTV as a base for commercial profit (MAAS?). The ttvdb TOS currently explicitly states that an apikey cannot be used (without approval, and I presume payment/licence) for anything involving commercial use. Embedding a apikey is a default enabler of misuse, and ttvdb would be well within their rights to revoke that key if it is so identified. And while it admittedly is true that that is also with the existing ttvdb TOS and the current apikey, it is unclear (to me at least) whether the TOS has changed over the years regarding commercial use, but it is clear now. And it has been my interpretation that the project has been quite consistent that it will not knowingly assist in the misuse of others content (why patches that bypass other sites limitations are not accepted), and this means (again, in my interpretation) that the default should likely not be to embed an apikey that is restricted and not at all easily modified. Whether the best solution is to require every user to have their own separate apikey, or install a key from a well known location (after agreeing that it will be used only for personal use), and then stored in the file system somewhere (like the raopkey?), or store some value in the settings table (value='ttvdb_apikey", data="whatever"?), is likely a good discussion (as signing up in order to get a personal apikey is free, that has the potential added benefit that it might encourage a few someones to be contributors, and not just takers, from the metadata source).
Lastly, I like python requests. It makes things a lot easier. While there are some features where requests is required (certain socket level options), it is often possible to rework the code (with additional pain) to use the legacy urllib functionality. However, as I am not doing to work, I am not going to recommend someone else make that effort either unless it is obviously trivial.
comment:32 Changed 7 years ago by
This is on Ubuntu 16.04.
I removed the components that were installed using pip (pip uninstall). I installed the packages using apt
python-future, python-requests, python-requests-cache
got these versions (plus a lot of other things)
- future (0.15.2)
- requests (2.9.1)
- requests-cache (0.4.10)
Full list - https://pastebin.com/W7ysj1Bf
Rebuilt with the latest source. I am back to the 403 error I had before.
ttvdb.py -d -l en -a US -N Hannibal Aperitif
Error message:
DEBUG:requests.packages.urllib3.connectionpool:https://api.thetvdb.com:443 "POST /login HTTP/1.1" 403 None
Full output: https://pastebin.com/e6tWiS3C
Changed 7 years ago by
| Attachment: | doctest.out added |
|---|
Removed via pip, removed via apt and installed via apt for future, requests, requests-cache and urllib3
comment:33 Changed 7 years ago by
another update.
should fix 403 error on Authorization with older requests. Who would have thought the User Agent would do that.
break away!
comment:34 follow-up: 35 Changed 7 years ago by
Review Responses:
- This is not really a bug fix so was not treated as such. hence the major version change to 2.0
- The reason for the new dependencies is the upstream api now uses them. The updated API was chosen to be the path of least resistance as the interface is the same (almost). There was a case that the data returned was not as good as the old api so some enhancements were made. These are being pushed upstream to the parent repo.
- I agree with the API key being in the module as bad but as you say this is legacy and beyond the scope of this API update.
- Python3 compatability can be split out from the pull request if required as it is independent and in a separate changeset. Naturally my preference is to not spend any extra effort if people are happy with the current state of the update.
Now that it (seems to) works with the older versions of requests and requests_cache, that should resolve any final issues.
comment:35 Changed 7 years ago by
Replying to markspieth:
Review Responses:
- I agree with the API key being in the module as bad but as you say this is legacy and beyond the scope of this API update.
Response to response: This is a new apikey, so you cannot claim beyond scope as you added it in your update. If you do not change the key, you can claim legacy.
Changed 7 years ago by
| Attachment: | 403.fix.error added |
|---|
Much better, single failure on one of 5 doctests ("bad handshake: SysCallError?(-1, 'Unexpected EOF')")
comment:36 follow-up: 38 Changed 7 years ago by
That one is probably a network issue. Not sure why. Was this the first one? And the rest were from cache?
Also clear your cache every so often too.
rm ~/.mythtv/cache/tvdb_api/*
comment:37 Changed 7 years ago by
In regard to the API key needing updating, my original thoughts were incorrect. The original key still works so I have reverted it.
This turned out to be an unnecessary change.
comment:38 Changed 7 years ago by
Replying to markspieth:
That one is probably a network issue. Not sure why. Was this the first one? And the rest were from cache?
Yes, it was the 1st test that had the 'bad handshake' error.
Just to be sure my doctests are the same, I use the script (in comment:16) that clears the cache. I'll just watch to see if this happens again.
This is looking good. Switching back to system testing and "60 Minutes" failed last Sunday (and my recording rule has the inetref set to "ttvdb.py_73290".) I'll see what happens when it records tomorrow.
For extra credit, the patch applies and doctest passes in 29.0 too.
comment:39 Changed 7 years ago by
It is looking good now, with just the Ubuntu packages installed, no pip packages.
Retrieving metadata from MythTV works
doctest gives 2 errors - see https://pastebin.com/eEmFSFBG (I deleted the cache before running it).
comment:40 Changed 7 years ago by
I cannot install this on raspbian jessie - packages python-future and python-requests-cache are not found. I installed pip and pip install crashes, error compiling future and many other errors. I suppose we can live without it on raspbian.
comment:41 Changed 7 years ago by
Very strange error exit code 1. In every case there should be a stderr message. Why is it missing? This would tell us why its failing.
I suspect you have not captured stderr in the output. Hopefully this will clarify the reason.
It will probably be one of the 2 "! Error" messages in the line number 2100's of ttvdb.py.
comment:42 Changed 7 years ago by
That is everything from the screen. I ran it again with -d option, and using &> to capture stdout and stderr.
comment:43 Changed 7 years ago by
I see the problem.
The directory you ran the test out of does not have tvdb_test.conf in it. can't find it so error.
These 2 tests validate config file reading with overrides.
tvdb_test.conf needs to be in the current directory when --doctest is run. This is intended to be the same directory that ttvdb.py is in.
comment:44 Changed 7 years ago by
Replying to Gary Buhrmaster <gary.buhrmaster@…>:
Now on to the review.
Third. requests-cache is an optimization (a perhaps good optimization, but still, just an optimization), and should not be required especially for backports (try (and pass?) may be the appropriate solutions here).
comment:45 Changed 7 years ago by
Very true. The library changed and I just did updated without changing functionality.
I could add an option to disable cache if you like.
Adaptive caching might be misleading but is possible. Not sure if this change would be accepted upstream though whereas the option is independent. Either case will still need a try except around the import for requests_cache. Is this really that important?
Backports was not originally a requirement but should have been mentioned up front. As a result it wasn't addressed when choosing the original least path of resistance solution.
comment:46 Changed 7 years ago by
updated pull request so that the error above is sent to stderr instead of /tmp/ttvdb.log which originally only output that error. How strange.
comment:47 Changed 7 years ago by
The --doctest works now if I copy the tvdb_test.conf file to the current directory. Also if it is not there, a message does tell me that.
So all looks good now.
comment:48 Changed 7 years ago by
My four Sunday programs all got the expected artwork when mythmetadatalookup ran after each show.
doctest has a new issue, there's a test that has 72108-30.jpg and 72108-29.jpg reversed in the expected/got output. It's on line 478 and for NCIS. Seems like something that will change as thetvdb.com changes their data. Just mentioning it in case others trip across it.
comment:49 Changed 7 years ago by
Replying to Bill Meek <keemllib@…>:
doctest has a new issue, there's a test that has 72108-30.jpg and 72108-29.jpg reversed in the
This is sorted by rating so I would guess the rating relative for these 2 fanarts have changed.
pull request updated with more tolerant test.
comment:50 Changed 7 years ago by
Raspbian stretch has just been released, and it includes the missing python packages, so hopefully the new ttvdb.py will work there. I will be testing that.
comment:51 Changed 7 years ago by
I have been using the patch in production for a week, and all seems ok. Can I commit it to git? Any objections?
comment:52 Changed 7 years ago by
I have been too since monday, but only ttvdb not any of the others. ttvdb seems to be fine.
tmdb3.py -M terminator
but
tmdb3.py -M happy
works. Music stuff I have no idea how to use.
I'm ok with merging.
comment:53 follow-up: 55 Changed 7 years ago by
Peter, I'd say merge too. Nice to get some 30-Pre testers.
Mark, could: Television/tvdb_test.conf live somewhere else? Looks like anything in that directory is expected to be an executable and mythmetadatalookup trys to run it with the -v flag just to see if it's available.
2017-08-24 20:43:19.408331 I [2048/2048] CoreContext mythsystemunix.cpp:948 (Fork) - Managed child (PID: 2097) has started! * command=/usr/local/share/mythtv/metadata/Television/tvdb_test.conf -v, timeout=0 2017-08-24 20:43:19.428473 I [2048/2054] SystemManager mythsystemunix.cpp:354 (run) - Managed child (PID: 2097) has exited! command=/usr/local/share/mythtv/metadata/Television/tvdb_test.conf -v, status=32256, result=126
comment:54 Changed 7 years ago by
I'm not sure where else it should go for testing purposes. It should go somewhere. I don't know where but we can put a path or even specify it as a param to --doctest. Makes unit testing a bit more cumbersome though.
Is there some sort of test framework structure that mythmetadatalookup will filter out?
My installed version is not executable. (deb)
comment:55 Changed 7 years ago by
Replying to Bill Meek <keemllib@…>:
Peter, I'd say merge too. Nice to get some 30-Pre testers.
Mark, could: Television/tvdb_test.conf live somewhere else? Looks like anything in that directory is expected to be an executable and mythmetadatalookup trys to run it with the -v flag just to see if it's available.
2017-08-24 20:43:19.408331 I [2048/2048] CoreContext mythsystemunix.cpp:948 (Fork) - Managed child (PID: 2097) has started! * command=/usr/local/share/mythtv/metadata/Television/tvdb_test.conf -v, timeout=0 2017-08-24 20:43:19.428473 I [2048/2054] SystemManager mythsystemunix.cpp:354 (run) - Managed child (PID: 2097) has exited! command=/usr/local/share/mythtv/metadata/Television/tvdb_test.conf -v, status=32256, result=126
I do not see any message like this in my log for mythmetadatalookup, even if tvdb_test.conf is marked executable, and even if verbose debug logging is on for mythmetadatalookup. Where do you see this?
comment:56 Changed 7 years ago by
Peter, right, it was in mythmetadatalookup log and -v system.
What I missed is that is that it happens when HouseKeeping? did RecordedArtworkUpdate?:
mythmetadatalookup --refresh-all-artwork --verbose general,system
comment:57 Changed 7 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
comment:58 Changed 7 years ago by
I have created ticket #13111 for the invalid executables that mythmetadatalookup is trying to run.
comment:62 Changed 7 years ago by
I think I've found a problem while working on a port to 0.27. The xml output by the v2 version is missing genre and runtime information that is provided by the v1 version.
Attachment nova-eclipse-v1.out was generated with this command:
/usr/share/mythtv/metadata/Television/ttvdb.py -l en -a US -N 76119 "Eclipse Over America"
Attachment nova-eclipse-v2.out was generated with the command:
PYTHONPATH=$HOME/git/mythtv/mythtv/bindings/python $HOME/git/mythtv/mythtv/programs/scripts/metadata/Television/ttvdb.py -l en -a US -N 76119 "Eclipse Over America"
Note that nova-eclipse-v1.out has these lines:
<categories>
<category type="genre" name="Documentary"/>
</categories>
which are missing from nova-eclipse-v2.out. And nova-eclipse-v1.out has this:
<runtime>60</runtime>
while nova-eclipse-v2.out has this:
<runtime/>
Watch Videos allows users to browse by genre and Watch Recording allows one to filter by category. And the MythCenter?-wide theme, for example, does display the runtime. in its "Video Info -> View Details" page. So losing genre and runtime has the potential to affect users who care about this info.
I can see the genre and runtime information is returned by thetvdb.com when I rerun the v2 command with the -d flag (see nova-eclipse-v2-debug.err). That file also shows that debug support is broken, since it ends with a stack trace and no XML written to stdout.
Changed 7 years ago by
| Attachment: | nova-eclipse-v2-debug.err added |
|---|
Changed 7 years ago by
| Attachment: | nova-eclipse.tgz added |
|---|
comment:63 Changed 7 years ago by
I put the xml files into a gzipped tar file, nova-eclipse.tgz, because they were rejected as spam when I tried to attach them individually.
comment:64 Changed 7 years ago by
I just checked fixes/29 and get the same issues as I described in comment 62 with my port to fixes/0.27.
comment:65 Changed 7 years ago by
I will look at them today or tomorrow. I have also seen another issue with priorities. do a -M Horizon and you dont get Horizon but "The Horizon" first which is wrong. Problem stems from using dict keys instead of lists.
comment:66 follow-up: 68 Changed 7 years ago by
pull request created.
genre and runtime fixed with new test.
please check for other issues.
priority was not an issue as it was sent by the server in the order I saw.
comment:67 Changed 7 years ago by
I tested the change and it is now including the information, and it is in the same format as in the old version of the program.
<categories>
<category type="genre" name="Crime"/>
<category type="genre" name="Drama"/>
<category type="genre" name="Horror"/>
</categories>
<studios>
<studio name="NBC"/>
</studios>
<runtime>45</runtime>
However, genres do not get updated in the MythTV database when running mythmetadatalookup. I chose a manual recording that had no metadata, changed its title to Hannibal and subtitle to Aperitif. I ran the metadata lookup job. It got the artwork, but no genres were added to the database for that show. The above display shows an extract of what is returned for that show. The programgenres table has no entry for that show and filtering in the frontend has that show under unknown genre. It looks like genres are not being populated by mythmetadatalookup. Perhaps only mythfilldatabase supports them.
comment:68 Changed 7 years ago by
Replying to markspieth:
pull request created.
I have merged it into master, so that somebody can add these to MythTV if needed. I did not merge into fixes.
comment:69 Changed 7 years ago by
From what I can tell, the programgenres table is only used for program data, not for recorded data. Maybe there's a grabber out there that uses thetvdb.com to populate the program table as well as the other program* tables, including programgenres?
I can confirm that Mark's latest commit makes a difference to video metadata. I added a video file to my library named .../Hannibal/s01e01.mpg, and ran "mythutil --scanvideos" under a few different conditions:
- 0.27 original ttvdb.py and bindings: Adds three records to videometadatagenre that link to the videometadata record for Hannibal/s01e01.mpg and records in videogenre for Crime, Drama, and Horror.
- My port of Mark's work to 0.27 without the commit that fixes genres and runtime: Does not add any records to videometadatagenre.
- My port of Mark's work to 0.27 with the commit that fixes genres and runtime: Adds the same records to videometadatagenre that the original ttvdb.py and bindings added.
I have also tested mytharchive's mythburn.py and the smolt code via "Submit your hardware profile" with the updated python bindings and found no regressions.
My port of Mark's work that support the new ttvdb api on 0.27 can be found here: https://github.com/faginbagin/mythtv/tree/0.27-ttvdb-api-v2
The latest commit to the above branch, 0de280b, fixes a test case for 'Nova - Eclipse Over America'. thetvdb.com has changed that episode from belonging to season 44 to being a special episode, plus tweaked a few other things like description, director and guest star. This commit should probably be pulled into master.
If anyone is interested in sticking with fixes/0.27 on ubuntu 14.04, You will also need to install or update some python packages as follows:
sudo apt-get install python-pip sudo pip install future requests_cache sudo pip install --upgrade urllib3 requests
Note, mythtv (using a github fork for fixes/0.27) will install python bindings in: /usr/lib/python2.7/site-packages However, the .deb package for libmyth-python from the mythbuntu PPA for fixes/0.27 will install them in: /usr/lib/python2.7/dist-packages I don't know if there's a similar difference in 0.28 or 29. I moved /usr/lib/python2.7/site-packages/MythTV to /usr/lib/python2.7/dist-packages/MythTV after moving the original version to /usr/lib/python2.7/dist-packages/MythTV.sav
comment:70 Changed 7 years ago by
Another way to get the missing packages if you are running trusty: https://lists.gt.net/mythtv/users/611123#611123
comment:71 follow-up: 72 Changed 7 years ago by
Would be nice to tell which python-urllib3 version is exactly needed. Also python-requests-cache and python-future are needed ? If yes which version ?
comment:72 follow-up: 73 Changed 7 years ago by
Replying to marillat <marillat@…>:
Would be nice to tell which python-urllib3 version is exactly needed. Also python-requests-cache and python-future are needed ? If yes which version ?
Please see the wiki article https://www.mythtv.org/wiki/TheTVDB_API_v2
comment:73 Changed 7 years ago by
Replying to pbennett:
Also python-requests-cache and python-future are needed ? If yes which version ?
Please see the wiki article https://www.mythtv.org/wiki/TheTVDB_API_v2
Ha, nice wiki :)
Thanks
The current ttvdb.py uses a package from http://github.com/dbr/tvdb_api that only supports the old api. It is stored in MythTV git under directory mythtv/bindings/python/MythTV/ttvdb
There are other packages available in github for ttvdb that do support the new api. Searching github for tvdb and filtering on python gives a bunch of results. Selecting ones updated recently gives those that support the new API.