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.

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

Early stage pipeline plugin functions #2814

Merged
merged 5 commits into from
Feb 22, 2018

Conversation

autrimpo
Copy link
Contributor

@autrimpo autrimpo commented Feb 18, 2018

This is a bug report together with a PR because I like to scratch my own itch :)
Something has been bugging me for quite some time after switching to Opus - some of the albums I've imported didn't have replaygain calculated, even though the replaygain plugin did definitely run. After lots of debugging the issue seems to be as follows:

  1. User tries to import a FLAC album
  2. Since the plugins don't run in a deterministic order (as far as I've noticed), ReplayGain is sometimes calculated on the FLAC files
  3. Files get converted to Opus
  4. There are ReplayGain values calculated from FLAC but Opus only takes R128
  5. The imported files need to have replaygain calculated again, manually

I've added a (quite ugly) hack to always convert the file first, which from my testing seems to reliably solve the issue. If you have better idea how to solve this (I've considered dividing the plugins into groups by 'priority' but that felt like an overkill), I'll happily implement it.

@sampsyo
Copy link
Member

sampsyo commented Feb 18, 2018

Got it! Thanks for explaining why it's nice for this to run first. I can see it being a good idea in general for convert to run before other operations.

It would be good to come up with a general way to prioritize import stages. We don't have to go crazy with anything too elaborate (such as a priority score), but how about just a mechanism for asking to be inserted at the beginning of the list? For example, we could add an early_import_stages alternative to import_stages that does this.

@autrimpo
Copy link
Contributor Author

So if I understand correctly, plugins would be either 'regular' or 'early' and 'early' ones would always run before 'regular' ones but other than that the order would be still non-deterministic? That's very similar to what I was trying with the priority groups, except I had three and this only has two. I'll have a go at it tomorrow.

@sampsyo
Copy link
Member

sampsyo commented Feb 18, 2018

Yeah, you got it exactly. I could also see three groups, but two might suffice—I don't quite see a use case at the moment for running as late as possible.

@autrimpo
Copy link
Contributor Author

It can always be added later if the need arises.

@autrimpo
Copy link
Contributor Author

autrimpo commented Feb 19, 2018

I've added functionality for early stage functions, decided it might be even better because plugins can now request to have part of their functions ran early and the rest at the normal time.

I think it might be worthwhile to add a test for this as well, have a plugin with 1 early and 1 normal function in the pipeline and check that they run in the correct order. I'll add one after we agree on the final implementation.

@autrimpo autrimpo changed the title Make 'convert' plugin run first in the pipeline Early stage pipeline plugin functions Feb 19, 2018
Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Looks perfect! To finish this off, could you please document the new API in the plugin docs (and add a quick changelog entry)?

beets/plugins.py Outdated
plugin: specifically, the logging level is adjusted to WARNING.
"""
return [self._set_log_level_and_params(logging.WARNING, import_stage)
for import_stage in self.early_import_stages]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the logging wrapper code (here and in the function below) can be factored out into a helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@autrimpo
Copy link
Contributor Author

Done, though I haven't written a test as I initially planned because unfortunately I don't know how.

@sampsyo sampsyo merged commit bbadb5f into beetbox:master Feb 22, 2018
sampsyo added a commit that referenced this pull request Feb 22, 2018
Early stage pipeline plugin functions
sampsyo added a commit that referenced this pull request Feb 22, 2018
sampsyo added a commit that referenced this pull request Feb 22, 2018
@sampsyo
Copy link
Member

sampsyo commented Feb 22, 2018

Awesome; thank you! ✨ 🐟 🌹 Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants