Skip to content

Moving to uv instead of poetry. #2919

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

Merged
merged 15 commits into from
Jan 17, 2025
Merged

Moving to uv instead of poetry. #2919

merged 15 commits into from
Jan 17, 2025

Conversation

Narsil
Copy link
Collaborator

@Narsil Narsil commented Jan 16, 2025

More in the standard, faster, seemingly better lockfile.

What does this PR do?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

More in the standard, faster, seemingly better lockfile.
@angt
Copy link
Member

angt commented Jan 16, 2025

Nice 👍
Maybe my setup is broken (or it's expected?) but I had to manually install uv pip install grpcio-tools mypy-protobuf in my default environment to build.

@Narsil
Copy link
Collaborator Author

Narsil commented Jan 16, 2025

Nice 👍 Maybe my setup is broken (or it's expected?) but I had to manually install uv pip install grpcio-tools mypy-protobuf in my default environment to build.

This should be done automatically when you run make gen-server (or install-server or whatever).

Copy link
Member

@alvarobartt alvarobartt left a comment

Choose a reason for hiding this comment

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

hi here! thanks for this, left some comments here and there, but mainly suggestions, so feel free to discard!

@@ -44,10 +44,14 @@ jobs:
run: |
sudo apt update
sudo apt install python3.11-dev -y
pip install -U pip uv
Copy link
Member

Choose a reason for hiding this comment

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

here maybe we could also benefit from the Python installation with uv install python 3.11, as well as installing uv from the standalone installer instead of pip WDYT? reference at https://docs.astral.sh/uv/getting-started/installation/#standalone-installer

@@ -104,7 +104,7 @@ RUN case ${TARGETPLATFORM} in \
/opt/conda/bin/conda clean -ya

# Install flash-attention, torch dependencies
RUN python3 -m pip install --upgrade pip && pip install numpy einops ninja joblib msgpack cmake --no-cache-dir && rm -rf /var/lib/apt/lists/*
RUN python3 -m pip install --upgrade pip uv && pip install numpy einops ninja joblib msgpack cmake --no-cache-dir && rm -rf /var/lib/apt/lists/*
Copy link
Member

@alvarobartt alvarobartt Jan 16, 2025

Choose a reason for hiding this comment

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

either adding uv as a prefix to the follow-up pip install command, or just removing the uv installation for this stage, and install with pip instead.

Suggested change
RUN python3 -m pip install --upgrade pip uv && pip install numpy einops ninja joblib msgpack cmake --no-cache-dir && rm -rf /var/lib/apt/lists/*
RUN python3 -m pip install --upgrade pip uv && uv pip install numpy einops ninja joblib msgpack cmake --no-cache-dir && rm -rf /var/lib/apt/lists/*

p.s. besides the comment, shouldn't rm -rf /var/lib/apt/lists/* be placed after an apt-get somewhere for consistency, instead of after the pip install?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's probably bad copy-paste. I'm not touching anything non essential. This is a significant PR already

@Narsil Narsil merged commit de19e7e into main Jan 17, 2025
15 checks passed
@Narsil Narsil deleted the move_to_uv branch January 17, 2025 11:32
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.

4 participants