[release auto] add uv and default python in forge #58874
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces uv for Python environment management in the forge Dockerfiles and standardizes curl command options. The changes are generally good, especially the move to curl -fsSL for more robust scripting.
I've left a few comments regarding minor inconsistencies and potential improvements:
- There's a mismatch between comments and code regarding the Python version being installed.
- An unnecessary
exportcommand can be removed for better clarity. - There is an inconsistent
pipversion pinned in one of the Dockerfiles.
Addressing these points will improve the maintainability and consistency of the Docker setup.
| curl -fsSL https://astral.sh/uv/install.sh | sudo env UV_UNMANAGED_INSTALL="/usr/local/bin" sh | ||
|
|
||
| mkdir -p /usr/local/python | ||
| # Install Python 3.9 using uv |
There was a problem hiding this comment.
The comment states that Python 3.9 is being installed, but the code below sets the version to 3.10. Please update the comment to reflect the correct Python version being installed.
# Install Python 3.10 using uv
| UV_PYTHON_VERSION=3.10 | ||
| uv python install --install-dir /usr/local/python "$UV_PYTHON_VERSION" | ||
|
|
||
| export UV_PYTHON_INSTALL_DIR=/usr/local/python |
There was a problem hiding this comment.
The export UV_PYTHON_INSTALL_DIR command appears to be unnecessary. This environment variable is not a standard uv variable and does not seem to be used by any subsequent commands in this RUN block. Removing it would improve clarity.
| curl -fsSL https://astral.sh/uv/install.sh | sudo env UV_UNMANAGED_INSTALL="/usr/local/bin" sh | ||
|
|
||
| mkdir -p /usr/local/python | ||
| # Install Python 3.9 using uv |
There was a problem hiding this comment.
The comment states that Python 3.9 is being installed, but the code below sets the version to 3.10. Please update the comment to reflect the correct Python version.
# Install Python 3.10 using uv
| UV_PYTHON_VERSION=3.10 | ||
| uv python install --install-dir /usr/local/python "$UV_PYTHON_VERSION" | ||
|
|
||
| export UV_PYTHON_INSTALL_DIR=/usr/local/python |
There was a problem hiding this comment.
The export UV_PYTHON_INSTALL_DIR command appears to be unnecessary. This environment variable is not a standard uv variable and does not seem to be used by any subsequent commands in this RUN block. Removing it would improve clarity.
ci/docker/forge.Dockerfile
Outdated
| curl -fsSL https://astral.sh/uv/install.sh | sudo env UV_UNMANAGED_INSTALL="/usr/local/bin" sh | ||
|
|
||
| mkdir -p /usr/local/python | ||
| # Install Python 3.9 using uv |
There was a problem hiding this comment.
The comment states that Python 3.9 is being installed, but the code below sets the version to 3.10. Please update the comment to reflect the correct Python version.
# Install Python 3.10 using uv
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
aslonnie
force-pushed
the
lonnie-251120-autoforge
branch
2 times, most recently
from
d38ddf7 to
a23be58
Compare
also uses `curl -fsSL` as much as possible, and removes unnecessary sudos Signed-off-by: Lonnie Liu <[email protected]>
aslonnie
force-pushed
the
lonnie-251120-autoforge
branch
from
a23be58 to
773396f
Compare
|
|
||
| # As a convention, we pin all python packages to a specific version. This | ||
| # is to to make sure we can control version upgrades through code changes. | ||
| uv pip install --system pip==25.2 cffi==1.16.0 |
There was a problem hiding this comment.
Bug: Inconsistent pip versions across forge Docker images
The new forge images install pip==25.2, while ci/docker/forge.Dockerfile uses pip==25.0. The comment explicitly states packages are pinned "to make sure we can control version upgrades through code changes", but this version mismatch across forge images creates inconsistent environments that could lead to different build behaviors between ARM64, x86_64, and the main forge image.
also uses `curl -fsSL` as much as possible otherwise the release blocker checker is not working. also removes unnecessary sudos. Signed-off-by: Lonnie Liu <[email protected]>
also uses `curl -fsSL` as much as possible otherwise the release blocker checker is not working. also removes unnecessary sudos. Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: YK <[email protected]>
also uses `curl -fsSL` as much as possible otherwise the release blocker checker is not working. also removes unnecessary sudos. Signed-off-by: Lonnie Liu <[email protected]>
also uses
curl -fsSLas much as possibleotherwise the release blocker checker is not working.
also removes unnecessary sudos.