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

Parse secondary album types from MusicBrainz #2200

Open
sepo opened this Issue Sep 19, 2016 · 26 comments

Comments

Projects
None yet
4 participants
@sepo
Copy link

sepo commented Sep 19, 2016

Problem

I've different radio plays. If i import one (e.g. https://musicbrainz.org/release-group/19e6209b-2ddc-30b8-9273-484bd075fe7b) to my library the ALBUMTYPE tag is always "other". The Musicbrainz site says "other + spokenword". I think the ALBUMTYPE should be "spokenword" instead. "other" is pretty useless.

Setup

  • OS: Fedora 24
  • Python version: 2.7.12
  • beets version: 1.3.19
  • Turning off plugins made problem go away (yes/no): no

My configuration (output of beet config) is:

directory: /mnt/Dateien/Musik
plugins: inline discogs chroma copyartifacts info badfiles edit scrub missing mbsync
paths:
    albumtype:soundtrack: Soundtracks/$album/$disc_and_track $title
    albumtype:audiobook: Hörbuch/$album/$disc_and_track $title
    albumtype:spokenword: Hörspiel/$albumartist/$album/$disc_and_track $title
    singleton:true: Tracks/$artist - $title
    comp:true: Compilations/$album%aunique{}/$disc_and_track $title
    default: Albums/$albumartist/$album%aunique{}/$disc_and_track $title
import:
    languages: de en
    detail: no
match:
    preferred:
        countries: ['DE', 'US', 'GB|UK']
        media: ['CD', 'Digital Media|File']
copyartifacts:
    extensions: .cue .CUE .LOG  .log .txt .TXT .jpeg .JPG .jpg .png .PNG
    print_ignored: yes
scrub:
    auto: yes
item_fields:
    disc_and_track: u'%02i.%02i' % (disc, track) if
                    disctotal > 1 else u'%02i' % (track)
edit:
    itemfields: album albumartist artist asin catalognum comments composer country day disc disc_and_track disctitle disctotal genre label language lyrics media month original_day original_month original_year title track tracktotal year    albumfields: album albumartist albumstatus albumtotal  
    albumtype asin catalognum country day disctotal genre label language month original_day original_month original_year year

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Sep 21, 2016

Thanks for the report! It looks like release groups are now allowed to have "secondary" types. See the XML here: https://musicbrainz.org/ws/2/release-group/19e6209b-2ddc-30b8-9273-484bd075fe7b

We currently only parse the primary type, but we should consider getting the rest of them too.

@sampsyo sampsyo added the feature label Sep 21, 2016

@sampsyo sampsyo changed the title ALBUMTYPE is "other" instead of "spokenword" or "other + spokenword" Parse secondary album types from MusicBrainz Sep 21, 2016

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Hi !
I'd like to work on this issue.
Can you help me get started ? (a GCI student here)

We want both the primary tag and the secondary tag to be displayed or check if the primary is a non-useful tag (for eg, 'other') and accordingly use the secondary tag ?

(P.S. I'm unable to claim tasks at the moment due to parental consents issue, should be able to soon)

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

Hi, @anshuman73! Great!

Let's start by using all the album types. We can just just join them all into a single string using a comma or a semicolon for now. That should work well for a first implementation, and we can consider more nuanced policies in a later phase.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Sure, also, does beets use a local musicbrainz server or uses the api via http ?
I couldn't find the code where the requests to musicbrainz is made.
Any specific file you can point me to ?

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

There's no local server; it uses the public API.

The code in question here is beetsplug/mbcollection.py. You'll see the API calls go though the python-musicbrainzngs library, which is linked above.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

oh, thanks !
I'll get to work right away.

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

@anshuman73 Ack! I answered that question incorrectly—I was thinking of a different thread.

The actually relevant code is in beets/autotag/mb.py. That's where we pull data out of the MB responses and format strings for metadata fields.

Sorry about that! 😳 Please disregard my earlier comment.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

@sampsyo No issues.

I believe this is where I'd be focusing at ?

https://github.com/beetbox/beets/blob/master/beets/autotag/mb.py#L280

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

Yep, exactly!

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Also, is there any place where I can see example json/dictionary data given out ?
just so I can figure out where the secondary tags are stored (as the value of what key)

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Comparing it to the xml example given above, I cannot find a tag named "type", so I'm assuming it defaults to the primary-type tag ?

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

There's no place where it currently gets printed out, so I'd suggest temporarily inserting a print call to output the JSON data.

If you're interested in seeing (and experimenting with) how the XML is parsed precisely, the py-mb-ngs code might be the right place to look: https://github.com/alastair/python-musicbrainzngs/blob/master/musicbrainzngs/mbxml.py#L477

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Ahh that's just perfect.
Discloses the whole schema

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

@sampsyo I think I've written the required code, I just can't figure out how to pass a release result to test the function.
Any suggestions ?

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

Sure! The test input is defined here:

release = {

You can add a test method to that class to demonstrate the new data extraction.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

hmm, well it doesn't exactly have the primary type and secondary type fields. I just wanted to confirm the key names and whether secondary types are stored as a list or not. (though I checked the same from https://github.com/alastair/python-musicbrainzngs/blob/master/musicbrainzngs/mbxml.py)

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

Aha! Yes, you'll want to try running beets so it executes a real query in that case—and then check the data that gets returned by adding a print call.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Hmm, I'll build it from my local source by simply executing the setup.py ?

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 28, 2016

Good question! This wiki page has instructions for getting set up with a local installation:
https://github.com/beetbox/beets/wiki/Hacking

In particular, I'd recommend pip install -e .. (It's sometimes convenient to use the --user flag too.)

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

o.O I never knew about such an easy pip method existing for local installation.
Thanks !

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Nov 28, 2016

Yay ! Everything's copacetic
Submitting a PR in a min..

screenshot from 2016-11-29 01-49-04

@mineo

This comment has been minimized.

Copy link
Contributor

mineo commented Nov 30, 2016

Since this (and #2294) have seen way more attention than my old #357, I'll just mention it here so it can get closed or something when this gets done.

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Nov 30, 2016

Arg! Thanks for the reminder, @mineo—we should have noticed that a long time ago. As you suggested back on that ticket, the right way to resolve this is probably to store the types in separate tags.

@anshuman73

This comment has been minimized.

Copy link
Contributor

anshuman73 commented Dec 1, 2016

@sampsyo
So would you suggest me doing that ^ in the #2294 PR or add that when I'm tackling #1547 .
I think it would be better to implement it while adding support for more fields (now that we've already logged the new types)

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Dec 1, 2016

Good question. Since it has to do with which fields we set on the imported items, let's worry about it after #1547.

@sampsyo sampsyo closed this in 318661b Jun 11, 2017

sampsyo added a commit that referenced this issue Jun 11, 2017

@sampsyo sampsyo reopened this Jun 11, 2017

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Jun 11, 2017

I just merged #2294, which should log the multiple album types to beets's debug output. If you're interested in finishing off this feature, please try importing a few albums with that debug output enabled to check what's going on with the difference between the current one-shot album type and the new data—that will help us decide how to handle the new album types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment