Conversation
There was a problem hiding this comment.
Pull request overview
Updates launcher unit-test dependency management and documentation so local runs and GitHub Actions install a consistent set of Python packages.
Changes:
- Pin launcher Python dependencies in
inference_server/launcher/requirements.txt(and addhttpx). - Update launcher unit-test instructions to use the
tests/paths. - Update the Python code-quality GitHub Actions workflow to install dependencies via the launcher
requirements.txt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
inference_server/launcher/requirements.txt |
Pins launcher dependencies and adds httpx; still includes vllm behind a macOS arm64-only exclusion marker. |
inference_server/launcher/howto.md |
Makes unit-test commands match the tests/ layout and references installing deps via requirements. |
.github/workflows/python-code-quality.yml |
Switches CI dependency installation from an explicit package list to -r inference_server/launcher/requirements.txt. |
MikeSpreitzer
force-pushed
the
unify-launcher-unit-test
branch
from
4946f2e to
a8ff0a7
Compare
MikeSpreitzer
added this to the
3 - System with model swapping and sleep/wake milestone
MikeSpreitzer
requested review from
Copilot,
diegocastanibm,
lionelvillard and
waltforme
Make the GHA workflow use the requirements file. Update the requirements file to versions that are more recent AND work on MacOS. Make all the instructions about running the tests consistent. Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>
MikeSpreitzer
force-pushed
the
unify-launcher-unit-test
branch
from
a8ff0a7 to
e2908b4
Compare
rubambiza
left a comment
rubambiza
left a comment
There was a problem hiding this comment.
Clarifying question, otherwise LGTM.
| Install all the necessary packages (feel free to use a later version of pytest if you prefer): | ||
| ```bash | ||
| pip install -r requirements.txt | ||
| pip install pytest==9.0.2 -r requirements-sans-vllm.txt |
There was a problem hiding this comment.
Is there a reason we're making an exception for pytest? That is, why not pin it as-is and put it in the requirements file?
There was a problem hiding this comment.
Same comment applies to the python-code-quality file.
There was a problem hiding this comment.
As far as I know, requirements.txt is typically used to state only the dependencies of the base software, not also the extra dependencies of the tests.
There was a problem hiding this comment.
Because pytest is a dev/testing-only dependency, not a runtime dependency of the launcher. The requirements-sans-vllm.txt file lists runtime deps
There was a problem hiding this comment.
The requirements.txt file is the one that lists the full dependencies of the launcher; requirements-sans-vllm.txt lists the subset of requirements.txt that is needed for testing the launcher. There is a difference because when being tested, we want to use the test's mocks of vllm rather than the real vllm.
| Install all the necessary packages (feel free to use a later version of pytest if you prefer): | ||
| ```bash | ||
| pip install -r requirements.txt | ||
| pip install pytest==9.0.2 -r requirements-sans-vllm.txt |
There was a problem hiding this comment.
Because pytest is a dev/testing-only dependency, not a runtime dependency of the launcher. The requirements-sans-vllm.txt file lists runtime deps
Make the GHA workflow use the requirements file.
Update the requirements file to versions that are more recent AND work on MacOS.
Make all the instructions about running the tests consistent.
Fixes #366