Skip to content

Conversation

@joerunde
Copy link
Collaborator

@joerunde joerunde commented Mar 28, 2025

Fixes #61

This integrates our project with uv, so that we can cleanly reject torch upgrades while installing all dependencies. This also modernizes our pyproject.toml to contain the source of truth for our dependencies, and sorts them into groups.

The updates to the Dockerfile demonstrate this working, though I think the dockerfile may not be around for much longer.

This blast radius ended up a bit bigger than I originally intended, but...

  • The old version of yapf relied on lib2to3 which no longer exists in newer pythons, and has to be installed separately on the system. This broke the simpler uv setup flow on github actions, so It was easier to just upgrade. This caused some changes in formatting in the test files
  • I wanted to remove the requirements files since the requirements are now all properly specified in the pyproject.toml file. The old setup.py file pretty much only existed to parse those, so it was removed as well. This required a couple mode edits to pyproject.toml to specify setuptools as the build backend and set the extra entrypoint up

We can now either use uv directly to install the plugin and manage our local dev environments, or have uv compile the dependencies back into requirements files to emulate the old pip-only workflow. I've updated the pr comment action to provide instructions for both options of installing the linting dependencies.

@github-actions
Copy link

👋 Hi! Thank you for contributing to vLLM support on Spyre.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, first install the linting requirements, then run format.sh and commit the changes:

pip install -r requirements-lint.txt
bash format.sh

Now you are good to go 🚀

]

[tool.uv.sources]
vllm = { git = "https://github.com/vllm-project/vllm", rev = "v0.8.0" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Specifying the source for vllm allows uv to run the install from source, specifying the empty target so that the gpu deps don't get installed

dependencies = [
"fms-model-optimizer>=0.2.0",
"ibm-fms==0.0.8",
"vllm",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can specify vllm and fms-model-optimizer, and don't have to worry about either of them overriding torch 👍

ln -sf $(which pip${PYTHON_VERSION}) /usr/bin/pip

# Download and install vllm ###########################################################
RUN git clone --depth 1 https://github.com/vllm-project/vllm.git \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no more installing vllm manually 😎😎😎

@joerunde joerunde changed the title [WIP] 👷 use UV package manager 👷 use UV package manager Mar 28, 2025
@joerunde
Copy link
Collaborator Author

@tdoublep I put all the code linting checks together into one job, so if we want to merge this we can remove the requirement on the codespell and yapf checks that no longer exist.

Comment on lines +57 to +58
if: always()
run: codespell --toml pyproject.toml
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add mypy here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, though right now that's running on multiple Python versions. I assume that's to make sure that mypy will be happy with any version of Python that somebody wants to develop locally with. But I guess it's also not too hard to just run all these checks with every Python version as well

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we really need to run it with every python version, this is just inherited from upstream checks.

Copy link
Member

@tdoublep tdoublep left a comment

Choose a reason for hiding this comment

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

LGTM

@joerunde joerunde merged commit e355aa7 into main Apr 3, 2025
9 checks passed
@joerunde joerunde deleted the use-uv branch April 3, 2025 15:12
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.

Cleanly handle installing dependencies that require torch

2 participants