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

Use uv as python package manager instead of poetry in docker #32530

Closed
wants to merge 13 commits into from

Conversation

signed-long
Copy link
Contributor

@signed-long signed-long commented May 25, 2024

Description

Uses uv to install python dependencies instead of poetry if "uv" is passed into install_python_dependencies.sh.

Uv's requirements.txt files are platform specific so this is just for the docker build as of now. Also, uv doesn't have dependencies groups like poetry for dev dependencies, so this installs all deps.

I can add requirements.txt files to support mac builds and only non-dev deps too if the bounty #32522 gets locked to me, but with this PR the CI speed up may be worth merging. Seeing setup-with-retry with no cache hits taking 4min from 7min.

Verification

Builds and passes tests in CI.

Copy link
Contributor

github-actions bot commented May 25, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Lack of locking is a bit annoying, but the speedup is likely worth it. Let's do this:

  • remove requirements.docker.txt
  • just do uv pip install . in the Dockerfile (i.e. remove the install_python_dependencies.sh usage

Then, we can merge this, and I'll make a follow up to remove install_python_dependencies.sh. We really shouldn't be changing people's pip versions or anything weird like that with our weird setup script.

@adeebshihadeh
Copy link
Contributor

Here's my follow up to this: #32541

pyproject.toml Outdated Show resolved Hide resolved
@signed-long
Copy link
Contributor Author

The 7min time was from my memory playing with setup-retry last week, actual speed up building image with no cache in gha:

@adeebshihadeh
Copy link
Contributor

Unfortunately, the locking has already become an issue. Our weekly lockfile CI job just ran, and a couple different things broke (#32550). It would've been bad if those just randomly broke on master instead of that PR.

I'm closing it, but the bounty is yours since all the work was done.

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.

2 participants