-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Store albumtypes
multi-value field consistently in-DB & in-tag, preventing continual file re-tagging
#4582
Conversation
albumtypes
multi-value field correctly in-DB, preventing continual file re-taggingalbumtypes
multi-value field correctly in-DB & in-tag, preventing continual file re-tagging
albumtypes
multi-value field correctly in-DB & in-tag, preventing continual file re-taggingalbumtypes
multi-value field consistently in-DB & in-tag, preventing continual file re-tagging
I thought I'd give this build a try after stumbling over #4528 and I'm getting the following error when running anything:
Removing the Thanks for working on this issue by the way! Edit to add: I don't think removing that named parameter was appropriate as I'm getting the same original issue but with semicolons:
|
@CharlesSchimmel Thank you - you've caught a mistake I made when incorporating some changes from another user, which passed through mostly because I haven't been able to add any tests to this PR, as mentioned above. I've force-pushed a fix: please do try again and confirm it's working with your media library! |
b57666f
to
b424a42
Compare
Thanks! I've built the change, and I'm still getting the original issue except with semicolons:
|
@CharlesSchimmel that looks like your tags might have been degraded already; could you check the TXXX/MusicBrainz Album Type frame of one of those files? that file you mentioned should have the type |
@mkhl Thanks, I think you're correct. Looks like a lot of my files have releasetype of I |
@CharlesSchimmel Thank you for checking further! Did you by any chance mean the albumtypes, not releasetypes, tag? More generally, this suggests we need a Benevolent Dictator's view on the already-corrupt situation: do we need to recover if the tags are now wrong and also (as I /suspect/ was the case with @CharlesSchimmel's library) the in-DB entries have been corrupted as well? I might very well be misunderstanding the scope/frequency of the underlying Issue's affected users, but is /automatically/ repairing the albumtypes/type data of folks who've upgraded to 1.6.0+ possible and/or desirable? |
@jpluscplusm Correct, that's what I meant, the releasetype tag seems to be a mirror of the albumtype tag. Personally, I'm re-importing the problem files with |
i think that should be out of scope. beets 1.6.0 writes problematic tags to the files but keeps the database intact, and this fix works fine if the database is intact. |
b424a42
to
6fa5480
Compare
PR branch rebased against master, following the addition of the expected test failure in #4586. Another commit added, changing the test to reflect that it shouldn't be failing after this PR is merged [to the best of my pythony- and beetsy- abilities!] |
Some test seem to be failing due to not expecting a list-valued |
@jpluscplusm would it help if someone would explain how those failing tests could be modified to pass? Is there anything else that would help to kickstart this PR? I think it's a nasty bug and it covers a lot of ground. Thanks for your heroic efforts so far! |
@JOJ0 Hello 👋 Yes, any assistance any at all would be very welcome. I feel very unsure in proposing changes beyond the current state because:
I really really don't want to be responsible for proposing a "fix" that -- for instance -- only deals with my newcomer/fresh-library use case, whilst leaving long-time users in a potentially worse place. Any assistance or guidance is very very welcome! |
Hi, first of all appologies for letting you hang there for another ten days... Regarding your worries: At the end of the day, it's the beets project who is responsible because they merged something into the main code. If that breaks something, a contributor can't be made responsilbe for that. Certainly we will be happy if you would still support us and help moving forward with analysis and fixing. My personal opinion is: We are in this together and I only can be grateful that people like you are brave enough to submit a fix for such a tricky problem. Speaking of, last weekend I fiddled around a little with the currently failing unittests wisp3rwind mentionend above. I was not successful so far but will continue working on it. Did you give it a try yet? Also I'm still trying to wrap my head around "the problem" as a whole. What I understand so far is:
Please correct me if I'm technical wrong with any of this or give me your own opinion if you understood differently. My conclusion now would be:
Thus, I have the following ideas of how this could be tackled further:
Fortunately proper hints around such a tutorial are provided here already: #4582 (comment) Where would a tutorial like this be placed is what I'm unsure about: Release notes? Add as the final answer in the existing github issue to this PR? Somewhere in the documentation? All of this places, just to make sure ?! Feel free to give your opinion on these thoughts. |
Some database examples from my dev environment. We see that the releases on label "Music man records" are pretty messed up and I don't think that there is any possible auto-fix other then suggesting to reimport: The second "Astrophonica" release and the "Human Imprint" one are, as the uuid-style mb_albumid reveals, tagged using MusicBrainz, the rest is tagged from Discogs. We see that the albumtypes field is empty on those fields. The "Music Man" Records "I think" where tagged with MusicBrainz before and then retagged with Discogs. If that is not true, then my assumption about "the problem is only existing with MB-tagged albums" is not true! |
To prove my assumption that Discogs tagged files don't touch the albumtypes field let's do the following experiment. Let's focus on the 3 Music Man Records releases. I just reimported the last one from MusicBrainz (catalognum: MMCD026). We see that Now I reimport from Discogs and get: MM129A is the Catalog number we are taking about and we see that the Discogs metadata source plugin did not care about the That proves my assumption that at least this source plugin, doesn't touch Final note: Certainly in such a situation a writing to file tags using Hope that clarifies and doesn't confuse further :-) |
First of all: Thank you!
Let me preface with:
I don't think that's technically correct. I also believe they are separate in the MusicBrainz API, but beets tries to derive the albumtype when reading files as being the first element of albumtypes. This can lead to the secondary problem (that this PR doesn't try to address) where
I believe that's correct.
Yes it does.
I believe that writing to the file's tag won't be modified, only reading it and matching it against the database get fixed.
Yes, and we seem to agree that it doesn't need to.
I'd recommend adding it as a page in either the wiki or the documentation, then linking to that page from this PR, from the issue it fixes, and prominently in the release notes. |
This can be done quite easily, if you don't mind effectively retagging your whole library in the process: |
I finally found time to look into this and I think I have a solution. There is 2 things we need to include into this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to this PR's branch with the fix I just described. Sorry for hijacking, I hope that's ok! :-)
@@ -55,7 +55,7 @@ def _atypes(self, item: Album): | |||
bracket_r = '' | |||
|
|||
res = '' | |||
albumtypes = item.albumtypes.split('; ') | |||
albumtypes = item.albumtypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item.albumtypes will always return a list already,
@@ -106,6 +106,6 @@ def _set_config(self, types: [(str, str)], ignore_va: [str], bracket: str): | |||
|
|||
def _create_album(self, album_types: [str], artist_id: str = 0): | |||
return self.add_album( | |||
albumtypes='; '.join(album_types), | |||
albumtypes=album_types, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also when creating the test-album we can rely on the DelimitedString
type to translate to a ;
-delimited string for us.
Hi @mkhl, thanks so much for correcting and clarifying, that was tremendously helpful for moving on with this topic. Very much appreciated :-) As for the location of the "manual fixing advice" I would prefer to add it to the docs instead of the wiki. The wiki is a place that I have to admit I personally haven't looked much into as a beets user. Now that I look at it, there is a lot of interesting and developer-centric things in there (Shame on me for not realising this earlier :-~) :-) ) So one place I would like to add it in the docs is the already mentionend albumtypes plugin docs . A link to the howto would suffice here. But also for users that don't use that plugin a properly fixed So I'm still looking for a place in the docs where a tutorial containing of 3 to 4 steps should live......maybe the wiki is a good place for that afterall. All the places we would like to mention it could then link to the wiki. Do you have an idea where you would see such a tutorial? Maybe create something like a "known issues" chapter and add it there? |
Sanity Check & Fix for #4528The resolution for a bug that was introduced in beets 1.6.0 requires manual intervention. The issue was tracked in issue #4528. The bug usually shows itself by trying to repeatedly populate the When Have I been infected?If you've never used
FixThe quickest way to fix is by using the
Alternatively, or if you don't use the
Fix Non-MusicBrainz-tagged albumsThere might be albums in your library where initially MusicBrainz was used as the metadata source and later on a retag with another metadata source plugin (Discogs, Spotify, ...) was used. The existence of such broken albums can be verified with:
The best solution for these broken database fields is to "zero them out". This command will ask for confirmation. Answer
VerifyTo verify whether the issue is fixed: Then try a real write to an album: Read the tags from disk into the db again: and have a look at the albumtypes field. It should show a list of strings delimited with |
Hi thanks for the hint. I have to admit I didn't have the mbsync plugin on my radar since I don't use. But yes, essentially it does the same as the desired part of reimporting: It overwrites the |
1000% ok! Please feel to take this PR anywhere the project needs ... including closing and starting again if that'd best serve the purpose of the fix! I'm just happy if my initial code or thoughts were at all helpful :-) |
Potentially related: I do still notice an issue, where beets will keep overwriting the |
Hi @judemille, indeed there seems to still be an issue. I can confirm that 'live' doesn't seemt to be recognized, while writing as well as while reading:
|
Ah, there is one flaw in my test above. I check for the plural |
FWIW this is the issue I was referring to above:
If an album has multiple types the MusicBrainz API seems to indicate that one of those is the "primary" type, and beets stores that in the db as the Writing the |
@mkhl yes that is super helpful! Thanks! I have to admit I managed to forget already that we are still not through with this problem and that you already explained that above. Sorry! I will try to write a feature proposal in a new issue and then let's discuss over there what ways we could imagine to get rid of this remaining issue. |
No worries, glad the info is helpful! |
I used your words @mkhl, hope that is ok! Thanks again! #4715 (comment) |
@JOJ0 Your comments (and guide on solving the issue) has been helpful but I think something has changed that has broken your query for square brackets:
I get a bunch of results for this query, but after manually inspecting the library database file, I am convinced that I am still battling this tag (and I think a few others) always wanting to write. Looks like #4715 is the ongoing discussion of this one. EDIT: Hmmm... getting rid of the |
@taylorthurlow If you dont have square brackets in your albumtypes, then indeed #4715 is the way to go. |
Hi
|
How did you fix it? Seems like your db still has broken data. Have you had a look into the db directly with sqlite client (as a last resort solution) |
There are no a,l,b,u,m or single letter entries listed if I do: |
Oh sorry for not reading closely enough. Interesting...which OS? I'll see if I can repro with FLAC files on macOS |
Debian 12. |
Hi again,
Did you also check |
@MarkTerMors If all else fails and you dont use any plugin that requires |
Given that all else has failed, I deleted everything and set up a new venv with beets installed in it. It works now but I have no idea what was wrong. |
Hmm, glad you got it to work. Well, Python venvs sometimes can be broken as hell. I had weird things with them often, where recreating just helped. |
Store `albumtypes` multi-value field consistently in-DB & in-tag, preventing continual file re-tagging
Description
This PR proposes a tentative fix for #4528, where everyone with a 1.6.0 version of beets and with any multi-value
albumtypes
stored in their DB (or acquired after upgrading to 1.6.0+, via new music or data source refreshes) will see Beets continually re-tagging all the files which have multiplealbumtypes
stored.Additionally, as per the initial report in #4528, there's some interplay where even single
albumtypes
values (e.g.album
) are getting turned into multi-value tags, and rewritten on everybeets write
.I wrote the first version of the fix, which is included in the initial 2 commits of the PR branch. @mkhl was kind enough to review from a more Python-aware perspective, and from a place of having an existing Beets library, and I've realigned the PR branch with their known-better version. (I /don't/ have such a library (I encountered this issue on first setup), so I'm inclined to trust their code more!)
I strongly suggest that this only be merged after tests are added that would have caught this problem before 1.6.0 was released. I've added no tests in this PR as the relevant parts of the Beets test suite quickly put me out of my depth. I really think it's worth getting a failing test in place, before merging this (I'll happily rebase, or anything else needed) as I /believe/ this bug will be causing any Beets user on 1.6.0+ to be re-tagging much/all of their library on each
write
.Thanks for an awesome tool, @sampsyo - I hope this PR helps in some small way!
Fixes #4528
To Do
docs/
to describe it.)docs/changelog.rst
near the top of the document.)