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

Improve experience when system does not have PortAudio installed yet #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matchu
Copy link

@matchu matchu commented Nov 29, 2024

When I tried to use pymusiclooper for the first time, I ran into a confusing error message on playback:

ERROR    module 'sounddevice' has no attribute 'OutputStream'

To debug, I ran ~/.local/pipx/venvs/pymusiclooper/bin/python, and tried importing the sounddevice library, and got this clearer error message:

>>> import sounddevice
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/matchu/.local/pipx/venvs/pymusiclooper/lib/python3.10/site-packages/sounddevice.py", line 71, in <module>
    raise OSError('PortAudio library not found')
OSError: PortAudio library not found

Researching a bit, it looks like the lazy-loader library imports modules in a slightly complex way, which happens to not raise import side-effects like this one.

So, in this PR, I've made two changes:

  1. Add instructions to the README to help Linux users understand that they may need to install PortAudio, and how to do it.
  2. Replace the lazy-loader call with our own simple hand-written lazy-loader function for sounddevice, which will raise the more helpful error message if the user is missing PortAudio.

Thank you for this tool! I hope this is helpful.

Summary by Sourcery

Improve user experience by providing clearer error messages when PortAudio is not installed and update documentation to guide Linux users on installing PortAudio.

Enhancements:

  • Replace the 'lazy-loader' call with a custom lazy-loader function for 'sounddevice' to provide clearer error messages when dependencies like PortAudio are missing.

Documentation:

  • Add instructions to the README for Linux users on how to install the PortAudio library if needed.

For the sounddevice library to work on Linux systems, the PortAudio
library must be present on the system. In this change, we add a brief
explanation of this, and instructions for how to install it on the most
common Linux package manager, `apt`.

https://python-sounddevice.readthedocs.io/en/0.5.1/installation.html
When `sounddevice` is missing its PortAudio library dependency, it will
raise an error on import: `OSError: PortAudio library not found`.

However, the `lazy-loader` library imports the module in a way that
skips this import side-effect. I don't fully understand the details,
but I did observe that, if PortAudio is missing, running `pymusiclooper
play` outputs: `ERROR    module 'sounddevice' has no attribute
'OutputStream'`, suggesting it returns an empty-ish module instead of
raising an error.

In this change, we roll our own simple lazy-loader function, using
Python's built-in `importlib.import_module` function. This will raise
the more helpful error message if the PortAudio library is missing,
while still giving us the advantage of not importing the library until
we need actually need it.

I tested this change by:
1. Remove PortAudio from the system.
2. Run `pymusiclooper -i play --path <path>`.
3. See the analysis work correctly, but upon playback, `ERROR    module
   'sounddevice' has no attribute 'OutputStream'`. (This confirms both
   that we get the error, and that the module was lazy-loaded instead
   of eagerly loaded.)
4. Install PortAudio onto the system.
5. Run `pymusiclooper -i play --path <path>`.
6. See the analysis work correctly, then playback work correctly. (This
   confirms that the module loads correctly when PortAudio is present.)
Copy link

sourcery-ai bot commented Nov 29, 2024

Reviewer's Guide by Sourcery

The PR improves the user experience when PortAudio is not installed by implementing a custom lazy-loading mechanism for the sounddevice module and adding installation instructions. The changes replace the existing lazy-loader library with a hand-written lazy-loading function that provides clearer error messages when PortAudio dependencies are missing.

Class diagram for PlaybackHandler with custom lazy-loader

classDiagram
    class PlaybackHandler {
        - stream
        + __init__()
        + start_playback()
        + stop_playback()
    }
    class sd {
        + CallbackStop()
        + OutputStream()
    }
    class importlib {
        + import_module()
    }
    PlaybackHandler --> sd : uses
    PlaybackHandler --> importlib : uses
    note for sd "Custom lazy-loader function replaces lazy-loader library"
Loading

File-Level Changes

Change Details Files
Implement custom lazy-loading mechanism for sounddevice module
  • Remove lazy-loader library dependency for sounddevice
  • Add custom lazy-loading function using importlib
  • Update sounddevice module access to use the new lazy-loading function
  • Modify stream creation and callback to use the lazy-loaded module
pymusiclooper/playback.py
Add PortAudio installation instructions for Linux users
  • Add note about PortAudio requirement for the play command on Linux
  • Include Ubuntu-specific installation command for libportaudio2
README.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @matchu - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

1 participant