Pikaraoke Improvements

[mariugul]

Improve PiKaraoke

This PR contains a lot of changes. It's probably too large to merge but it displays a refactored and still working application and what it would look like. I would expect to make smaller PR's from this should the changes be accepted. Look at this more like a discussion and proof of concept PR.

This PR does not really introduce any new functionality to the app itself. It's a big refactor making things more generic and simpler (I think). The motivation was that PiKaraoke was broken on Windows and quite difficult to get to run for someone who is not a developer. Even if it wasn't broken, it's still more difficult to setup than it needs to be. This should be a good step in the right direction to get the app more generic, documented, future proof and easier to expand on.

I suggest checking out the branch and viewing the repo in an editor. I also suggest reviewing one commit at a time instead of going through all the files changed tab.

The reason this PR grew so big is mostly that there was a lot of dependencies that was difficult to isolate without refactoring larger parts of the app. It was also a good way for me to understand how the app works to refactor it.

pikaraoke	Windows	Mac	Ubuntu	RPI
Tested On	✅	❌	✅	❌

To Do

Divide this PR into smaller PR's:

• Code Quality PR
• Python Package PR
• CI DevOps PR
• Path refactor PR
• Logging PR

Get Started

Check out this branch:

git checkout refactor/make-code-more-generic

With pip

Install pikaraoke as a python package:

# Use a virtual environment
python -m venv venv
source venv/bin/activate

# From root repo
pip install . 

# Run pikaraoke
pikaraoke

With poetry

This is even simpler because poetry manages a venv automatically.

# From root repo
poetry install

# Run pikaraoke
poetry run pikaraoke

Python Package

I created a python package of pikaraoke using pyproject.toml with poetry. It's not required to use poetry, it can still be installed with pip.

Instead of having to clone the repo, deal with resources locally, having to start a script from the repo, dealing with .bat and .sh scripts, installing in a venv from requirements.txt and so on, it makes more sense to just have a python package of PiKaraoke that automatically installs into $PATH the app.py script with the alias pikaraoke.

1. The app would be versioned.
2. Easier to install.
3. Dependencies and resources are handled inside the package.
4. Can be hosted on PyPi.
5. More cross-platform friendly

yt-dlp

There was logic to find out the path of yt-dlp binary and also send in as a parameter yt-dlp binary path. I replaced this with just calling yt-dlp in subprocess. This is because yt-dlp is already a dependency installed by pip. So pip already puts it directly in $PATH and therefore there's no reason to figure out where the binary is.

I also looked into calling it directly from python, because it's a python dependency after all. The way it's written doesn't make it simple to do this, though I got it to work. So I left it as a subprocess call to the in-PATH yt-dlp command.

I don't see a reason to specify where it's installed as an input argument. Correct me if I'm wrong. I also don't see a reason to update yt-dlp in the app. This is a dependency of pikaraoke and should be tested when it's launched. If yt-dlp is updated that should be another release of pikaraoke, in my opinion.

Paths

This was one of the most important refactors. There was extensive use of os.path which is not a great way to do paths in python, at least not anymore. I switched out most of os.path for pathlib. The reason is that instead of hardcoding paths for windows in the windows style C:\\User\\ etc. and likewise for Linux using strings /host/user/etc, pathlib already solves this issue being a cross-platform path library. It also comes with many useful methods for creating directories, files, deleting them, checking if they exist, writing, reading++.

Switching out everything for pathlib made everything easier, concise, modern, cross-platform-friendly and expandable.

Qr Code

The qr code gets saved to the tmp directory for each run of pikaraoke and gets deleted after the app stops.

Logo

The logo is included as a resource in the python package and it's fetched from the package when needed.

Logs

More Logging

Adding logger = getLogger(__name__) at the top of a file gives it a logger to use.

Configuration

I moved configuring the logging to just after the parsing of input arguments. This means the whole app can be logged.

The log config formats a easy to read log for the console and a more detailed log for the logfiles. It also gets all the loggers and formats them in the same way. Also the ones from cherrypy.

Store Logs on Host

The logging configuration now logs to a logfile with a timestamp. The path can be anywhere really. Preferably in a pikaraoke app folder somewhere. I put it in .config/pikaraoke/logs and AppData/Local/pikaraoke/Logs for Windows.

Rotate Logs

Keeps the newest 5 logfiles. Files can only grow to 10MB and will be overwritten after this on rotation. The number and size can be configured.

Code Quality

Introduced linting and formatting of the code. Used the most common tools from pre-commit and ran it on all the files. This is black formatter, isort import formatter, pylint, yaml, markdown formatters, unused imports ++.
This should run in the CI ideally (I can't run a CI on a fork).

End to End Tests

Started on e2e tests using playwright. I could have used selenium too. It seems playwright is more modern and simpler to use and write tests with. That's the reason I chose it. It integrates with pytest, so it's a simple pytest run to run the tests.

I implemented a simple sanity test that the logo exists and is correct. Should expand on these to test the QR code, search, browse, play a song, rename a song and so on. This should run in CI on macOS, ubuntu and windows for good testing.

Separate Routes in Files

app.py was a large file with all the routes. I moved them to separate files under routes/ for more order and a cleaner app file.

Remove Global Variables

Moving the routes means there is no access to app and karaoke for example. Instead of using global variables I utilized Flask's way of registering objects and using the current_app object so we can access the karaoke object and other things in the routes no matter where they are defined.

Config File

I moved the defaults and constants from the top of app.py into a config.py. This is Flask's way of dealing with these kinds of settings. Then they are all available in the routes on current_app.config. Can move more config here later. For example tmp folder perhaps.

Parse Args in Separate File

Parsing of the input arguments for the cli has been moved out of app.py to parse_args.py. This makes it cleaner and easier to work with.

Intellisense on Args

Added a namespace to the args. This gives them intellisense. The type of some of the arguments wasn't necessarily so easy to understand. Some of them were ambiguous. I fixed the types and added this for clarity.

Docstrings and Type Hints

I added a lot of docstrings and type hints to methods. I don't think I did it everywhere. This just makes it easier to understand what's happening and to contribute to the code.

Open Host Default Web Browser

I noticed that Selenium was used to open pikaraoke. It was tied hard to the chrome browser. I realize this might have been to circumvent clicking the "confirm" button when opening pikaraoke. However, it's very limiting to force the users to use Google Chrome. This makes a lot of the cross-platform work trying to be compatible with macOS, Linux, Rpi, Windows in vain. I changed it to using the webrowser lib builtin to python to open the default browser on the host system. After all, a browser is already cross platform, right? Correct me if I'm wrong on something here and selenium does more than click the confirm button.

At least, if using selenium, it should support all the browsers like Firefox, Chrome, Edge, Safari ++. However, the app is hosted as a website, so why bother with all that work?

Platform Enum

Made a platform enum that holds OSX, RASPBERRY_PI, LINUX, WINDOWS and UNKNOWN so that the platform can be compared to an enum. Expanded on this and implemented methods on the enum like is_rpi() and is_windows()... so an if statement can simply be:

platform = get_platform()

if platform.is_rpi():
    pass

if platform.is_mac():
    pass

Known Issues

I know the rename of a song doesn't work right now. The issue is simple to fix. It's a small path + str issue.

[vicwomg]

wowww, I love what I'm reading here. Appreciate the focus on reorganization, standards, clean up as well as e2e testing. This is all stuff I think about constantly every time I need to update this project because it's a pain in the ass and most of the codebase was established when I didn't know wtf I was doing. But yes this is a pretty enormous changelist that might take some time to parse.

Regarding the selenium stuff, yes it only clicks the confirm button to bypass the manual need to do this. But I don't think it's safe to default to os default because, well, only Chrome works right now. Firefox and Safari are really particular about streaming mp4 in video tags. If users want to experiment with other browsers, I'd suggest they run with the --headless option and do it manually.

[vicwomg]

I've learned quite a few new things about the Python ecosystem just reading your description. A few thoughts and I'm certainly open to discussion:

The name of the project is pikaraoke, and originally it was developed to run on raspberry pi. This continues to be focus of development as the most common use is a standalone karaoke appliance that is always attached to the TV. So while cross platform considerations are great, we always have to be mindful of how it affects the pi.

On that note: pi is an interesting platform with lots of limitations that have led to many of the decisions made here. There are hardware limitations, obviously. But the more complex (to me) is that there are also consumer limitations. Many people who use them are dipping their toes for the first time into linux and really need a lot of help with seemingly simple things like using git. So when I start to look at things like adding more env managers like poetry (not opposed, it should just be optional, as you say), I immediately think about the extra layers of command line savvy installation friction that will lose a lot of those folks. Hence these sort of one-off setup scripts. The goal was trying to get users going as quickly as possible without having to install a bunch of support packages manually.

Things like being able to update yt-dlp manually from the ui are similarly implemented with that in mind. Youtube encryption keys change frequently and it breaks the app. Most users do NOT know how to do a git pull, much less poke around PyPi for new versions. So yes, in an ideal world updating the app dependency is the more traditional solution, but the reality is it needs to be made simple somehow and user-serviceable without command line to be adopted by more folks.

Personally, I am willing to slog through the review, provided we can get the conflicts addressed.

[mariugul]

I've learned quite a few new things about the Python ecosystem just reading your description. A few thoughts and I'm certainly open to discussion:
...
Personally, I am willing to slog through the review, provided we can get the conflicts addressed.

Great to hear that you are positive about it! 🎉 The python package versioning will make updating the project so much easier. It can be combined with automatic package incrementers in CI, so if you introduce something like Conventional Commits for the commit messages, a new release, tag and package will be built automatically on PR merges where the commits has feat, fix etc.

I agree with this being pikaraoke and that should be the focus. Regarding selenium I have ran on Microsoft Edge (on Ubuntu) and that works great. I guess it's because it's chromium so I guess Chrome, Chromium and Edge works. Could always say to users that pikaraoke supports chromium browsers but still let it choose the default of the OS. Just my thoughts.

I don't have access to setting up a CI for this project, so I didn't do any work on that. I work with devops daily, with pi's as self hosted runners as well, so I could introduce a CI if I have access to it. Ideally, you'd have a pi4 and a pi5 (maybe a pi3 too?) as self hosted runners where you can run the end to end tests. That would really qualify the code. Even without that, just running e2e tests, unit tests, code quality etc. on a ubuntu cloud would help a lot.

I didn't know yt-dlp broke so often. In that case leave it to be updated from the app. That's not an issue. I agree that lots of complex tools is not great for the regular user. I have another idea, which I started on but didn't introduce because the PR is so big already. That idea builds on this PR and was to use pyinstaller to create one binary of the whole application. What that means is that you pack pikaraoke as an application, with dependencies, resources and a python interpreter inside a single binary or folder and you distribute that. Then the user doesn't need to know about git or python even. They just need to get the binary and run it. Introducing a python package already removed the git dependency, so this next step would remove the python dependency.

I think that if you agree with the contents of the PR I can scope out parts of it to smaller PR's that are digestable. I think that makes a lot of sense. For example a "Python Package PR", "Code Quality PR", "CI PR", "Paths PR" and so on. What do you think?

[vicwomg]

Breaking it up and working off the current master would be MUCH appreciated. I'd code freeze and prioritize getting it all merged in. I think all this makes the project much more accessible and contributable.

[mariugul]

@vicwomg what makes more sense next? Setup a CI for basic install of pikaraoke and running my simple playwright end to end tests? Pikaraoke is starting to look real professional now.

[vicwomg]

@vicwomg what makes more sense next? Setup a CI for basic install of pikaraoke and running my simple playwright end to end tests? Pikaraoke is starting to look real professional now.

Let's do some bugfixes and enhancements to see how well the workflow functions. Getting all that running sounds like some heavy lifting and configuration. I could use a break from devops for a while 🙃

[mariugul]

@vicwomg what makes more sense next? Setup a CI for basic install of pikaraoke and running my simple playwright end to end tests? Pikaraoke is starting to look real professional now.

Let's do some bugfixes and enhancements to see how well the workflow functions. Getting all that running sounds like some heavy lifting and configuration. I could use a break from devops for a while 🙃

Alright! I'll wait with the end to end playwright stuff. That's a difficult one. I can look into the path refactor and the logging.