-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: master
Are you sure you want to change the base?
Conversation
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.)
Reviewer's Guide by SourceryThe 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-loaderclassDiagram
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"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this 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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When I tried to use
pymusiclooper
for the first time, I ran into a confusing error message on playback:To debug, I ran
~/.local/pipx/venvs/pymusiclooper/bin/python
, and tried importing thesounddevice
library, and got this clearer error message: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:
lazy-loader
call with our own simple hand-written lazy-loader function forsounddevice
, 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:
Documentation: