Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add parentwork plugin #3279

Merged
merged 27 commits into from Jun 9, 2019

Conversation

Projects
None yet
4 participants
@dosoe
Copy link
Contributor

commented May 31, 2019

New try, closes #2580 .

dosoe added some commits May 31, 2019

@sampsyo
Copy link
Member

left a comment

Here are a few code quality comments!

Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

It's very fun, I'm correcting my students' informatics exams (basic python course) and I'm getting informatics teached at the same time. Thanks for the comments! I never got healthy programming practices teached (at least not on this level).

dosoe added some commits May 31, 2019

@sampsyo
Copy link
Member

left a comment

Here's one docs comment. To continue the discussion on work_father: the real reason why I think this should change is that the name is confusingly gendered. Why is this work male, while the other thing is genderless? I would love to use a name that does not have a gender, as "mother" and "father" do. Would direct_parent or something work as the name for the thing that is not parent?

Show resolved Hide resolved docs/plugins/parentwork.rst Outdated
@codecov-io

This comment was marked as off-topic.

Copy link

commented Jun 3, 2019

Codecov Report

Merging #3279 into master will decrease coverage by 0.36%.
The diff coverage is 84.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3279      +/-   ##
==========================================
- Coverage   73.64%   73.27%   -0.37%     
==========================================
  Files          66       66              
  Lines       13736    13206     -530     
==========================================
- Hits        10116     9677     -439     
+ Misses       3620     3529      -91
Impacted Files Coverage Δ
beetsplug/parentwork.py 84.9% <84.9%> (ø)
beets/util/pipeline.py 92% <0%> (-0.89%) ⬇️
beetsplug/bpd/__init__.py 24.24% <0%> (-0.33%) ⬇️
beets/util/bluelet.py 25.29% <0%> (-0.16%) ⬇️
beets/ui/commands.py 80.18% <0%> (-0.03%) ⬇️
beets/util/confit.py
beets/ui/__init__.py 84.22% <0%> (+0.02%) ⬆️
beetsplug/fetchart.py 83.4% <0%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc9e26...92d005a. Read the comment docs.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I don't understand. How can my PR change the test coverage of fetchart?

dosoe added some commits Jun 3, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Arg, @codecov-io's output is pretty erratic and sometimes misleading. And we have configured it not to post these pull request messages, but sometimes it just does it anyway, for some reason…

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I have one more question: Works on MB change from time to time, and if the work changes, I would like the parentwork to be updated as well. A change in work happens only at a beet mbsync or maybe at a beet up if mb_workid gets implemented into mediafile. Right now, it doesn't get updated, so the only solution is to run beet parentwork -f again (with the --force option else it will only look for recordings with no parentwork) and this takes a huge time because it will recover all parentworks again (which is very slow). Could there be a way to apply beet parentwork --force only to items where the mb_workid got modified? Either by applying it only to recordings that got modified in the latest time (is there a way to list those? And I mean a modification in the database, not in the file itself like mtime) or by a more in-depth harmonization between mbsync, up and parentwork.

Upon verifying, there is plugins.send('write', item=self, path=path, tags=item_tags) at line 660 in library.py that seems exactly made for the purpose of signalling every plugin which tags is getting changed, however it tells when metadata is written into the files, where I look for changes in the database. How do I tell parentwork to listen to the corresponding call and kick in each time mb_workid gets changed?

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

One more thing, are there comments on the logging? It feels rather verbose right now, are you fine with that?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Interesting! Here's one thing I'm uncertain about: which of these two scenarios are you most concerned about adapting to?

  1. The user changes the track ID for an item. So you need to get a new work to match that.
  2. The user does nothing, but the MusicBrainz database changes to associate a new work with an existing track. The user wants to synchronize these new changes with the file.

The thing is that "1" above seems less likely to be an issue (how often do users really change the MBID for tracks they already have in their library?), but it's easier to deal with—there are a few ways we can consider to track when the MBID has gone out of sync with the work. "2" seems like much more of a real issue, but there's essentially nothing to be done about it—to know whether anything changed, you have to check with the MB server again, which essentially means doing beet parentwork -f. There is basically no way around this.


About logging: it's probably OK to be somewhat verbose in "debug" mode. But of course, brevity is the soul of wit. I'll take a look at the log messages now.

@sampsyo
Copy link
Member

left a comment

Some logging comments.

Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
@arcresu

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Regarding the issue of updating the parent work when things change, this is an instance of the more general issue with metadata fetching covered in #266. We're planning to change how plugins work behind the scenes, and hopefully as part of that process we can solve that issue in a more satisfying way. That's a bigger issue that will take some time to get right though.

For example we could model mbsync as a cache of remote metadata, so it would be possible to sync just the least-recently updated items rather than the entire library. Derived metadata like the parent work could be "invalidated" if the dependent metadata (work ID) changes, triggering an immediate recalculation. This is just brainstorming rather than a concrete proposal.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Another thought occurred to me about how to simplify the logging: you don't need to do this:

self._log.info('processing {} - {}', item.artist, item.title)

Instead, you can just do this:

self._log.info('processing {}', item)

and the item will get formatted in a sensible way automatically.

@arcresu
Copy link
Member

left a comment

I just had some small proofreading comments to add

Show resolved Hide resolved test/test_parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved docs/plugins/parentwork.rst Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved docs/plugins/parentwork.rst Outdated
Show resolved Hide resolved docs/plugins/parentwork.rst Outdated
Show resolved Hide resolved docs/plugins/parentwork.rst Outdated
@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Thanks for the detailed comments! I think I adressed them all.
Concerning the metadata change, what I would like to have is that parentwork notices when mbsync (or anyone else) modifies the mb_workid tag and kicks in to update the parentwork tag (and all associated tags).
While the mb_trackid almost never changes once in the library, the mb_workid can change (and I notice it changing from time to time), so what I'm looking for is a solution to 1. Is there already something implemented that could make it easier (maybe using register_listener from the BeetsPlugin class)?

@arcresu arcresu added the newplugin label Jun 6, 2019

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

I'm trying to read through https://beets.readthedocs.io/en/v1.4.9/dev/plugins.html#listen-for-events but I don't fully get it: I tried something like

class ParentWorkPlugin(BeetsPlugin):
    def __init__(self):
        ...
        if self.config['auto']:
            self.register_listener('trackinfo_received', self.find_work2)
    def find_work2(self, item):
        self.find_work(self, item, True)

This should call find_work each time we get data from MB. However, maybe the event that I should listen to is database_change and not trackinfo_received , as the second one would trigger even if there is no change. For the first one, the parameters are lib and model and I don't know how to get an item out of them to feed to find_work .
Additionally, as coded right now (with trackinfo_received) a beet mbsync fails:

Traceback (most recent call last):
  File "/usr/local/bin/beet", line 11, in <module>
    load_entry_point('beets==1.5.0', 'console_scripts', 'beet')()
  File "/usr/local/lib/python2.7/dist-packages/beets/ui/__init__.py", line 1266, in main
    _raw_main(args)
  File "/usr/local/lib/python2.7/dist-packages/beets/ui/__init__.py", line 1253, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/mbsync.py", line 75, in func
    self.singletons(lib, query, move, pretend, write)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/mbsync.py", line 96, in singletons
    track_info = hooks.track_for_mbid(item.mb_trackid)
  File "/usr/local/lib/python2.7/dist-packages/beets/autotag/hooks.py", line 574, in track_for_mbid
    plugins.send(u'trackinfo_received', info=track)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 489, in send
    result = handler(**arguments)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 149, in wrapper
    return func(*args, **kwargs)
TypeError: find_work2() takes exactly 2 arguments (1 given)

I'm puzzled, because I wrote find_work2 specifically to have only one argument.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Hi! I found my error. I changed it to:

class ParentWorkPlugin(BeetsPlugin):
    def __init__(self):
        ...
        if self.config['auto']:
            self.register_listener('database_change', self.find_work2)
    def find_work2(self, lib, model):
        self.find_work(model, True)

This however does not work entirely fine. A try with mbsync gives:

Requesting MusicBrainz release 05909b27-18c5-47dc-bcd8-971a7f25ab79
primary MB release type: album
Sending event: albuminfo_received
mbsync: applying changes to Johann Sebastian Bach; Richard Galliano - Galliano | Bach
Johann Sebastian Bach - Galliano | Bach - Art of Fugue, BWV 1080: Contrapunctus 1
  composer_sort:  -> Bach, Johann Sebastian
  work:  -> Die Kunst der Fuge, BWV 1080: I. Contrapunctus 1
  mb_workid:  -> 621651bc-7282-47ad-a401-d8be5a0988bd
  work_disambig:  -> catch all for arrangements
Sending event: database_change
parentwork: no composer for Johann Sebastian Bach - Galliano | Bach - Art of Fugue, BWV 1080: Contrapunctus 1; add one at https://musicbrainz.org/work/621651bc-7282-47ad-a401-d8be5a0988bd
parentwork: Work fetched: Die Kunst der Fuge, BWV 1080: I. Contrapunctus 1 - Johann Sebastian Bach
Johann Sebastian Bach - Galliano | Bach - Art of Fugue, BWV 1080: Contrapunctus 1
  parentwork: Die Kunst der Fuge, BWV 1080: I. Contrapunctus 1
  parent_composer_sort: Bach, Johann Sebastian
  parentwork_disambig: catch all for arrangements
  parent_composer: Johann Sebastian Bach
  mb_parentworkid: 621651bc-7282-47ad-a401-d8be5a0988bd
Sending event: database_change
Traceback (most recent call last):
  File "/usr/local/bin/beet", line 11, in <module>
    load_entry_point('beets==1.5.0', 'console_scripts', 'beet')()
  File "/usr/local/lib/python2.7/dist-packages/beets/ui/__init__.py", line 1266, in main
    _raw_main(args)
  File "/usr/local/lib/python2.7/dist-packages/beets/ui/__init__.py", line 1253, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/mbsync.py", line 76, in func
    self.albums(lib, query, move, pretend, write)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/mbsync.py", line 178, in albums
    apply_item_changes(lib, item, move, pretend, write)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/mbsync.py", line 40, in apply_item_changes
    item.store()
  File "/usr/local/lib/python2.7/dist-packages/beets/library.py", line 348, in store
    plugins.send('database_change', lib=self._db, model=self)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 489, in send
    result = handler(**arguments)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 143, in wrapper
    return func(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/parentwork.py", line 90, in find_work2
    self.find_work(model, True)
  File "/usr/local/lib/python2.7/dist-packages/beetsplug/parentwork.py", line 208, in find_work
    item.store()
  File "/usr/local/lib/python2.7/dist-packages/beets/library.py", line 348, in store
    plugins.send('database_change', lib=self._db, model=self)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 489, in send
    result = handler(**arguments)
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 137, in wrapper
    assert self._log.level == logging.NOTSET
AssertionError

Do you know what the error is? Could it be that it is listening to itself? From the error message, the error seems to come from the logging, but I don't know what is wrong there.

Another problem is that it now listens to itself, which means each time it changes a work, it will notice that the corresponding track has been changed and fetch it again.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Hi! Can we please leave this "automation" feature to a separate pull request? It would be awesome to close the book on the basic version before we start adding fancier features.

I was even going to suggest a simpler approach to this that might be a little less surprising to users. It seems little surprising that simple metadata changes with beet modify, which is typically a "local only" operation, could cause network requests and stuff. Instead, maybe the plugin could keep around a flexible attribute that says "here's the work ID that the current parent work is based off of." Then, when running for a second time, check whether that still matches the current work ID and re-fetch if so. This way, the heavy lifting only happens when users ask for it rather than truly automatically.

I don't exactly see the source of that error, but it might be caused by the presence of other plugins…

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Ok, so I added a tag mb_workid_current which is there for this purpose, do you thing the name is appropriate?
Also, I think that the plugin calling itself can be a real problem.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Seems reasonable to me! But can we leave this enhancement to a separate pull request? (You could even base the new branch on this branch and create the PR now.)

dosoe added some commits Jun 7, 2019

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Ok then this should be it!

@sampsyo
Copy link
Member

left a comment

OK, I think this really might be the last round of comments!

Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py
Show resolved Hide resolved beetsplug/parentwork.py
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated
Show resolved Hide resolved beetsplug/parentwork.py Outdated

dosoe and others added some commits Jun 8, 2019

Update docs/plugins/parentwork.rst
Co-Authored-By: Adrian Sampson <adrian@radbox.org>
Update docs/plugins/parentwork.rst
Co-Authored-By: Adrian Sampson <adrian@radbox.org>
Update docs/plugins/parentwork.rst
Co-Authored-By: Adrian Sampson <adrian@radbox.org>

@sampsyo sampsyo merged commit 9d184e3 into beetbox:master Jun 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Jun 9, 2019

sampsyo added a commit that referenced this pull request Jun 9, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Merged! Thank you for your long-term persistence in seeing this through!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.