fetchart: defer file removal config option evaluation

[mgoltzsche]

Description

Defer the evaluation of the source file removal options (import.delete and import.move) to the point where the fetchart plugin is actually called instead of only evaluating those configuration options on plugin initialization.
This is to allow other plugins (such as the ytimport plugin) to invoke the import directly (within the same python process; implicitly invoking the fetchart plugin) with temporarily overwritten configuration options.

Addresses #5167 (comment)

To Do

• Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[Serene-Arc]

Whoops, wrong button there. Accidentally approved it.

There are failing tests. The code seems fine at first glance, but it's affected the tests.

[mgoltzsche]

I adjusted the test correspondingly now. Please re-review.

[snejus]

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):
            ...

[mgoltzsche]

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.

[snejus]

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.

[mgoltzsche]

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?

[snejus]

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?

[mgoltzsche]

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)?

[snejus]

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(...)

[mgoltzsche]

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.

[snejus]

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!