Create Python Package #365
Conversation
mariugul
force-pushed
the
feat/create-python-package
branch
from
b892fa3
to
d35f51b
Compare
|
I feel that the readme and and setup/launch scripts should still be in the project root to preserve backwards compatibility. |
The readme I agree. I think it was a mistake, it's supposed to be root. Usually in GitHub I like to put in in Edit: The readme was already in root I think? I moved out the scripts. I put them in the folder that was already called scripts. Does it look good? |
mariugul
force-pushed
the
feat/create-python-package
branch
from
e5673e4
to
5209693
Compare
mariugul
force-pushed
the
feat/create-python-package
branch
from
5209693
to
da9ae04
Compare
We could also move parts of the README regarding the scripts into the |
- Used Poetry to build a package. Can still install with pip.
mariugul
force-pushed
the
feat/create-python-package
branch
from
da9ae04
to
889517f
Compare
- Move troubleshooting section to its own README. - Move legacy installation method with scripts to its own README in scripts folder. - Update README with new information on python package and cleanup.
|
@vicwomg I pushed an example of cleaning up the READMEs a bit. See what you think. I still think the main readme is a bit messy. I think it should be easy, clean, straight forward with not too much images and feature information. You will have releases on github with all the information anyway |
README in docs is probably fine, as long as github will still render it as a home page. My concern with moving the scripts is that many users have already installed via git and assumed paths for these scripts (auto launch scripts, desktop shortcut configurations, etc). As I have already mentioned (and as you will find from many discussions) many pi users are linux newcomers and thrown off very easily by small changes. |
docs/README.md
Outdated
| |---------------------------------------|-----------------------------------------------------------------------------------------------| | ||
| | Raspberry Pi 3 and above | Suitable, but anything below Pi 3 will likely be too slow. | | ||
| | Raspberry Pi Desktop OS (headed) | Should be used if running with a display, as it requires a browser. | | ||
| | 32-bit Bullseye OS (recommended for Pi 3) | Recommended OS for Pi 3. | | ||
| | 64-bit Bookworm OS (recommended for Pi 4 and above) | Fine for Pi 4 and above. | | ||
| | Raspberry Pi 3 High-Res Video Playback | Pi 3 might struggle with high-res video playback, but overclocking can help. | | ||
| | Disable Screen Blanking | Disable "screen blanking" in `raspi-config` to prevent the display from turning off when idle.| | ||
| | Modern Mac, PCs, and Linux | Works fine on modern devices. | |
There was a problem hiding this comment.
I'm not sure this column designation makes sense. The first 6 items are notes about raspberry pi.
There was a problem hiding this comment.
It was just an attempt at making a table instead of just having text. It's all up to you
docs/README.md
Outdated
| # Setup a virtual environment (optional) (recommended) | ||
| python -m venv venv | ||
| source venv/bin/activate |
There was a problem hiding this comment.
What happens if no venv is created?
There was a problem hiding this comment.
Then it's just installed directly in your PC environment. It should work perfectly fine, it's just easier to deal with a venv when deleting, upgrading, downgrading, changing versions etc. It doesn't clutter your PC installation. Especially important if you use the same dependencies in different python packages. That can lead to conflicts.
|
Overall this looks great. It would be very nice indeed to make pikaraoke installable via pip. However, I think I'll need to pull this all down and do some thorough testing in various platforms to confirm everything is working. It will probably cause quite a bit of initial confusion for existing pi users, but might be worth it to get releases running smoothly moving forward. |
| # Install pikaraoke from PyPi | ||
| pip install pikaraoke | ||
| ``` |
There was a problem hiding this comment.
I don't know if this is complete. Installation of ffmpeg is necessary. So is chromium and chromium-driver for raspberry pi users. Hence the install scripts.
There was a problem hiding this comment.
Sure yeah. It's possible to embed those scripts in the package. For example something like have pikaraoke install the script in path as pikaraoke-setup:
pip install pikaraoke
# Setup pikaraoke
pikaraoke-setup
# Run pikaraoke
pikaraoke
That's a good concern to have. Make it clear in the readme how to run it like before. You have this tagged and released already as 1.2 if I remember correctly. The only thing they need to understand is really to clone tag |
That's a good idea. I will also test it on a pi, just haven't gotten to that yet. I know that the logo and qr code needs to be embedded as resources for example. I haven't done that yet in this PR. It was done in the original, so I will just port it. I can polish this PR a bit and test it and I will make it ready for review. |
Modifies the files minimally with correct imports as well as a few minor fixes to make it run again.
|
@vicwomg I pushed some updates to the package. I can now run pikaraoke on my Ubuntu. Do this: # from root repo (inside a venv)
pip install -e .Or with poetry: # from root repo
poetry install
poetry run pikaraokeI have some issues with playing songs. I have a few already downloaded, those work. If I search a song, it's downloaded but doesn't play. It just goes to "song ended". Did not test on RPI, only my Ubuntu. |
|
I think it might make sense to just make this PR work inside the repo. Don't bother with PyPi just yet to simplify. That would mean you still need to clone the repo, but can install it as a package. Then we can do another PR where all the resources are packed into the package and it's stored on PyPi. |
|
@vicwomg is there anything you need from me to make this progress further? |
Your last message indicates that download/playback don't work, so obviously that is not mergeable. Did you want me to have a look at that? Sorry I haven't had much time to work on it |
No worries. I just tested on rpi. Same issue. I think you are quicker to diagnose the issue. It will not download and play. |
|
I loosened the versions for ffmpeg and yt-dlp and it worked. Will test some more but I think that was it. |
|
@vicwomg Made |
Run pikaraoke-setup after installing package.
mariugul
force-pushed
the
feat/create-python-package
branch
from
ae3a8ee
to
2d0e973
Compare
|
I spent some time trying to figure out publishing, and I think I have a release here (it's a bit behind your latest changes): I had to change the target to python 3.9 (was 3.10) for it to work on the raspberry pi default installation. It does seem to install on my mac and on my pi 3 which is promising |
That sounds promising, good job! Do you have a specific lowest python version you wish to support? I know |
If you want to do some crazy tests and not screw up anything you can publish to https://test.pypi.org/. I can setup an automated workflow for the publishing in another PR. Then simply comitting |
|
I tried on my rapsberry pi 4 to install the package you published. python -m venv venv
source venv/bin/activate
pip install pikaraoke
pikaraoke --headless |
|
I added a PR to your branch with the Python 3.9 requirement and to clean up some outdated documentation. If approved, you can merge into this PR. I thought long about this, but I removed the steps to set up venv as I think they will confuse most beginner pi users and replaced them with a suggestion that they use venv if they feel its necessary and link to the official python docs on how to set one up. We'll see how many package conflicts arise, but I imagine not many. I tried update the README to make the legacy scripts work by adding ./scripts/ path to them in the documentation, but they ultimately fail on the restructured imports. May just have to not recommend that approach for now. Next test is to try a pip install on a Windows machine. If that goes well then I think the release plan is to push the package to PyPi as pikaraoke 1.3.0 (since it has breaking changes from 1.2). Then once it is confirmed there, merge the PR. |
More CI would be amazing! But we probably only want CI to upload to pypi when an official release is drafted, not on every master checkin. |
The setup script only works on debian-based distros, so might make sense to rename it to |
It sounds like a good plan. I will take a look at your PR. I can also test on windows, I have dual boot. In time we can set up these tests in CI also. I think that suggesting a There are several strategies for packaging. I agree that |
This comes down to packaging strategy! SemanticIn semantic versioning you would increment every feature as Release CandidatesAnother strategy is to do release candidates. Python does Beta/AlphaThe other two can be combined with alpha and betas. For example For such a small app it might be smart to do a simple strategy like basic semantic versioning. My question would be, assuming you will increment pikaraoke with semantic versioning, why would you wait to publish a package? If tests are ran on a PR and on merge, then |
I can do that if you wish! My idea was that |
If there's a naming that makes more sense feel free to go with it. I just don't want mac and windows users running this script |
That makes sense. I think |
I was referring to the abstracted command: In fact, the setup really only installs ffmpeg and chrome. We could also just communicate that in the README (in fact, I did add that in my PR to your PR) |
Can call it |
|
Promising news, I managed to get the pip package installed on my windows box. Works as expected. I did have to fight with the fact that python 3.8 was installed already and thus not compatible. I used the windows installer to put 3.12 on there, but pip refused to recognize the upgraded version, which caused a lot of headaches. Ultimately, I had to: delete all paths to the old 3.8 version of pip from the environment. Uninstall/Reinstall 3.12 and be sure to check the "add environment variables" setting in the installer (a custom option buried under some screens). I don't know how anyone on windows manages to use python without going crazy. |
Great! I have used python on windows and I did go crazy 🤣 Awesome to hear that it worked! 📦 🎉 |
I could be in the minority, but I just tend to think of releases as a collection of bugfixes, and a version bump is tied to a release, not a checkin. Master serves as a bleeding edge sandbox, but until it's fully tested (and I consider manual QA a part of that process) a release would not be considered. If it is simpler to semantic bump on every merge, we can try it that way and see if it becomes unwieldy. You're right that master commits are relatively rare now. My hope is more maintainers (I hope you'd be interested in that role!) would join the project and it becomes more common. |
There are different strategies and approaches. I haven't seen this one so much. What I suggested would be a Continuous Deployment (CD) with Continuous Integration (CI) strategy which is an agile concept. I'm sure you are familiar. However, you raise a point with what master is supposed to be. That would be the branching strategy. GitFlow is a popular one with I don't think your strategy is bad, I just think it's not the best for a small app like this and not so agile. I would vouch for good tests (don't have it yet) that integrate code so when it reaches master it's always seen as deployable. As you say, we could always try and see how it goes. I could probably help out with maintaining. |
|
Inching forward on this. Pushed the latest to pypi, but noticing a couple of things on the pi.
|
Great! Yes installing directly with pip can give these warnings. I think that installing a python package inside a venv is python101 and not crazy to expect. With some quick trying I think that pip install pipenv
pipenv install pikaraoke
pipenv run pikaraokeCould also use pipx like this, but I think it's more difficult to install pipx itself correctly. python3 -m pip install --user pipx
python3 -m pipx ensurepath
pipx install pikaraoke
pikaraokeI think that poetry is not great for direct installation from PyPi. A simple Honestly it's like 3 or 4 commands that people can copy from a readme to the terminal. It should be ok no matter which method you recommend. |
Create Python Package
This introduces versioning of the project as a python package.
Pros:
This PR is still draft, I haven't tested it yet. The idea is to just move files and change them as little as possible and make the project run like before but as a python package. @vicwomg you should make an account on https://pypi.org/. See how to package here packaging-projects.
I can walk you through how to build the repo with poetry and that gives you
.whland.tarfiles. These can be uploaded quite easily to PyPi, but it should be your account. Then we can integrate it into CI so it gets automatically updated on features1.X.0and fixes1.0.Xetc.