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

Fix autobpm and improve its docs #5389

Merged
merged 9 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
poetry install --extras replaygain

- name: Install Python dependencies
run: poetry install --only=main,test
run: poetry install --only=main,test --extras=autobpm

- if: ${{ env.IS_MAIN_PYTHON != 'true' }}
name: Test without coverage
Expand Down
23 changes: 13 additions & 10 deletions beets/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
# The above copyright notice and this permission notice shall be
# included in all copies or substantial portions of the Software.

"""The core data store and collection logic for beets.
"""
"""The core data store and collection logic for beets."""

from __future__ import annotations

import os
Expand All @@ -24,6 +24,7 @@
import time
import unicodedata
from functools import cached_property
from pathlib import Path

from mediafile import MediaFile, UnreadableFileError

Expand Down Expand Up @@ -658,6 +659,11 @@ def relation_join(cls) -> str:
f"ON {cls._table}.album_id = {cls._relation._table}.id"
)

@property
def filepath(self) -> Path | None:
"""The path to the item's file as pathlib.Path."""
return Path(os.fsdecode(self.path)) if self.path else self.path

snejus marked this conversation as resolved.
Show resolved Hide resolved
@property
def _cached_album(self):
"""The Album object that this item belongs to, if any, or
Expand Down Expand Up @@ -1741,6 +1747,11 @@ class DefaultTemplateFunctions:

_prefix = "tmpl_"

@cached_classproperty
def _func_names(cls) -> list[str]:
"""Names of tmpl_* functions in this class."""
return [s for s in dir(cls) if s.startswith(cls._prefix)]

def __init__(self, item=None, lib=None):
"""Parametrize the functions.

Expand Down Expand Up @@ -2038,11 +2049,3 @@ def tmpl_ifdef(self, field, trueval="", falseval=""):
return trueval if trueval else self.item.formatted().get(field)
else:
return falseval


# Get the name of tmpl_* functions in the above class.
DefaultTemplateFunctions._func_names = [
s
for s in dir(DefaultTemplateFunctions)
if s.startswith(DefaultTemplateFunctions._prefix)
]
8 changes: 4 additions & 4 deletions beets/test/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,13 @@ class PluginMixin:
plugin: ClassVar[str]
preload_plugin: ClassVar[bool] = True

def setUp(self):
super().setUp()
def setup_beets(self):
super().setup_beets()
if self.preload_plugin:
self.load_plugins()

def tearDown(self):
super().tearDown()
def teardown_beets(self):
super().teardown_beets()
self.unload_plugins()

def load_plugins(self, *plugins: str) -> None:
Expand Down
74 changes: 32 additions & 42 deletions beetsplug/autobpm.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,81 +11,71 @@
# The above copyright notice and this permission notice shall be
# included in all copies or substantial portions of the Software.

"""Uses Librosa to calculate the `bpm` field.
"""
"""Uses Librosa to calculate the `bpm` field."""

from __future__ import annotations

from librosa import beat, load
from soundfile import LibsndfileError
from typing import Iterable

from beets import ui, util
import librosa

from beets.importer import ImportTask
from beets.library import Item, Library
from beets.plugins import BeetsPlugin
from beets.ui import Subcommand, should_write


class AutoBPMPlugin(BeetsPlugin):
def __init__(self):
def __init__(self) -> None:
super().__init__()
self.config.add(
{
"auto": True,
"overwrite": False,
"beat_track_kwargs": {},
}
)

if self.config["auto"].get(bool):
if self.config["auto"]:
self.import_stages = [self.imported]

def commands(self):
cmd = ui.Subcommand(
def commands(self) -> list[Subcommand]:
cmd = Subcommand(
"autobpm", help="detect and add bpm from audio using Librosa"
)
cmd.func = self.command
return [cmd]

def command(self, lib, opts, args):
self.calculate_bpm(lib.items(ui.decargs(args)), write=ui.should_write())
def command(self, lib: Library, _, args: list[str]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the unused arguments should be marked, but I think their names should be preserved, i.e. using _opts instead of just _. It's just a bit clearer, especially since the whole codebase isn't well-typed yet. Also, a type annotation should still be used for the argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree regarding general clarity, but do you reckon this is needed here given that this argument is unused? I find that _ in such case clearly communicates that. I find the presence of the type a bit confusing if the argument is not used.

Note that my opinion here is mostly based on refactoring interfaces like the one that BeetsPlugin provides: in this case using _ notation allows the interface (and the type) to change without the need to update the code here.

Essentially, I'm applying the ISP principle to the function arguments here

Within object-oriented design, interfaces provide layers of abstraction that simplify code and create a barrier preventing coupling to dependencies. A system may become so coupled at multiple levels that it is no longer possible to make a change in one place without necessitating many additional changes.[1] Using an interface or an abstract class can prevent this side effect.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, I'm just not used to it. Any preference, @Serene-Arc?

self.calculate_bpm(list(lib.items(args)), write=should_write())

def imported(self, session, task):
def imported(self, _, task: ImportTask) -> None:
self.calculate_bpm(task.imported_items())

def calculate_bpm(self, items, write=False):
overwrite = self.config["overwrite"].get(bool)

def calculate_bpm(self, items: list[Item], write: bool = False) -> None:
for item in items:
if item["bpm"]:
self._log.info(
"found bpm {0} for {1}",
item["bpm"],
util.displayable_path(item.path),
)
if not overwrite:
path = item.filepath
if bpm := item.bpm:
self._log.info("BPM for {} already exists: {}", path, bpm)
if not self.config["overwrite"]:
continue

try:
y, sr = load(util.syspath(item.path), res_type="kaiser_fast")
except LibsndfileError as exc:
self._log.error(
"LibsndfileError: failed to load {0} {1}",
util.displayable_path(item.path),
exc,
)
y, sr = librosa.load(item.filepath, res_type="kaiser_fast")
except Exception as exc:
self._log.error("Failed to load {}: {}", path, exc)
continue
except ValueError as exc:
self._log.error(
"ValueError: failed to load {0} {1}",
util.displayable_path(item.path),
exc,
)

kwargs = self.config["beat_track_kwargs"].flatten()
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, there's no security considerations here, right? cc @Serene-Arc

try:
tempo, _ = librosa.beat.beat_track(y=y, sr=sr, **kwargs)
except Exception as exc:
self._log.error("Failed to measure BPM for {}: {}", path, exc)
continue

tempo, _ = beat.beat_track(y=y, sr=sr)
bpm = round(tempo)
bpm = round(tempo[0] if isinstance(tempo, Iterable) else tempo)
item["bpm"] = bpm
self._log.info(
"added computed bpm {0} for {1}",
bpm,
util.displayable_path(item.path),
)
self._log.info("Computed BPM for {}: {}", path, bpm)

if write:
item.try_write()
Expand Down
17 changes: 16 additions & 1 deletion docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@ New features:
`beet list -a title:something` or `beet list artpath:cover`. Consequently
album queries involving `path` field have been sped up, like `beet list -a
path:/path/`.
* :doc:`plugins/autobpm`: Add new configuration option ``beat_track_kwargs``
which enables adjusting keyword arguments supplied to librosa's
``beat_track`` function call.

Bug fixes:

* Improved naming of temporary files by separating the random part with the file extension.
* Fixed the ``auto`` value for the :ref:`reflink` config option.
* Fixed lyrics plugin only getting part of the lyrics from ``Genius.com`` :bug:`4815`
* :doc:`plugins/autobpm`: Fix the ``TypeError`` where tempo was being returned
as a numpy array. Update ``librosa`` dependency constraint to prevent similar
issues in the future.
:bug:`5289`

For packagers:

Expand Down Expand Up @@ -50,6 +57,14 @@ Other changes:
checked.
* The long-deprecated `beets.util.confit` module has been removed. This may
cause extremely outdated external plugins to fail to load.
* :doc:`plugins/autobpm`: Add plugin dependencies to `pyproject.toml` under
the `autobpm` extra and update the plugin installation instructions in the
docs.
Since importing the bpm calculation functionality from ``librosa`` takes
around 4 seconds, update the plugin to only do so when it actually needs to
calculate the bpm. Previously this import was being done immediately, so
every ``beet`` invocation was being delayed by a couple of seconds.
:bug:`5185`

2.0.0 (May 30, 2024)
--------------------
Expand Down Expand Up @@ -350,7 +365,7 @@ Bug fixes:
:bug:`4947`
* Fix bug where unimported plugin would not ignore children directories of
ignored directories.
:bug:`5130`
:bug:`5130`
* Fix bug where some plugin commands hang indefinitely due to a missing
`requests` timeout.
* Fix cover art resizing logic to support multiple steps of resizing
Expand Down
20 changes: 18 additions & 2 deletions docs/plugins/autobpm.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,15 @@ of a track from its audio data and store it in the `bpm` field of your
database. It does so automatically when importing music or through
the ``beet autobpm [QUERY]`` command.

To use the ``autobpm`` plugin, enable it in your configuration (see
:ref:`using-plugins`).
Install
-------

To use the ``autobpm`` plugin, first enable it in your configuration (see
:ref:`using-plugins`). Then, install ``beets`` with ``autobpm`` extra

.. code-block:: bash

pip install "beets[autobpm]"

Configuration
-------------
Expand All @@ -21,5 +28,14 @@ configuration file. The available options are:
- **overwrite**: Calculate a BPM even for files that already have a
`bpm` value.
Default: ``no``.
- **beat_track_kwargs**: Any extra keyword arguments that you would like to
provide to librosa's `beat_track`_ function call, for example:

.. code-block:: yaml

autobpm:
beat_track_kwargs:
start_bpm: 160

.. _Librosa: https://github.com/librosa/librosa/
.. _beat_track: https://librosa.org/doc/latest/generated/librosa.beat.beat_track.html
Loading
Loading