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.

Sign up for GitHub

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

fetchart: defer file removal config option evaluation #5244

Merged
merged 2 commits into from
Sep 16, 2024
Merged

fetchart: defer file removal config option evaluation #5244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90f0ae2
fetchart: defer file removal config option eval
mgoltzsche May 13, 2024
00add27
fetchart: apply review remarks
mgoltzsche Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions beetsplug/fetchart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,10 +1253,6 @@ def __init__(self):
self.cautious = self.config["cautious"].get(bool)
self.store_source = self.config["store_source"].get(bool)

self.src_removed = config["import"]["delete"].get(bool) or config[
"import"
]["move"].get(bool)

self.cover_format = self.config["cover_format"].get(
confuse.Optional(str)
)
Expand Down Expand Up @@ -1298,6 +1294,10 @@ def __init__(self):
for s, c in sources
]

@staticmethod
def _is_source_file_removal_enabled():
return config["import"]["delete"] or config["import"]["move"]

# Asynchronous; after music is added to the library.
def fetch_art(self, session, task):
"""Find art for the album being imported."""
Expand Down Expand Up @@ -1340,10 +1340,11 @@ def assign_art(self, session, task):
"""Place the discovered art in the filesystem."""
if task in self.art_candidates:
candidate = self.art_candidates.pop(task)
removal_enabled = FetchArtPlugin._is_source_file_removal_enabled()

self._set_art(task.album, candidate, not self.src_removed)
self._set_art(task.album, candidate, not removal_enabled)

if self.src_removed:
if removal_enabled:
task.prune(candidate.path)

# Manual album art fetching.
Expand Down
2 changes: 2 additions & 0 deletions docs/changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,8 @@ New features:
* Add support for `barcode` field.
:bug:`3172`
* :doc:`/plugins/smartplaylist`: Add new config option `smartplaylist.fields`.
* :doc:`/plugins/fetchart`: Defer source removal config option evaluation to
the point where they are used really, supporting temporary config changes.

Bug fixes:

Expand Down
10 changes: 7 additions & 3 deletions test/plugins/test_art.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,9 +825,13 @@ def test_leave_original_file_in_place(self):
self.assertExists(self.art_file)

def test_delete_original_file(self):
self.plugin.src_removed = True
self._fetch_art(True)
self.assertNotExists(self.art_file)
prev_move = config["import"]["move"].get()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of editing the config try patching it (or the relevant method in fetchart.py):

from unittest.mock import patch
...

        with patch("beetsplug.fetchart.FetchArtPlugin.should_delete", True):
            ...

Copy link
Contributor Author

@mgoltzsche mgoltzsche Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hm, I am not sure I follow you here:

  1. I don't see a should_delete property or method within the FetchArtPlugin that could be mocked this way.
  2. The point of the test is to ensure that another plugin that calls the fetchart plugin CAN temporarily overwrite the config effectively in the way the test does it. This is why I created the PR in the first place. Mocking that piece of logic away within the test makes the test pointless.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for linking up the reason why it's needed - this makes more sense now.

I'm concerned about the idea of a plugin overriding global settings, especially a setting like move. I'm not sure whether this would be the expected behaviour for users that have setups with move: false.

Meanwhile, I think I see why this may be required for your plugin which firstly downloads and only then imports music.

If your plugin functionality completely depends on move being set to True, have you considered validating it instead?

if not config["import"]["move"]:
    raise confuse.ConfigValueError("'move' must be set to True in order to use ytimport")

Ultimately, from beets implementation perspective, allowing global config adjustments in flight is an anti-pattern and in my opinion should be avoided.

Copy link
Contributor Author

@mgoltzsche mgoltzsche Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure whether this would be the expected behaviour for users that have setups with move: false.

The user is not calling beet import but beet ytimport which has its own configuration (that it also applies to the import operation). Thus, the ytimport plugin overwriting some import parameters is not really unexpected to the user imho.
Also, ytimport downloads into and imports from a temporary directory that it manages itself. As a user I don't want to edit/switch my import configuration every time I perform a different import operation - I have different CLI commands for that.

Ultimately, from beets implementation perspective, allowing global config adjustments in flight is an anti-pattern and in my opinion should be avoided.

I agree it is not great. For instance if somebody wanted to run multiple imports in parallel they couldn't do that within a single Python process, each with different configuration like this.
However, as of now there is no need to run multiple in parallel and the alternative is to start a separate beet import Python process and overwrite the configuration via CLI options.

Also, the other configuration options can be temporarily overwritten this way already.

Therefore why not allow a plugin the workaround of also overwriting the move configuration option this way within its CLI?

Copy link
Member

Choose a reason for hiding this comment

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

The user is not calling beet import but beet ytimport which has its own configuration (that it also applies to the import operation). Thus, the ytimport plugin overwriting some import parameters is not really unexpected to the user imho.

For me as a user this is unexpected in the following ways:

  1. First and foremost - the fact that this command does not respect my global move configuration and always moves music into my library is not documented (didn't see it in the readme nor the command help).
  2. I have no control of it (the command doesn't have --move nor --no-move flag)

Based on other commands that may move files, what I'm used to is

  1. By default my global configuration being respected - if my configuration does not allow any filename adjustments, no adjustments are being made
  2. Being able to override global configuration using the flag --move, --write etc.

My previous beets configuration had move: no and copy: no preventing any path adjustments during import. After the import, I did some additional processing on the files, and only then ran a command to move the files to my library directory.

As a user I don't want to edit/switch my import configuration every time I perform a different import operation.

I'm struggling to think of a use case where I'd have to switch the configuration, i.e. want to have move: no for the import command and move: yes for ytimport. I presume users may want the same configuration to apply for both commands consistently?

ytimport downloads into and imports from a temporary directory that it manages itself

This makes sense. It sounds like once ytimport downloads a file to the temp directory, the situation is more or less equivalent to a user importing a local file manually. Did you consider delegating the import to the beet import command?

Copy link
Contributor Author

@mgoltzsche mgoltzsche Sep 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

First and foremost - the fact that this command does not respect my global move configuration and always moves music into my library is not documented (didn't see it in the readme nor the command help).

True but like I said: ytimport is importing from a temporary directory that it manages itself. In a way the fact that ihe ytimport plugin uses the beets' import plugin under the hood is more of an implementation detail.

I have no control of it (the command doesn't have --move nor --no-move flag)

You kind of have that control: the ytimport plugin supports a --no-import option which makes it download the files but not import them yet. Calling beet ytimport imports all files within the temp download directory that weren't imported previously and that weren't skipped explicitly. Leaving the files in the temp dir after they were imported makes no sense to me. The additional postprocessing and renaming of files after the import that you mentioned sounds more of a hacky workaround than a well designed/clean workflow to me. Either way your workflow would also work with the ytimport plugin if you call it with --no-import and then run your custom import command(s) afterwards.

I'm struggling to think of a use case where I'd have to switch the configuration, i.e. want to have move: no for the import command and move: yes for ytimport. I presume users may want the same configuration to apply for both commands consistently?

Here's an example use-case: I am importing from another library. Because I don't want that to be a destructive operation, I just copy the files but leave the source files untouched. Because of the destructiveness of the move operation, I'd actually like it to be set to false in my beets configuration and just have the ytimport plugin enable it temporarily for its managed temp dir import.

Did you consider delegating the import to the beet import command?

Yes, in fact it was like that initially before I changed it to using the Python API as opposed to the CLI. This was to avoid spawning a new Python process for the import. Apart from the unnecessary overhead, spawning a new Python process comes with the disadvantage that global beets CLI options are not applied to the beets child process oob and would each have to be passed through to the child process explicitly, covering all beets options exhaustively, ideally. E.g. when beets' config file option is not passed through, it can result in the child process using a different beets configuration than its parent process unexpectedly. Even with the option being passed through the configuration could change during execution of the ytimport command since the child process reloads it. To give another example: the log verbosity (-vvv) of the parent process is not applied to the child process oob. In this matter it is beneficial to be able to call a Python API from within the same Python process as opposed to having to call a CLI, spawning a new process.

After all why is beets' import_files method public when it shouldn't be called by 3rd party Python code? Now that it is public, why would you still want to force users to call the CLI and be fine with the CLI overwriting the --move option but not allow doing the same via the Python API (especially given the fact that this already works with all the other config options but the move option)?

Copy link
Member

Choose a reason for hiding this comment

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

True but like I said: ytimport is importing from a temporary directory that it manages itself.

In addition to this it also by default manages my library directory even when beets is configured not to do so.

The additional postprocessing and renaming of files after the import that you mentioned sounds more of a hacky workaround than a well designed/clean workflow to me.

It may have been hacky, but that's the beauty of beets with the flexibility it provides.

Yes, in fact it was like that initially before I changed it to using the Python API as opposed to the CLI. This was to avoid spawning a new Python process for the import. Apart from the unnecessary overhead, spawning a new Python process comes with the disadvantage that global beets CLI options are not applied to the beets child process oob and would each have to be passed through to the child process explicitly, covering all beets options exhaustively, ideally. E.g. when beets' config file option is not passed through, it can result in the child process using a different beets configuration than its parent process unexpectedly. Even with the option being passed through the configuration could change during execution of the ytimport command since the child process reloads it. To give another example: the log verbosity (-vvv) of the parent process is not applied to the child process oob. In this matter it is beneficial to be able to call a Python API from within the same Python process as opposed to having to call a CLI, spawning a new process.

After all why is beets' import_files method public when it shouldn't be called by 3rd party Python code? Now that it is public, why would you still want to force users to call the CLI and be fine with the CLI overwriting the --move option but not allow doing the same via the Python API (especially given the fact that this already works with all the other config options but the move option)?

Sorry, I should have clarified that I meant using the Python API, but instead of the import_files function, using the import Subcommand (I think it's found in beets.commands as import_cmd variable) and extending it with flags custom to ytimport. This would ensure consistent import behaviour.

I imagine you'd also want to adjust import_cmd.func which runs beets.commands::import_func to something like

def ytimport_func(lib, opts, args):
    download(...)
    # add the files to args, I guess
    ...
    import_func(...)

Copy link
Contributor Author

@mgoltzsche mgoltzsche Sep 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How is calling the import_func better than calling the import_files function that the former function delegates to? I think the approach you're suggesting is disadvantageous: It would require the ytimport plugin to write all options it needs to overwrite into the opts dict just to have the import_func copy it into the config as opposed to letting the ytimport plugin do that directly. The import_func function also doesn't treat these config changes as temporary but permanent ones, still requiring the ytimport plugin to restore the original configuration afterwards which is less obvious to maintain and more error-prone when it does not make the config changes directly but via the opts but then still has to access the config directly to restore its original values. (The latter is actually optional since usually no other code/plugin is run within the same Python process after the ytimport CLI command terminates but it leaves the door open for another 3rd party plugin to call the ytimport plugin via its Python API directly and run additional code afterwards within the same Python process based on the original user-specified configuration.)

Also, the problem this PR is solving still exists in the approach you're describing (the global config is modified after the fetchart plugin was initialized) and as I understand it also exists within Beets internally when you look at e.g. the import plugin overwriting the move option explicitly after the fetchart plugin was already initialized with the original/previous value of that option, resulting in the fetchart plugin not honouring the new value.

Copy link
Member

Choose a reason for hiding this comment

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

It would require the ytimport plugin to write all options it needs to overwrite into the opts dict just to have the import_func copy it into the config as opposed to letting the ytimport plugin do that directly.

Given that these options are only relevant for the import, and importing is delegated to beets, why should it be partly reimplemented in ytimport? I am assuming that ytimport does not overwrite any import-specific options but it has flags --import and --no-import to control whether the import_func is called.

The import_func function also doesn't treat these config changes as temporary but permanent ones, still requiring the ytimport plugin to restore the original configuration afterwards which is less obvious to maintain and more error-prone

The global config object is updated this way since the flags override what is specified in the configuration file. This way plugins that run during the import respect whatever is configured by the flags.

CLI command terminates but it leaves the door open for another 3rd party plugin to call the ytimport plugin via its Python API directly and run additional code afterwards within the same Python process based on the original user-specified configuration.

Let's consider the example I used above

def ytimport_func(lib, opts, args):
    download(...)
    # add the files to args, I guess
    ...
    import_func(...)

Assuming that the global configuration is adjusted by import_func only, the case you described can only happen if the 3rd party plugin extends ytimport command with some functionality that runs after the import is complete. Following my suggestion, we would have a command like

beet post-ytimport --help

...

Options:
  ...
  -m, --move ...
  ... the rest of beet import options
  ... beet ytimport options
  ... beet post-ytimport options

If it happens to depend on import configuration, I believe it is a good idea to keep the configuration that import_func overrides, which would happen if the user runs beet post-ytimport --move, for example.

Realistically, though, I think, it is more likely that a 3rd party plugin is going to be more interested in the functionality specific to your plugin: the download function in my example above (everything except the import).

Also, the problem this PR is solving still exists in the approach you're describing (the global config is modified after the fetchart plugin was initialized) and as I understand it also exists within Beets internally when you look at e.g. the import plugin overwriting the move option explicitly after the fetchart plugin was already initialized with the original/previous value of that option, resulting in the fetchart plugin not honouring the new value.

Have just checked this - you're completely right. Fetchart has essentially been using the default configuration all this time.

I'm going to approve this in a second, just going to quickly scan whether this issue has been reported before and add them to the PR.

Thanks for your patience!

try:
config["import"]["move"] = True
self._fetch_art(True)
self.assertNotExists(self.art_file)
finally:
config["import"]["move"] = prev_move

def test_do_not_delete_original_if_already_in_place(self):
artdest = os.path.join(os.path.dirname(self.i.path), b"cover.jpg")
Expand Down