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

Stop perpetually writing mb_artistid, mb_albumartistid and albumtypes fields #5540

Merged
merged 3 commits into from
Dec 15, 2024

Conversation

snejus
Copy link
Member

@snejus snejus commented Dec 7, 2024

This PR fixes an issue where the beet write command repeatedly shows differences for certain fields (mb_artistid, mb_albumartistid, albumtype) even after writing the tags.
This happens because these fields are actually stored as lists in the media files (mb_artistids, mb_albumartistids, albumtypes), but beets maintains both single and list versions in its database.

This PR addresses this issue in a non-invasive way: the fix ensures consistency between single fields and their list counterparts by:

  1. When setting a single field value, making it the first element of the corresponding list
  2. When setting a list, using its first element as the single field value

This resolves long-standing issues #5265, #5371, and #4715 where users experienced persistent "differences" in these fields despite writing tags multiple times.

Changes:

  • Added ensure_consistent_list_fields() function to synchronize field pairs
  • Applied this function during metadata application
  • Added tests for all field combinations

Fixes #5265, #5371, #4715

Note: your music needs to be reimported with beet import -LI or synchronised with beet mbsync in order to fix these fields!

@snejus snejus self-assigned this Dec 7, 2024
@snejus snejus requested review from bal-e and JOJ0 December 7, 2024 09:15
@snejus snejus changed the title Stop\ perpetually\ writing\ \mb_artistid\,\ \mb_albumartistid\\ and\ \albumtypes\\ fields Stop perpetually writing mb_artistid, mb_albumartistid and albumtypes fields Dec 7, 2024
@JOJ0 JOJ0 self-assigned this Dec 9, 2024
@JOJ0
Copy link
Member

JOJ0 commented Dec 9, 2024

This is so great @snejus. Finally, someone! So far it looks like it's the right thing to do. I'll try find time to look more closely and even play around with it in the next few days.

@snejus snejus force-pushed the fix-perpetually-different-artist-ids-albumtypes branch from d9fb59d to 7bdb0b5 Compare December 9, 2024 07:08
@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

Hi @snejus, finally found some time to ressurect my beets-dev environment to play around a little. Unfortunately not the best results yet. It could certainly be that my dev-library is totally messed up. I didn't thoroughly check or clean it up yet. This is what I get:

Tests run fine

pytest -v test/test_autotag.py -k test_ensure_consistent
=================================================================================== test session starts ===================================================================================
platform linux -- Python 3.11.3, pytest-7.4.0, pluggy-1.2.0 -- /home/jojo/.pyenv/versions/beets/bin/python
cachedir: /tmp/pytest_cache
rootdir: /home/jojo/git/beets
configfile: setup.cfg
plugins: anyio-3.7.0
collected 105 items / 84 deselected / 21 selected

test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value0-mb_artistid-mb_artistids] PASSED                                                                         [ 1/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value0-mb_albumartistid-mb_albumartistids] PASSED                                                               [ 2/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value0-albumtype-albumtypes] PASSED                                                                             [ 3/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value1-mb_artistid-mb_artistids] PASSED                                                                         [ 4/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value1-mb_albumartistid-mb_albumartistids] PASSED                                                               [ 5/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value1-albumtype-albumtypes] PASSED                                                                             [ 6/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value2-mb_artistid-mb_artistids] PASSED                                                                         [ 7/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value2-mb_albumartistid-mb_albumartistids] PASSED                                                               [ 8/21]
test/test_autotag.py::test_ensure_consistent_list_fields[None-list_value2-albumtype-albumtypes] PASSED                                                                             [ 9/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value3-mb_artistid-mb_artistids] PASSED                                                                            [10/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value3-mb_albumartistid-mb_albumartistids] PASSED                                                                  [11/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value3-albumtype-albumtypes] PASSED                                                                                [12/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value4-mb_artistid-mb_artistids] PASSED                                                                            [13/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value4-mb_albumartistid-mb_albumartistids] PASSED                                                                  [14/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value4-albumtype-albumtypes] PASSED                                                                                [15/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value5-mb_artistid-mb_artistids] PASSED                                                                            [16/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value5-mb_albumartistid-mb_albumartistids] PASSED                                                                  [17/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value5-albumtype-albumtypes] PASSED                                                                                [18/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value6-mb_artistid-mb_artistids] PASSED                                                                            [19/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value6-mb_albumartistid-mb_albumartistids] PASSED                                                                  [20/21]
test/test_autotag.py::test_ensure_consistent_list_fields[1-list_value6-albumtype-albumtypes] PASSED                                                                                [21/21]

============================================================================ 21 passed, 84 deselected in 0.19s ============================================================================

beet write still gives me loads of these

...
Nikki Nair - Renormalization Support Group - 03 - It’s NP-Complicated  (2022/Unknown)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  mb_artistid:  -> 6341383
  mb_albumartistid:  -> 6341383
  albumtype:  -> 12"
Sending event: write
Sending event: after_write
Sending event: database_change
Nikki Nair - Renormalization Support Group - 04 - Donut Time (2022/Unknown)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  mb_artistid:  -> 6341383
  mb_albumartistid:  -> 6341383
  albumtype:  -> 12"
Sending event: write
Sending event: after_write
Sending event: database_change
Sending event: cli_exit
smartplaylist: Updating 21 smart playlists...
...

What else can I do to help/debug?

@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

I forgot to mention - you need to reimport your music first! Try beet import -LI some of these albums, and then re-run beet write?

@snejus snejus force-pushed the fix-perpetually-different-artist-ids-albumtypes branch from 7bdb0b5 to efffcd7 Compare December 14, 2024 19:26
@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

beet mbsync may also do the job and it should be much quicker

@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

Thanks for the hint! Yeah mbsync is a good idea but reimport for this one album was good enough for now. Much better now:

Nikki Nair - Renormalization Support Group - 01 - Plug  (2022/UK)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  albumtype:  -> 12", 33 ⅓ RPM, EP
Nikki Nair - Renormalization Support Group - 02 - Where Are U  (2022/UK)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  albumtype:  -> 12", 33 ⅓ RPM, EP
Nikki Nair - Renormalization Support Group - 03 - It’s NP-Complicated  (2022/UK)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  albumtype:  -> 12", 33 ⅓ RPM, EP
Nikki Nair - Renormalization Support Group - 04 - Donut Time (2022/UK)  [APHA027] f:MP3-128kCBR a:70-01/ASIS |Electronic, Breakbeat, Electro
  albumtype:  -> 12", 33 ⅓ RPM, EP

but note that data_source is Discogs in this case! Maybe we have to fix something in the Discogs plugin? Or it hast to do with the weird characters in there?

data in db is now:

$ sqlite3 ~/.config/devbeets/library.db "select artist,title,mb_artistid,mb_albumartistid,albumtype from items where mb_artistid == '6341383';"
Nikki Nair|Plug |6341383|6341383|12", 33 ⅓ RPM, EP
Nikki Nair|Where Are U |6341383|6341383|12", 33 ⅓ RPM, EP
Nikki Nair|It’s NP-Complicated |6341383|6341383|12", 33 ⅓ RPM, EP
Nikki Nair|Donut Time|6341383|6341383|12", 33 ⅓ RPM, EP

so at least this albumtype data looks properly

@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

Did an mbsync and all MusicBrainz tagged items are fine now btw! 👍

@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

I have a suspicion why albumtype is a problem with Discogs plugin. It only saves a single field albumtype, but not albumtypes.

@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

I have a suspicion why albumtype is a problem with Discogs plugin. It only saves a single field albumtype, but not albumtypes.

This change should handle such cases and set albumtypes = [albumtype]

@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

I have a suspicion why albumtype is a problem with Discogs plugin. It only saves a single field albumtype, but not albumtypes.

This change should handle such cases and set albumtypes = [albumtype]

Ok I understand, that's odd. Anyway I just scrambled together some code that really sets albumtypes in the first place on a re/import. Do you mind if I push a commit to this branch? Just so we can talk about it better. You can revert it later on.

@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

go ahead!

beetsplug/discogs.py Outdated Show resolved Hide resolved
beetsplug/discogs.py Outdated Show resolved Hide resolved
@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

Could you by any chance provide beet info and beet info -l outputs for one of those tracks that still writes albumtype field?

@snejus snejus force-pushed the fix-perpetually-different-artist-ids-albumtypes branch from fb18c62 to efffcd7 Compare December 14, 2024 21:23
@JOJ0
Copy link
Member

JOJ0 commented Dec 14, 2024

There you go:

MORESOUNDS - Show Your Respect - 02 - Forward (2021/UK)  [APHALTD003] f:MP3-128kCBR a:70-01/ASIS |Electronic, Drum n Bass, Halftime
  mb_artistid:  -> 1253344
  mb_albumartistid:  -> 1253344
  albumtype:  -> 10", Limited Edition, White Label
$ beet info -l forward | grep albumtype
           albumtype: 10", Limited Edition, White Label
          albumtypes:
$ beet info forward | grep albumtype

(Missing entirely from the file tag.)

Sufficient or you really want to see all tags and lib fields?

database, just in case:

sqlite3 ~/.config/devbeets/library.db "select artist,title,mb_artistid,albumtypes,albumtype from items where artist='MORESOUNDS';"
MORESOUNDS|Show Your Respect|1253344||10", Limited Edition, White Label
MORESOUNDS|Forward|1253344||10", Limited Edition, White Label

@snejus
Copy link
Member Author

snejus commented Dec 14, 2024

Thanks. I messaged you on Zulip asking for one of the files in question so that I can test it out

@snejus snejus force-pushed the fix-perpetually-different-artist-ids-albumtypes branch from efffcd7 to 550a9a8 Compare December 14, 2024 21:39
@snejus
Copy link
Member Author

snejus commented Dec 15, 2024

I found that album metadata was applied after these corrections, where albumtypes field was overwritten with an empty list (since discogs does not populate any). I made an adjustment where the corrections are applied after album metadata is applied. Also added these corrections to singletons.

@snejus snejus force-pushed the fix-perpetually-different-artist-ids-albumtypes branch from 5001005 to a091c2e Compare December 15, 2024 00:12
Copy link
Member

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge it! 🎉

I still found ways to trigger the issue but that was with files that I imported as-is, so manual corrections using beet mod albumtype=something query and beet mod albumtypes=something query are the way to fix those. The apply_metadata... methods are not run in that case, thus it's not possible to correct those on (re)import. We shouldn't try to fix that here, since there "just is" quite some things in beets that depend on properly tagging with a tagging source. Importing as-is, just has flaws. It is what it is, and fixing that is for another day. :-)

Most of all: Thank you! Fixing this was long due. You secured yourself an entry in the books of "beets history" with that! ❤️

beets/autotag/__init__.py Show resolved Hide resolved
beets/autotag/__init__.py Outdated Show resolved Hide resolved
beets/autotag/__init__.py Outdated Show resolved Hide resolved
beets/autotag/__init__.py Show resolved Hide resolved
beets/autotag/__init__.py Show resolved Hide resolved
@snejus snejus merged commit 9110a11 into master Dec 15, 2024
13 checks passed
@snejus snejus deleted the fix-perpetually-different-artist-ids-albumtypes branch December 15, 2024 14:17
@arsaboo
Copy link
Contributor

arsaboo commented Dec 15, 2024

@snejus Seeing this error with beet mbsync:

Traceback (most recent call last):
  File "/home/arsaboo/.local/bin/beet", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/arsaboo/.local/lib/python3.12/site-packages/beets/ui/__init__.py", line 1870, in main
    _raw_main(args)
  File "/home/arsaboo/.local/lib/python3.12/site-packages/beets/ui/__init__.py", line 1849, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/home/arsaboo/.local/lib/python3.12/site-packages/beetsplug/mbsync.py", line 73, in func
    self.albums(lib, query, move, pretend, write)
  File "/home/arsaboo/.local/lib/python3.12/site-packages/beetsplug/mbsync.py", line 183, in albums
    any_changed_item = items[0]
                       ~~~~~^^^
IndexError: list index out of range

I will create a PR to check for empty albums.

@snejus
Copy link
Member Author

snejus commented Dec 15, 2024

@arsaboo could you check whether this errors out before this change was merged? Doesn't seem like it's related

@arsaboo
Copy link
Contributor

arsaboo commented Dec 15, 2024

It is quite possible that those errors are unrelated to this PR (I ran mbsync for the first time today). May be it was another bug that was always there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants