Skip to content

RHOAIENG-21690: chore(Dockerfiles): augument micropipenv with uv for faster package installs #968

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RHOAIENG-21690: chore(Dockerfiles): augument micropipenv with uv for faster package installs #968

wants to merge 1 commit into from

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Mar 19, 2025 •
edited
Loading

https://issues.redhat.com/browse/RHOAIENG-21690

This change depends upon

Description

In our Dockerfile builds, micropipenv is used to read Pipfile.lock and run pip install to install everything in there.

it works by first creating a temporary requirement.txt with hashes, and then it runs pip install -r to install from that generated file

Collecting scikit-learn==1.5.2 (from -r /tmp/requirements_micropipenv-rti6r8e5.txt (line 2))
Downloading scikit_learn-1.5.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (13.3 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 13.3/13.3 MB 172.2 MB/s eta 0:00:00
Installing collected packages: scikit-learn
Successfully installed scikit-learn-1.5.2

In enhancement

we added generating requirements.txt with hashes and committing them into our repository directly. That means we now can install from the requirements.txt without needing micropipenv. Turns out that uv is very fast requirements.txt installer that can be a drop-in replacement https://astral.sh/blog/uv

How Has This Been Tested?

This change still keeps micropipenv around, it uses it to generate requirements.txt, and then it uses uv to install the requirements.

Checking the install logs, it still prints the desirable warning

warning: The package `tf2onnx` requires `protobuf~=3.20`, but `4.25.6` is installed

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from caponetto and dibryant March 19, 2025 13:21
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 19, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign atheo89 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the size/l label Mar 19, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
This was referenced Mar 21, 2025
Merged
Merged
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
@jiridanek jiridanek changed the title try to install with uv instead of micropipenv RHOAIENG-21690: chore(Dockerfiles): augument micropipenv with uv for faster package installs Mar 21, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 21, 2025
@jiridanek
Copy link
Member Author

/retest jupyter-minimal-ubi9-python-3-11-on-pull-request

@opendatahub-io opendatahub-io deleted a comment from openshift-ci bot Mar 24, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 24, 2025
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 24, 2025
@jiridanek
RHOAIENG-21690: chore(Dockerfiles): augument micropipenv with uv for …
c025d67 
…faster package installs

```
  × No solution found when resolving dependencies:
  ╰─▶ Because there is no version of certifi==2025.1.31 and you require
      certifi==2025.1.31, we can conclude that your requirements are
      unsatisfiable.

      hint: `certifi` was found on https://download.pytorch.org/whl/cu121, but
      not at the requested version (certifi==2025.1.31). A compatible version
      may be available on a subsequent index (e.g., https://pypi.org/simple).
      By default, uv will only consider versions that are published on the
      first index that contains a given package, to avoid dependency confusion
      attacks. If all indexes are equally trusted, use `--index-strategy
      unsafe-best-match` to consider all versions from all indexes, regardless
      of the order in which they were defined.
```

When installing as `USER 1001` in /opt/app-root

```
Using Python 3.11.9 environment at: /opt/app-root
error: Permission denied (os error 13) at path "/opt/app-root/.tmpQEqqyQ"
```

believe we should _not_ enable
https://docs.astral.sh/uv/reference/settings/#compile-bytecode
astral-sh/uv#6493
```
UV_COMPILE_BYTECODE=1
--compile-bytecode
```

uv has bug installing tensorflow
astral-sh/uv#12092
@openshift-ci openshift-ci bot added size/l and removed size/l labels Mar 26, 2025
@jstourac
Copy link
Member

otherwise lgtm

@jstourac
Copy link
Member

/lgtm

the failed CI looks like an ugly infra issue...

@openshift-ci openshift-ci bot added the lgtm label Mar 26, 2025
@jiridanek
Copy link
Member Author

Thanks, I'll think what to do about this. Hopefully somebody else likes this too.

Next step could be to leave out micropipenv completely and use the requirements.txt we already have generated in the git repo for every image.

harshad16
harshad16 reviewed Mar 26, 2025
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

Amazing initative
As this is bring in core change to workbench image

holding for sometime to get time to review this.

/hold

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2025

@jiridanek: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/rocm-runtimes-ubi9-e2e-tests c025d67 link true /test rocm-runtimes-ubi9-e2e-tests
ci/prow/rocm-notebooks-e2e-tests c025d67 link true /test rocm-notebooks-e2e-tests
ci/prow/images c025d67 link true /test images

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jiridanek
Copy link
Member Author

the failed CI looks like an ugly infra issue...

upon rerun, the openshift CI shows real issues,

@jiridanek
Copy link
Member Author

I've discussed with Jan and I've talked myself out of wanting to do this.

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstourac jstourac jstourac left review comments

@harshad16 harshad16 harshad16 left review comments

@caponetto caponetto Awaiting requested review from caponetto

@dibryant dibryant Awaiting requested review from dibryant

Assignees

@jstourac jstourac

Labels
do-not-merge/hold lgtm needs-rebase size/l
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jiridanek @jstourac @openshift-merge-robot @harshad16