Cross-platform third-party stubs requirements install script #13482
Conversation
| from ts_utils.requirements import get_external_stub_requirements | ||
|
|
||
| requirements = get_external_stub_requirements() | ||
| subprocess.check_call(("pip", "install", *[str(requirement) for requirement in requirements])) |
There was a problem hiding this comment.
nit: we have uv as a dependency in requirements-tests.txt. I think the prior steps in the README would already have failed if you hadn't installed those dependencies, by the time you get to the instructions about this script on line 30
| subprocess.check_call(("pip", "install", *[str(requirement) for requirement in requirements])) | |
| subprocess.check_call(("uv", "pip", "install", *map(str,requirements))) |
There was a problem hiding this comment.
I initially considered it. But we still have a handful of places where we use pip directly instead of uv. And last I checked, uv doesn't tend to respect the activated environment (or lack thereof if contributor wants to install to system Python). Which feels like a speedbump to newer contributors or those using custom venvs.
From our contributing guidelines:
$ python3 -m venv .venv
$ source .venv/bin/activate
(.venv)$ pip install -U pip
(.venv)$ pip install -r requirements-tests.txtThe current places where uv pip is used in tests is where temporary venvs are created anyway.
There was a problem hiding this comment.
Hmm, I'm pretty sure it does respect the activated environment — if you have an example where it doesn't, that definitely sounds like a bug you should report! 😃 but you're right that some of the external requirements might not be PEP-517-compliant. So, probably best to stick with pip for now 👍
There was a problem hiding this comment.
(FWIW it does seem like all the packages installed by this script are PEP-517 compliant and installable by uv)
There was a problem hiding this comment.
Hmm, I'm pretty sure it does respect the activated environment — if you have an example where it doesn't, that definitely sounds like a bug you should report!
Looks like that may have just changed in 0.5.29. This is what I used to get: astral-sh/uv#1495 (comment)
So, probably best to stick with pip for now 👍
That being said, I wouldn't mind having a follow-up that updates all python -m pip, python3 -m pip, and pip usages in docs and scripts for uv pip. (except for pip install -r requirements-tests.txt because that's a bit of a catch-22)
There was a problem hiding this comment.
Looks like that may have just changed in
0.5.29. This is what I used to get: astral-sh/uv#1495 (comment)
Oh, right. Yeah, it might not respect the activated environment for the "project API" commands like uv sync. It should always respect it for any uv pip commands though, I think?
| if __name__ == "__main__": | ||
| distributions = sys.argv[1:] | ||
| for requirement in sorted(get_external_stub_requirements(distributions), key=str): | ||
| print(requirement) |
There was a problem hiding this comment.
I think we could get rid of the mapfile usage in our workflows if we made a couple of tweaks here:
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index 32cd27c49..8e222aebf 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -47,7 +47,7 @@ jobs:
- run: uv pip install -r requirements-tests.txt --system
- name: Install external dependencies for 3rd-party stubs
run: |
- mapfile -t DEPENDENCIES < <( python tests/get_external_stub_requirements.py )
+ DEPENDENCIES=$( python tests/get_external_stub_requirements.py )
if [ -n "$DEPENDENCIES" ]; then
echo "Installing packages:"
for DEP in "${DEPENDENCIES[@]}"; do echo " ${DEP}"; done
@@ -108,7 +108,7 @@ jobs:
run: uv venv .venv
- name: Install 3rd-party stub dependencies
run: |
- mapfile -t DEPENDENCIES < <( python tests/get_external_stub_requirements.py )
+ DEPENDENCIES=$( python tests/get_external_stub_requirements.py )
if [ -n "$DEPENDENCIES" ]; then
echo "Installing packages:"
for DEP in "${DEPENDENCIES[@]}"; do echo " ${DEP}"; done
diff --git a/tests/get_external_stub_requirements.py b/tests/get_external_stub_requirements.py
index 177da48ab..144fd8478 100755
--- a/tests/get_external_stub_requirements.py
+++ b/tests/get_external_stub_requirements.py
@@ -8,6 +8,4 @@ import sys
from ts_utils.requirements import get_external_stub_requirements
if __name__ == "__main__":
- distributions = sys.argv[1:]
- for requirement in sorted(get_external_stub_requirements(distributions), key=str):
- print(requirement)
+ print(" ".join(sorted(map(str, get_external_stub_requirements(sys.argv[1:])))))There was a problem hiding this comment.
Sure, that's in line with where my changes are going. As mentioned in the PR description. I think the entire run code block could be replaced by a reusable python script.
There was a problem hiding this comment.
ugh, maybe getting rid of the
mapfileusage isn't quite as easy as I thought. Sorry for the bother -- maybe just revert the last commit :-)
I figured it out ^^
|
ugh, maybe getting rid of the |
Avasam
deleted the
Cross-platform-third-party-stubs-requirements-install-script
branch
I got tired of translating the bash script to powershell, so there you go.
As further steps it's probably possible to obsolete the
get_external_stub_requirementsandget_stubtest_system_requirementsscripts entirely with a few modifications (like cli flags for "system vs local" install, "uv vs pip", and specifying third-party stubs to install from) to run such install scripts directly on the CI.