fix(trainer): ignore PEP 668 system python check#384
fix(trainer): ignore PEP 668 system python check#384google-oss-prow[bot] merged 1 commit intokubeflow:mainfrom
Conversation
|
🎉 Welcome to the Kubeflow SDK! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
There was a problem hiding this comment.
Pull request overview
This pull request adds PIP_BREAK_SYSTEM_PACKAGES=1 environment variable to pip install commands in the Kubeflow Trainer SDK. This fix enables Python package installation in Python 3.11+ externally managed environments (like Debian/Ubuntu system Python) where pip otherwise refuses to install packages without explicit permission.
Changes:
- Add
PIP_BREAK_SYSTEM_PACKAGES=1flag to pip install commands in Kubernetes backend - Update all test expectations in Kubernetes backend tests to reflect the new pip command format
- Add
PIP_BREAK_SYSTEM_PACKAGES=1flag to pip install commands in Container backend
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kubeflow/trainer/backends/kubernetes/utils.py | Updated two pip install command invocations to include PIP_BREAK_SYSTEM_PACKAGES=1 flag |
| kubeflow/trainer/backends/kubernetes/utils_test.py | Updated 10 test assertion lines across 5 test cases to expect the new pip command format |
| kubeflow/trainer/backends/container/utils.py | Updated pip install command builder function to include PIP_BREAK_SYSTEM_PACKAGES=1 flag |
| if PIP_DISABLE_PIP_VERSION_CHECK=1 PIP_BREAK_SYSTEM_PACKAGES=1 python -m pip install --quiet \\ | ||
| --no-warn-script-location $PIP_OPTS --user $PACKAGES >"$LOG_FILE" 2>&1; then | ||
| echo "Successfully installed Python packages: $PACKAGES" | ||
| elif PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\ | ||
| elif PIP_DISABLE_PIP_VERSION_CHECK=1 PIP_BREAK_SYSTEM_PACKAGES=1 python -m pip install --quiet \\ |
There was a problem hiding this comment.
The flag should not be added to the localprocess backend. It is not safe to install packages into the system python outside containerised environments, and it's not necessary as this backend uses a venv.
| rm -f "$LOG_FILE" | ||
|
|
||
| if PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\ | ||
| if PIP_DISABLE_PIP_VERSION_CHECK=1 PIP_BREAK_SYSTEM_PACKAGES=1 python -m pip install --quiet \\ |
There was a problem hiding this comment.
I am wondering if PIP_BREAK_SYSTEM_PACKAGES can break other user's deployment.
WDYT @kubeflow/kubeflow-sdk-team ?
There was a problem hiding this comment.
It should be a safe default given it's only set in a container. It just means that pip doesn't complain if it tries to modify an externally-managed python. It doesn't change how pip installs the packages.
Signed-off-by: Rob Bell <robell@redhat.com>
robert-bell
force-pushed
the
fix-pep-668
branch
from
00eadbe to
4936a33
Compare
|
/ok-to-test |
astefanutti
left a comment
astefanutti
left a comment
There was a problem hiding this comment.
Thanks @robert-bell!
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
The
pytorch/pytorch:2.10.0-cuda12.8-cudnn9-runtimeimage brought in by kubeflow/trainer#3320 has PEP 668 enabled which causespip installto fail when creating custom train jobs, causing the trainjob to fail.Generally, it sounds like python images shouldn't be enabling PEP 668, but I think it's a good idea for us to explicitly disable the check whenever we're creating jobs in a container.
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Checklist: