PICARD-2526: Allow starting processing actions from the command line

[skelly37]

Summary

• This is a…
  • [ ] Bug fix
  • [x] Feature addition
  • [ ] Refactoring
  • [ ] Minor / simple change (like a typo)
  • [ ] Other
• Describe this change in 1-2 sentences:

Users will be able to pass some commands to the existing instance with -e/--exec flag.

Problem

It would be useful to tell the existing Picard instance what to do, e.g. QUIT, useful in combination with the other single-instance mode improvements

• JIRA ticket (optional): PICARD-2526

Solution

• add command:// prefix to the commands
• send them as any other arg
• parse with urlparse, split by ";"
• execute

Action

[skelly37]

Hm

• The core works
• SHOW works
• QUIT gets stuck as when the pipe was introduced, then removed (but force quit will not be a reasonable solution here)
• print leftovers removed
• the cli flag got refactored

Time to implement other commands and think about the QUIT in the meantime, I guess...

(Sorry for the bad responsivity btw, I'll finish moving on Saturday/Sunday)

[zas]

@skelly37 https://github.com/skelly37/picard/pull/5 adds a bit more logging

[skelly37]

Update:

• QUIT got fixed and has a proper flow
• 5/9 commands are already implemented (QUIT + SHOW + all the cluster pane ones)
• Only album pane ones left to do
• After that we can discuss ideas/design of the functions that actually take the arguments suggested by @zas

@phw, @zas What's wrong there? I don't understand...

[zas]

@phw, @zas What's wrong there? I don't understand...

This was a temporary server issue (it was returning http 500) when trying to upload SARIF results from CodeQL action. This is configured by this workflow.

I re-ran the failed job, and it worked.

[zas]

@skelly37 your last commit is titled "consider it ready", it wouldn't help one looking at commit history.

In general, I think you should be more careful about your commit messages.Things like "almost done" or "typo fix v2" don't help anyone to understand what's the commit is about. Also, please use correct English (capitalization, punctuation, verbs).

In this case, since that was a draft pull request for a while, I think an interactive rebase / history cleanup is needed before merging.

This was also the case with your others PRs. Nothing catastrophic, but we usually prefer clean explicit commit messages (and sometimes we get lazy too, so our commit messages aren't always fantastic...).

Please just take care about that.

[skelly37]

@skelly37 your last commit is titled "consider it ready", it wouldn't help one looking at commit history.

In general, I think you should be more careful about your commit messages.Things like "almost done" or "typo fix v2" don't help anyone to understand what's the commit is about. Also, please use correct English (capitalization, punctuation, verbs).

In this case, since that was a draft pull request for a while, I think an interactive rebase / history cleanup is needed before merging.

This was also the case with your others PRs. Nothing catastrophic, but we usually prefer clean explicit commit messages (and sometimes we get lazy too, so our commit messages aren't always fantastic...).

Please just take care about that.

Sorry about that, I get your point.

Though, I'm personally used to the squash on merge flow, where commits serve mostly only the in-PR purposes. And actually I was going to mention this issue, too. Why doesn't Picard use squash on merge? The CI/CD works just fine so bisect is rather not a case and I can't think of any other reason not to squash.

[zas]

Why doesn't Picard use squash on merge?

Usually we want to preserve dev history, it is better to have smaller commits, especially when it comes to use bisect to find when an issue was introduced. Of course, it all depends on the dev process, but small logical units often make sense. I use to squash commits, doing interactive rebase before the PR is merged, so, at the end, a simple PR can be one commit, but for bigger PRs squashing all may be too much, and having more steps is usually preferred.

Commits that are squashed are typically:

• commits cancelling each other (like added/removed code)
• commits fixing a previous commit (like typos)
• commits fixing a previous commit that broke tests

Each commit is supposed to test successfully, that's not always easy for big feature introduction, but often that's more about code organization: introducing a bunch of methods without their calls (so those are no-op) until everything is in place, then add a commit enabling the added code.

There's nothing written in stone in this matter, but the general rule is:

• small "logic" commits
• clear commit messages
• commit doesn't break tests
• independent changes go in different commits

[zas]

For example, https://github.com/metabrainz/picard/pull/2137/commits/e012e84d22d23dd6a093426d25500ed80f910959 should be 2 small commits:

• one introducing _init_remote_commands()
• one changing the loop in get_album_pane_tracks()

Why? Because those are independent changes.

[skelly37]

@zas Okay, understood, thanks for explaining your convention to me.

My suggestion is then to split it into such commits commits:

1. ParseItemsToLoad got prepared to parse commands // all the changes that happened there
2. Method to get all the tracks from album pane for Tagger // get_album_pane_tracks
3. Command handling introduced to Tagger // L389-L441
4. Commands are actually passed via command-line // parser.add_argument and needed changes in the main function

Are you okay with that, assuming that none of you will request changes not related to the git?

[skelly37]

ping @phw

[skelly37]

Okay, all the ideas implemented, the PR is ready imo.

I'd just like to ask anyone to manually test the lookup_cd command. I have no music CDs here so I cannot actually test if I've rewritten it correctly.

[phw]

Sorry for the long silence, I had been totally occupied by life here. Things look good to me. I can theoretically test the CD lookup feature, but right now don't have my CD drive handy. I'll see to do this later, but would be happy with merging this before.

Wanted to comment on the squash merge question a bit. zas has said it basically all already, and I fully agree here. When merging a single commit I usually go with the rebase and merge approach, so we get a simpler history. Also if it's really a simple change with some of those fixup commits I might choose a squash and merge.

But I'm not really a fan of squash and merge as a rule for every PR. IMHO many more complex changes warrant to be separated into multiple logical commits, that just makes the change easier to understand. Also having an explicit merge commit then makes it clear these changes formed a PR.

But what's right is that we don't always are very strict and consistent with this, and haven't really formalized the rules here. We had some rather wild commit histories merged, we sometimes have single commits with separate merge commits.

[skelly37]

Looks okay to me. I notice that the available commands are not displayed in any help, so we'll need to make sure they are documented well in the on-line documentation.

Something we might consider is to add a -e --help or --help -e or --e-help or some other option to display the available commands.

EDIT: Because -e always requires a command as an argument, perhaps the easiest way would be to have help as one of the commands so if the user enters -e help the available commands for the -e command line argument would be displayed.

-e help seems okay to me. Should be done by the end of the day.

[rdswift]

Nah, you don't need the quotes, unless the path contains spaces (which is an expected behavior imo)

I agree that is the expected behavior.

Having said that, do we have a problem if we try to pass both a command AND a file to load? For example, picard file/to/load is a valid command line as is picard -e lookup file/to/lookup. What happens if the two are combined as picard -e lookup file/to/lookup file/to/load? I suspect that the file/to/load argument will also get passed on to the -e lookup command along with file/to/lookup, and I believe that will be a problem.

[skelly37]

What happens if the two are combined as picard -e lookup file/to/lookup file/to/load? I suspect that the file/to/load argument will also get passed on to the -e lookup command along with file/to/lookup, and I believe that will be a problem.

Depends.

• If there is an existing instance, picard -e will send all the positional arguments in the given order. Then the commands. So yeah, you are right, all files will be looked up alongside with the file/to/load in your scenario
• If there is no running instance, nothing will happen, as picard -e blocks Picards startup

[zas]

@skelly37 nice to have help for remote commands, have a look at https://github.com/skelly37/picard/pull/7

[rdswift]

• If there is no running instance, nothing will happen, as picard -e blocks Picards startup

In this case, would it make sense to start an instance and process the commands rather than (silently) do nothing?

[skelly37]

• If there is no running instance, nothing will happen, as picard -e blocks Picards startup

In this case, would it make sense to start an instance and process the commands rather than (silently) do nothing?

Imo it makes sense to execute stuff only on an existing instance; execute a command in a running Picard instance. If others insist I might change it but I'd prefer not to. Take for example: picard -e QUIT. Would it make sense to start Picard only to quit it?

The current logic is as intuitive as possible for me.

[rdswift]

• If there is an existing instance, picard -e will send all the positional arguments in the given order. Then the commands. So yeah, you are right, all files will be looked up alongside with the file/to/load in your scenario

This still bothers me a little bit, because the command line behavior of adding a file via the command line differs depending on whether or not there is a -e command specified before the path of the file to add. I wonder if it will work properly if the file to add is specified before ANY -e command options. I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

[zas]

I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

Yes, that's a bit confusing. I wonder if we should add a LOAD url/file command for convenience so one can only use -e commands (optionally).

picard -e LOAD filepath1 filepath2 filepath3 -e CLUSTER -e LOOKUP

[skelly37]

• If there is an existing instance, picard -e will send all the positional arguments in the given order. Then the commands. So yeah, you are right, all files will be looked up alongside with the file/to/load in your scenario

This still bothers me a little bit, because the command line behavior of adding a file via the command line differs depending on whether or not there is a -e command specified before the path of the file to add. I wonder if it will work properly if the file to add is specified before ANY -e command options. I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

Any -e argument prevents Picard from starting, regardless of its type and the order of other arguments, see: https://github.com/metabrainz/picard/pull/2137/files#diff-754164b29034aa27e27d501630b16fe491c78108500b10caeab6b272bc240fbdR1328

This explains it well, imo:

If there is no running instance, picard -e blocks Picards startup and does nothing.

[skelly37]

I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

Yes, that's a bit confusing. I wonder if we should add a LOAD url/file command for convenience so one can only use -e commands (optionally).

picard -e LOAD filepath1 filepath2 filepath3 -e CLUSTER -e LOOKUP

So you're saying that we should ditch the positional argument FILE_OR_URL in favor of adding a LOAD command which would be the only way to load something to picard?

[zas]

I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

Yes, that's a bit confusing. I wonder if we should add a LOAD url/file command for convenience so one can only use -e commands (optionally). picard -e LOAD filepath1 filepath2 filepath3 -e CLUSTER -e LOOKUP

So you're saying that we should ditch the positional argument FILE_OR_URL in favor of adding a LOAD command which would be the only way to load something to picard?

Nope, in addition. Basically picard <FILE> would be equivalent to picard -e LOAD <FILE>. The idea is to let user use only -e commands to achieve the same thing.

[skelly37]

I think it might, in which case the documentation should probably note that any -e command options should only appear at the end of the command line.

Yes, that's a bit confusing. I wonder if we should add a LOAD url/file command for convenience so one can only use -e commands (optionally). picard -e LOAD filepath1 filepath2 filepath3 -e CLUSTER -e LOOKUP

So you're saying that we should ditch the positional argument FILE_OR_URL in favor of adding a LOAD command which would be the only way to load something to picard?

Nope, in addition. Basically picard <FILE> would be equivalent to picard -e LOAD <FILE>. The idea is to let user use only -e commands to achieve the same thing.

In that case we have to either redesign the argstring handling or allow only one file per LOAD (like in REMOVE), because paths can contain spaces

[rdswift]

In that case we have to either redesign the argstring handling or allow only one file per LOAD (like in REMOVE), because paths can contain spaces

At the risk of introducing some scope creep, I think it might be cleanest overall to update the arg handling as it is passed to the pipe in the line:

                pipe_handler = pipe.Pipe(app_name=PICARD_APP_NAME, app_version=PICARD_FANCY_VERSION_STR, args=to_be_added)

Currently to_be_added is an iterable of strings. If it was changed to an iterable of tuples of (command, command_args) where command is a string and command_args is a list of strings, this would simplify argument processing because argparse would take care of quoted arguments for you automatically. This could also make use of the LOAD command proposed by @zas because the contents of the argparse FILE_OR_URL could automatically be converted to a LOAD command (ultimately handled with a single processing path). It would also remove the single file argument restriction for the REMOVE command.

To reuse most of your existing pipe and command processing code, commands with multiple arguments could be sent to the pipe as separate command/argument message strings.

[rdswift]

Take for example: picard -e QUIT. Would it make sense to start Picard only to quit it?

Actually, I believe that it would make sense because it would allow the user to process a command stack such as:

picard -e load my/ripped/album/directory -e cluster -e lookup -e save_complete -e fingerprint -e submit_fingerprints -e remove_saved -e quit

This would all be handled in a single running instance rather than first having to create an instance for processing and then create another instance to pass the command stack to the first instance. We might even consider adding a DUMP_LOG command to dump the log entries to a file specified as an argument to the command, like -e dump_log my/log/for/this/album.txt which could be included in the command stack just prior to -e quit.

Don't get me wrong, I think what you've done with this so far is great. I just think that with a little tweaking it could be so much more versatile. Having said that, perhaps these additional tweaks should be on a separate PR after this one has been merged.

[rdswift]

One final (?) item... I see that we have a SAVE_COMPLETE command to save fully matched releases, but nothing for saving partially matched releases which are quite common when the user chooses to SCAN rather than LOOKUP. Perhaps we should also have a SAVE_ALL command to accommodate that type of workflow.

[rdswift]

Sorry, but I just thought of another potential command (enhancement), that being the ability to read the commands from a text file. Perhaps the command could be called BATCH or INCLUDE. For example, if the text file contained:

load "/my/ripped/files/directory/selected album"
cluster
lookup
save_complete
fingerprint
submit_fingerprints
remove_saved
dump_log /var/log/picard_selected_album.log
quit

then the command stack in the text file could be executed by calling picard -e batch "/my/command/batch/file". You could use shlex.split() to parse each non-empty line from the text file and then add them to the queue in the same way as if they had been entered on the command line.

This would allow the users to build potentially lengthy command stacks (either by hand or dynamically through their ripping workflow) and defer Picard processing until later using a batching process.

[skelly37]

Sorry, but I just thought of another potential command (enhancement), that being the ability to read the commands from a text file. Perhaps the command could be called BATCH or INCLUDE. For example, if the text file contained:

load "/my/ripped/files/directory/selected album"
cluster
lookup
save_complete
fingerprint
submit_fingerprints
remove_saved
dump_log /var/log/picard_selected_album.log
quit

then the command stack in the text file could be executed by calling picard -e batch "/my/command/batch/file". You could use shlex.split() to parse each non-empty line from the text file and then add them to the queue in the same way as if they had been entered on the command line.

This would allow the users to build potentially lengthy command stacks (either by hand or dynamically through their ripping workflow) and defer Picard processing until later using a batching process.

That would be really cool, thanks.

One final (?) item... I see that we have a SAVE_COMPLETE command to save fully matched releases, but nothing for saving partially matched releases which are quite common when the user chooses to SCAN rather than LOOKUP. Perhaps we should also have a SAVE_ALL command to accommodate that type of workflow.

It saves all changed files, if I understood the enums correctly.

Currently to_be_added is an iterable of strings. If it was changed to an iterable of tuples of (command, command_args) where command is a string and command_args is a list of strings, this would simplify argument processing because argparse would take care of quoted arguments for you automatically. This could also make use of the LOAD command proposed by @zas because the contents of the argparse FILE_OR_URL could automatically be converted to a LOAD command (ultimately handled with a single processing path). It would also remove the single file argument restriction for the REMOVE command.

To reuse most of your existing pipe and command processing code, commands with multiple arguments could be sent to the pipe as separate command/argument message strings.

So you're saying that picard -e REMOVE file1 file2 should be split into:

• picard -e REMOVE file1
• picard -e REMOVE file2 ? This might be indeed a good solution.

Take for example: picard -e QUIT. Would it make sense to start Picard only to quit it?

Actually, I believe that it would make sense because it would allow the user to process a command stack such as:

picard -e load my/ripped/album/directory -e cluster -e lookup -e save_complete -e fingerprint -e submit_fingerprints -e remove_saved -e quit

This would all be handled in a single running instance rather than first having to create an instance for processing and then create another instance to pass the command stack to the first instance. We might even consider adding a DUMP_LOG command to dump the log entries to a file specified as an argument to the command, like -e dump_log my/log/for/this/album.txt which could be included in the command stack just prior to -e quit.

Yeah, now I tend to agree with you, picard -e shouldn't prevent Picard from starting.

Don't get me wrong, I think what you've done with this so far is great. I just think that with a little tweaking it could be so much more versatile. Having said that, perhaps these additional tweaks should be on a separate PR after this one has been merged.

Thanks :) Though I'm not sure how much should we add in this PR. Imo we should add the reading from file and load in this PR, but patch the multi-argument commands and picard -e blocking startup in the second one. How about that?

[rdswift]

One final (?) item... I see that we have a SAVE_COMPLETE command to save fully matched releases, but nothing for saving partially matched releases which are quite common when the user chooses to SCAN rather than LOOKUP. Perhaps we should also have a SAVE_ALL command to accommodate that type of workflow.

It saves all changed files, if I understood the enums correctly.

Okay, I just took a look at the code and I see that it actually saves all matched files as you said, rather than just the releases that are marked as "complete" (indicating that all tracks on the album/release have one [and only one?] file attached). That being the case, I suggest that the current SAVE_COMPLETE command be renamed to SAVE_MATCHED or SAVE_ALL to avoid confusion, and perhaps consider adding a SAVE_COMPLETE command that only saves files where the release is marked as "complete".

So you're saying that picard -e REMOVE file1 file2 should be split into:

• picard -e REMOVE file1
• picard -e REMOVE file2 ? This might be indeed a good solution.

Not exactly (unless I misunderstood your comment). What I meant was that the user could still enter it as picard -e REMOVE file1 file2 and when you process it you send it to the pipe as two separate commands as if it had been entered as picard -e REMOVE file1 -e REMOVE file2. To accomplish this I suggest replacing line 1364 which is currently:

                to_be_added.append("command://" + " ".join(e))

with something like (WARNING: Untested Code):

                if len(e) > 1:
                    for _arg in e[1:]:
                        to_be_added.append("command://{0} {1}".format(e[0], _arg))
                else:
                    to_be_added.append("command://" + e[0])

Don't get me wrong, I think what you've done with this so far is great. I just think that with a little tweaking it could be so much more versatile. Having said that, perhaps these additional tweaks should be on a separate PR after this one has been merged.

Thanks :) Though I'm not sure how much should we add in this PR. Imo we should add the reading from file and load in this PR, but patch the multi-argument commands and picard -e blocking startup in the second one. How about that?

That works for me, but I'll defer to @zas and @phw for guidance. I'd still l ike to see the LOG_DUMP (and possibly LOG_CLEAR) command added at some point as well.