Streamline album attributes modification behaviour and allow override via CLI

[JOJ0]

Description

• Fixes Album flexible attributes modifications are not inherited to tracks #4822
• Keeps the default behaviour of inheriting album (fixed) attributes modifications to album tracks.
• Fixes album modifications of flexible attributes to be inherited to album-tracks as well.
• Adds a new CLI option --noinherit/-I that prevents inheriting to album-tracks, which allows changing fixed/flexible attributes on an album only.

To Do

• Documentation.
• Changelog.
• Tests.

[JOJ0]

Hi @sampsyo we talked about this a couple of months ago and agreed that it would make sense to have the same behaviour of inheritance of album-mods no matter if fixed or flex attr #4742 (reply in thread)

Now before I add documentation and changelog to this PR I would like to discuss a usability thing: I just added a capital option of -I to beets modify. Would it make sense to add a non-capital option -i that states Inherit album-changes to tracks (default) in its help text? One one hand it would make it clear to the user immediately what the default behaviour is, on the other -i would be kind of never to be used, since it's the default anyway and it's not configurable. Am I making sense? I just want to make sure we don't overengineer this feature and make it as logical as possible. Certainly docs of modify will mention what the default behaviour is, and how it can be changed.

BTW I realised that there is one use-case I'm facing quite often myself where a --noinherit option makes sense: Genres. While tinkering with lastgenre plugin set to "track specific genres" I realized that an album-level genre is exiting and wants to be taken care of still. A mod -a genre=something command would just overwrite everything, which makes it impossible to fix album-level, while keeping track-level genres.

[sampsyo]

Thanks for getting this started!

At a very high level, I think it may make more sense to make the "inheritance" happen when making the change on the model, not when calling store. The store command is supposed to mean "take these temporary changes and commit to them, storing them to the on-disk database." In general, it is not supposed to change the values of fields; it is just supposed to store the changes that have already been made. Does that make any sense?

This could happen either in __setitem__, which we already override for Item although not Album yet:

beets/beets/library.py

Line 633 in 0c3f428

def __setitem__(self, key, value):

Or in the modify command, if we think this is a modify-specific behavior we want.

[wisp3rwind]

At a very high level, I think it may make more sense to make the "inheritance" happen when making the change on the model, not when calling store. The store command is supposed to mean "take these temporary changes and commit to them, storing them to the on-disk database." In general, it is not supposed to change the values of fields; it is just supposed to store the changes that have already been made. Does that make any sense?

My initial reaction was to agree with you, but after a brief glance at the code, I'm not so sure anymore: Album.store already has the track_updates/track_deletes mechanism. Even if one might design this differently now, it would also be unfortunate to introduce a second code path that propagates tags to items. Also, if I'm not mistaken, Albums don't keep their tracks around, but Album.items() fetches them freshly from the DB. I doubt we would want to do that in each __setitem__ call, but once per store seems reasonable (and, is already being done for track_updates).

[sampsyo]

Hmm; you're right about that—I hadn't thought that through completely (that store is where we currently "propagate" changes as it stands). Which means that you can think of synchronizing the album and item metadata as part of the "sync" that store represents. Thanks for catching this!

[JOJ0]

Well, thanks to both of you for clarifying @sampsyo @wisp3rwind. I have to admit that @sampsyo's first suggestion confused me a little, but no worries, I'm glad we seem to be back on track :-)

Good, before I move on to wrapping my head around all the tests that are failing now, any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?

Also any opinions on whether or not it makes sense to add an additional -i option. I would suggest to not do that. A single option -I (capital since we often see that in beets for negation) I'd consider sufficient.

[arsaboo]

@JOJ0 I thought -I is used for non-incremental or something. This is my go-to import command beet import -m -I -t ~/shared/music/

[sampsyo]

any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?

The main way would be to make this a proper beets configuration option (i.e., make it appear in config_default.yaml). These can be overridden by command-line flags.

[JOJ0]

@JOJ0 I thought -I is used for non-incremental or something. This is my go-to import command beet import -m -I -t ~/shared/music/

Hmm, yes that's true but actually I thought that its not problem to have the same letter for a different subcommand. beet modify -I is not beet import -I

But yeah if you guys can think of a different letter I'm up for suggestions.

beet mod -n better? (but -n often is somethin like --no-op in a lot of cli tools, didnt like it because of that)

[arsaboo]

If it does not conflict, I am fine with beet modify -I

[JOJ0]

any other suggestions if there is a better way than passing a kwarg through a few levels to make this "configurable"?

The main way would be to make this a proper beets configuration option (i.e., make it appear in config_default.yaml). These can be overridden by command-line flags.

Sorry for being unclear with my question. I used the word configurable but put it in double quotes because I was trying to say something like: ....any better way...to implement this?

Well, you did kind of answered to that as well. A regular beets config option which a cli option overrides, is probably a typicial form of implementing this. Now my initial idea was to have a config option but after thinking this through a couple of times I came to the conclusion that actually I don't consider it necessary. If I want to modify something, then a consistent default should be used - which we finally achieve with these changes. Finally album mods propagate to items for flex attrs as well (not touching the current behaviour for fixed attrs). If this is not what the user wants, they can use this new cli option to modify.

That were my thoughts, also trying to keep this change minimal. We can certainly discuss it. Maybe having an option does not hurt....

Any suggestions where an option like this would "live" in config_default.yaml?

Ideas:

modify:
    inherit: yes

modify_inherit: yes

inherit_modifications: yes

album_to_item_mods: yes

And now, if we would add such an option it would further be required to have a second cli option that can alter the behaviour.

Suggestion:

If user config option set to no, this can be used to alter:

beet modify -i

If user config option set to yes (or using the default), this can be used to alter:

beet modify -I

Thoughts welcome! Thanks!

[arsaboo]

I prefer:

modify:
    inherit: yes

I am also thinking that the default behavior should be to inherit attributes.

[sampsyo]

Aha! FWIW, I think it is totally fine to have this not be configurable, i.e., to have "propagation" be the default and not be overridable. If we do have a config option, it would also be OK to leave it undocumented, just as an "escape hatch" for people who really need the old behavior for some reason. In that case, I'd argue it should be a top-level option, not under a modify section, because it affects the global behavior of the beets library database and not just the modify command itself.

[JOJ0]

Guys! @sampsyo @wisp3rwind I'm a little lost here and need some brainstorming on how to proceed. A lot of tests are failing now. I've just looked into some of them yet.

First of, these two:

beets/test/test_edit.py

Lines 226 to 246 in b19b961

	def test_a_album_edit_apply(self, mock_write):
	"""Album query (-a), edit album field, apply changes."""
	self.run_mocked_command({'replacements': {'\u00e4lbum':
	'modified \u00e4lbum'}},
	# Apply changes.
	['a'],
	args=['-a'])
	self.album.load()
	self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT)
	self.assertEqual(self.album.album, 'modified \u00e4lbum')
	self.assertItemFieldsModified(self.album.items(), self.items_orig,
	['album', 'mtime'])
	def test_a_albumartist_edit_apply(self, mock_write):
	"""Album query (-a), edit albumartist field, apply changes."""
	self.run_mocked_command({'replacements': {'album artist':
	'modified album artist'}},
	# Apply changes.
	['a'],
	args=['-a'])

fail like this:

======================================================================
FAIL: test_a_albumartist_edit_apply (test.test_edit.EditCommandTest)
Album query (-a), edit albumartist field, apply changes.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jojo/.pyenv/versions/3.10.5/lib/python3.10/unittest/mock.py", line 1369, in patched
    return func(*newargs, **newkeywargs)
  File "/Users/jojo/git/beets/test/test_edit.py", line 251, in test_a_albumartist_edit_apply
    self.assertItemFieldsModified(self.album.items(), self.items_orig,
  File "/Users/jojo/git/beets/test/test_edit.py", line 87, in assertItemFieldsModified
    self.assertEqual(set(diff_fields).difference(allowed),
AssertionError: Items in the second set but not the first:
'mtime'
'albumartist'

----------------------------------------------------------------------
Ran 1 test in 0.239s

FAILED (failures=1)

======================================================================
FAIL: test_a_album_edit_apply (test.test_edit.EditCommandTest.test_a_album_edit_apply)
Album query (-a), edit album field, apply changes.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/.pyenv/versions/3.11.3/lib/python3.11/unittest/mock.py", line 1369, in patched
    return func(*newargs, **newkeywargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jojo/git/beets/test/test_edit.py", line 244, in test_a_album_edit_apply
    self.assertItemFieldsModified(self.album.items(), self.items_orig,
  File "/home/jojo/git/beets/test/test_edit.py", line 94, in assertItemFieldsModified
    self.assertEqual(set(diff_fields).difference(allowed),
AssertionError: Items in the second set but not the first:
'album'
'mtime'

----------------------------------------------------------------------
Ran 1 test in 0.140s

FAILED (failures=1)

After a lot of debugging I realized that the id field would also be inherited with my code (a00bafe#diff-d6c57a8deb6bf406de08bd231833fafb4515c984bf0d854a50a0e6cb3dcd3d1fR1388-R1389) and that is the reason the test fails.

So I fixed it like this:
998425f#diff-d6c57a8deb6bf406de08bd231833fafb4515c984bf0d854a50a0e6cb3dcd3d1fL1387-L1389

I'm not sure if this is what we want. High level question: I would say the 'id' field should always be prevented to propagate from album mods to items? If yes, there might be other fields that should always not be inherited. I could put a CONSTANT somewhere that collects these fields. Would that be the right way?

[JOJ0]

Next up is this test:

beets/test/test_library.py

Lines 1025 to 1030 in b19b961

	def test_albuminfo_change_artist_does_not_change_items(self):
	ai = self.lib.get_album(self.i)
	ai.artist = 'myNewArtist'
	ai.store()
	i = self.lib.items()[0]
	self.assertNotEqual(i.artist, 'myNewArtist')

Failing like so:

======================================================================
FAIL: test_albuminfo_change_artist_does_not_change_items (test.test_library.AlbumInfoTest.test_albuminfo_change_artist_does_not_change_items)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jojo/git/beets/test/test_library.py", line 1030, in test_albuminfo_change_artist_does_not_change_items
    self.assertNotEqual(i.artist, 'myNewArtist')
AssertionError: 'myNewArtist' == 'myNewArtist'

----------------------------------------------------------------------
Ran 12 tests in 0.216s

FAILED (failures=1)

It seems like change of artist (of the album) intentionally is not propagated to items which this check proves.

I could easily fix this test by passing inherit=False to store() when running the test:
9d0a4dd

but is this what we want in general? Should editing of artist perhaps never propagate to items?

On the other hand editing of an albumartist (of an album) is checked whether it successfully propagates to items with this test:

beets/test/test_library.py

Lines 1017 to 1023 in b19b961

	def test_albuminfo_change_albumartist_changes_items(self):
	ai = self.lib.get_album(self.i)
	ai.albumartist = 'myNewArtist'
	ai.store()
	i = self.lib.items()[0]
	self.assertEqual(i.albumartist, 'myNewArtist')
	self.assertNotEqual(i.artist, 'myNewArtist')

[sampsyo]

I'm not sure if this is what we want. High level question: I would say the 'id' field should always be prevented to propagate from album mods to items? If yes, there might be other fields that should always not be inherited. I could put a CONSTANT somewhere that collects these fields. Would that be the right way?

Good question. Yes, absolutely; we would never ever want the id field to propagate to items; that does indeed seem like a recipe for confusion. But it's a good question to ask whether there are other things that are similar… my gut reaction says "no, id stands alone," but I could very well be proven wrong. Maybe we should just watch out for other related effects where this kind of alignment is clearly undesirable?

I could easily fix this test by passing inherit=False to store() when running the test: 9d0a4dd

but is this what we want in general? Should editing of artist perhaps never propagate to items?

It seems like this test was expressly meant to "lock in" the old behavior, which we are now changing. I think what seems reasonable here is that we should split this into two tests: one with inherit=True and one with inherit=False, and we check that the resulting behavior differs in the expected way between the two.

[JOJ0]

Ready for a final review @sampsyo, maybe have a look at the docs I wrote, I also reworded existing stuff for mod -a a little. Is that easily understandable and correct like that: d30ac60

I left the check for ìd`simply inline with the if-statement. An extra GLOBAL_NEVER_INHERIT_THIS_FIELD or something like that felt overkill: 7313fe1

As a final step I tried to clarify and extend docstring and comments in this function. Nitpicking appreciated, since it is a very core-thing everybody who comes across, should have the chance to understand quickly. Does that make sense? a6937f0

[sampsyo]

Nice!!! This is looking great. I have just a couple of small suggestions, but I think we are basically in business!

[sampsyo]

I think you alluded to this in the main comment thread, but this could be a little easier to read if we put the entire stanza under a if inherit: condition. That would make it clear to the reader that nothing happens here when inherit=False.

[JOJ0]

Good point! I changed it to this: https://github.com/beetbox/beets/pull/4823/files#diff-d6c57a8deb6bf406de08bd231833fafb4515c984bf0d854a50a0e6cb3dcd3d1fR1386-R1392

[JOJ0]

That was a tough one but I think it was worth it. Thanks @sampsyo for all the guidance!