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

pip -> uv #22

Merged
merged 24 commits into from
Aug 30, 2024
Merged

pip -> uv #22

merged 24 commits into from
Aug 30, 2024

Conversation

dakinggg
Copy link
Contributor

@dakinggg dakinggg commented Aug 30, 2024

Replace pip with uv. This reduces the time every action takes by 3-4 minutes because uv is just that much faster at installing things. Tested this PR against LLM Foundry (last commit here) and an internal repo to ensure it works. I do not fully understand the specific incantations that were needed to get everything to install correctly...but it does seem to work.

@dakinggg dakinggg marked this pull request as ready for review August 30, 2024 06:47
@dakinggg dakinggg requested review from b-chu and mvpatel2000 August 30, 2024 06:47
@eitanturok
Copy link

Two comments:

  1. uv also supports virtual environments: https://docs.astral.sh/uv/pip/environments/. This should be faster than using python -m venv ../.venv
  2. You currently install uv with pip install uv. But you can install uv without needing python as a dependency, curl -LsSf https://astral.sh/uv/install.sh | sh.

Copy link

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

uv requires venv, so these changes seem sensible

a bit confused why we need site packages but that seems minor

approving to unblock but would be good to resolve that small q

.github/actions/code-quality/action.yaml Show resolved Hide resolved
.github/mcli/mcli_pytest.py Show resolved Hide resolved
@dakinggg
Copy link
Contributor Author

@eitanturok both your statements are true, I was just trying to do the simplest change that would have benefit because making CI changes is difficult and annoying.

Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

ok we ball

Copy link
Collaborator

@b-chu b-chu left a comment

Choose a reason for hiding this comment

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

Thanks!

@dakinggg dakinggg merged commit d3b44d1 into main Aug 30, 2024
@mvpatel2000 mvpatel2000 deleted the uv branch August 30, 2024 18:57
@b-chu b-chu restored the uv branch August 30, 2024 22:28
@b-chu b-chu deleted the uv branch August 30, 2024 22:28
b-chu added a commit that referenced this pull request Aug 30, 2024
This reverts commit d3b44d1.
@b-chu b-chu mentioned this pull request Aug 30, 2024
b-chu added a commit that referenced this pull request Aug 30, 2024
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.

5 participants