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

PyInstaller #288

Closed
wants to merge 5 commits into from
Closed

PyInstaller #288

wants to merge 5 commits into from

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Jun 15, 2023

Adds PyInstaller to the dependencies and a poe task for building the binary. This allows us to distribute a binary without needing users to install python and pipx first, which is particularly useful for devs not using Python in their development stack but still want to use algokit.

I still need to do some more testing, but so far things seem to be working on Mac (ARM) and Ubuntu.

pyproject.toml Outdated Show resolved Hide resolved
@@ -48,3 +48,6 @@ jobs:

- name: Check docs are up to date
run: poetry run poe docs && [ $(git status --porcelain docs/ | wc -l) -eq "0" ]

- name: Check PyInstaller compilation
run: poetry run poe build
Copy link
Contributor

@robdmoore robdmoore Jun 15, 2023

Choose a reason for hiding this comment

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

We probably want this in the python-build workflow rather than the python-check workflow. And then probably should add it as an artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to have it here as an initial smoke test but yeah I agree, before we merge we should make an action for automatically building it and including zips in the release.

@joe-p joe-p marked this pull request as ready for review October 15, 2023 23:34
@joe-p
Copy link
Contributor Author

joe-p commented Oct 15, 2023

I'd like to bring this into consideration again. Main thing missing that should be here before merge is integrating into workflows.

Also, I did some benchmarking for the PyInstaller folder build vs Nuitka vs pipx install. They all seem to be pretty close with eachother so we don't loose much going from pipx > pyinstaller. PyInstaller is marginally faster than Nuitka, but since PyInstaller is more popular probably makes sense to stick with it even if we find there are other commands that Nuitka outperform it.

Note that this is for the directory distribution. It can be compiled to a single binary, but performance is much worse. This is totally fine for brew/choco since they can just put the files in PATH and call it a day. I think eventually we'd want an msi installer for windows for those that don't have choco.

❯ hyperfine --warmup 3 -i "algokit doctor" "./pyinstaller-dist/algokit/algokit doctor" "nuitka-dist/algokit.bin doctor"
Benchmark 1: algokit doctor
  Time (mean ± σ):      1.220 s ±  0.060 s    [User: 0.645 s, System: 0.211 s]
  Range (min … max):    1.160 s …  1.353 s    10 runs
 
Benchmark 2: ./pyinstaller-dist/algokit/algokit doctor
  Time (mean ± σ):      1.251 s ±  0.032 s    [User: 0.696 s, System: 0.198 s]
  Range (min … max):    1.211 s …  1.299 s    10 runs
 
Benchmark 3: nuitka-dist/algokit.bin doctor
  Time (mean ± σ):      1.268 s ±  0.033 s    [User: 0.726 s, System: 0.186 s]
  Range (min … max):    1.232 s …  1.339 s    10 runs
 
Summary
  algokit doctor ran
    1.02 ± 0.06 times faster than ./pyinstaller-dist/algokit/algokit doctor
    1.04 ± 0.06 times faster than nuitka-dist/algokit.bin doctor

@aorumbayev
Copy link
Collaborator

@joe-p thanks again for all the work on this PR so far! Just fyi, i moved your latest and merged latest from main into #382. @negar-abbasi will be spiking an ADR to asses pyinstaller and few other alternatives - there are still some limitations with pyinstaller+gh actions when it comes to apple silicon builds and linux distros with glib dependency - will be assessing how to address those as part of ADR. As for this PR we might revisit and close it once we get to implementation (cc @neilcampbell )

@aorumbayev aorumbayev closed this Jan 15, 2024
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.

3 participants