-
Notifications
You must be signed in to change notification settings - Fork 67
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
pyenv to uv based python and venv #303
Changes from 65 commits
b415c16
63b8458
c60e49f
5e5b8ef
d178ab8
a3de1a2
f7e55c0
56067dd
fd5f9db
2232fe4
1bd9278
7bafbe4
8bd41f1
db09a10
58ad598
d434034
04d7a05
4ec3a41
c5f49df
6571d23
d91713b
8a36dae
5b4a0c9
f609d15
920962a
5b80d73
ed2e2f1
85ef729
87dd173
71f46b5
7763a31
f40e27d
6b22035
8cdd398
753f242
11ed4cb
f75ba91
ff5ae89
10ea20a
10e8f81
d7db0cf
3fbda24
2e1f21d
1837383
af505ef
e0d1b14
a4480ca
08b886f
9e313ba
ee5679b
d453fe0
537630b
9c46cf7
2efdb90
2b8a3f2
1aed813
d99c3fd
1b133a7
ece511e
e47a5e5
41e6dd6
69f9cd1
d61f3cd
3af0750
e7328c1
406e943
23f97a8
53e3723
02386fc
0ae90b9
6a9c466
b6d1fd0
62aa5a6
6468a9b
db232fd
fcc6da6
b0b5f1e
b3ee073
9905cbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,14 @@ | ||
#!/bin/bash -e | ||
|
||
echo "Installing python for openpilot" | ||
echo "installing uv..." | ||
|
||
# Install pyenv | ||
export PYENV_ROOT="/usr/local/pyenv" | ||
curl https://pyenv.run | bash | ||
export PATH="$PYENV_ROOT/bin:$PATH" | ||
eval "$(pyenv init -)" | ||
export XDG_DATA_HOME="/usr/local" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this for? I suspect this has implications for other things There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adeebshihadeh The XDG_DATA_HOME only sets the uv venv location (usr/local) otherwise the default location would fall to current directory (.venv). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it doesn't affect anything except providing the installation part for the venv. But we should avoid usage it /etc/profile as this will make it Into the envvars and might have some influence. |
||
export CARGO_HOME="$XDG_DATA_HOME/.cargo" | ||
|
||
PYTHON_VERSION="3.11.4" | ||
if [ "$(uname -p)" == "aarch64" ]; then | ||
pyenv install --verbose $PYTHON_VERSION | ||
else | ||
MAKEFLAGS="-j1" MAKE_OPTS="-j1" taskset --cpu-list 0 pyenv install --verbose $PYTHON_VERSION | ||
fi | ||
curl -LsSf https://astral.sh/uv/install.sh | sh | ||
eval ". $CARGO_HOME/env" | ||
|
||
echo "Setting global python version" | ||
pyenv global $PYTHON_VERSION | ||
PYTHON_VERSION="3.12.3" | ||
|
||
pip3 install --no-cache-dir --upgrade pip uv | ||
# uv requires virtual env either managed or system before installing dependencies | ||
uv venv $XDG_DATA_HOME/venv --seed --python-preference only-managed --python=$PYTHON_VERSION |
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.
any reason not to do this every time now that it's parallel?
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.
Considering the todo item PyQt5: either need a script to build/pull the wheel or build/install in the Docker build again,
Even with parallel builds, seems building wheel every time would slow down the build. I have put build flag rebuildpyqt5=0 when set to 0 uses predefined wheel when set to 1 builds the wheel. I suppose other options could include to stay with building the wheel every time but then using prebuilt wheel becomes redundant.
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.
How did you build the wheel? rebuildpyqt5 does not actually rebuild it as in saving an updated whl file, but just chooses between your file and building from scratch.
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.
@robin-reckmann rebuildpyqt5 would either select the prebuilt wheel or build again from source and then save the file for use. Perhaps the better alternative as suggested by @adeebshihadeh is to rebuild wheel every time but doing so makes the prebuilt wheel redundant so perhaps remove the prebuilt wheel and rebuild wheel every time.
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.
Yes, ideally we would just use the whl file and have a completely separated script to build it on demand. My only issue with your solution is, that the whl file is not actually saved outside of the docker build context. And the ARG is not expose to the build script.
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.
@adeebshihadeh If the .whl file were available on the PyPI server, we probably wouldn't have an issue using it, or?(Similar to not re-building all the deb files). Given that we don't even modify it (for now) and it saves a considerable amount of build time, I would again propose to completely separate out the whl compile and use that to update if necessary.
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.
Let's build it every time. I already explained my rationale in the other PR