-
Notifications
You must be signed in to change notification settings - Fork 6
Convert to using uv #1089
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
base: main
Are you sure you want to change the base?
Convert to using uv #1089
Changes from all commits
e320979
0dfb460
091c6c4
9a4050a
a3c6058
8ba2727
c229d70
335dd05
9e37240
e081661
f47427a
cf38536
d69f43f
acb1741
6f7af1c
c989b4d
1caa29e
8579853
261e064
8784091
b49bf2c
33b284e
4798fa9
d3cd892
3ee9de6
ab1cdde
d52bb61
2ef4662
522b3ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,13 @@ jobs: | |
| # Need this to get version number from last tag | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Install | ||
| uses: ./.github/actions/install_requirements | ||
|
|
||
| - name: Build sdist and wheel | ||
| run: > | ||
| export SOURCE_DATE_EPOCH=$(git log -1 --pretty=%ct) && | ||
| pipx run build | ||
| uv build | ||
|
|
||
| - name: Upload sdist and wheel as artifacts | ||
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 | ||
|
|
@@ -24,11 +27,12 @@ jobs: | |
| path: dist | ||
|
|
||
| - name: Check for packaging errors | ||
| run: pipx run twine check --strict dist/* | ||
| run: uvx twine check --strict dist/* | ||
|
|
||
| - name: Install produced wheel | ||
| uses: ./.github/actions/install_requirements | ||
| with: | ||
| # TODO: figure out what's going on | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't checked dists yet, what problems do you see?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None but I wasn't sure what I was looking for. The comment was more that I had no idea what the step was meant to be doing. |
||
| pip-install: dist/*.whl | ||
|
|
||
| - name: Test module --version works using the installed wheel | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,4 +50,4 @@ jobs: | |
| run: blueapi -c ${{ github.workspace }}/tests/system_tests/config.yaml serve & | ||
|
|
||
| - name: Run tests | ||
| run: tox -e system-test | ||
| run: uv run --frozen tox -e system-test | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, maybe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not my reading of it:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might have been to do with the pre-release stuff again. It looks like dodal has been updated now so it doesn't depend on alpha releases but with the current lock file: |
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,31 +3,33 @@ | |
| # Version SHA has been removed, see: https://github.com/DiamondLightSource/blueapi/issues/1053 | ||
| ARG PYTHON_VERSION=3.11 | ||
| FROM python:${PYTHON_VERSION} AS developer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched to ubuntu and use a uv managed python, this seems to work now
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds sensible, I will look into that. Does it re-use the downloaded python or does it need to download it again every time it's built?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Difficult to tell, the download takes a fraction of a second on my machine, so I miss the output. I think it is downloaded each time, but I can't be sure |
||
| COPY --from=ghcr.io/astral-sh/uv:0.7.17 /uv /uvx /bin/ | ||
|
|
||
| # Add any system dependencies for the developer/build environment here | ||
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| graphviz \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Install helm for the dev container. This is the recommended | ||
| # Install helm for the dev container. This is the recommended | ||
| # approach per the docs: https://helm.sh/docs/intro/install | ||
| RUN curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3; \ | ||
| chmod 700 get_helm.sh; \ | ||
| ./get_helm.sh; \ | ||
| rm get_helm.sh | ||
| RUN helm plugin install https://github.com/losisin/helm-values-schema-json.git | ||
|
|
||
| # Set up a virtual environment and put it in PATH | ||
| RUN python -m venv /venv | ||
| ENV PATH=/venv/bin:$PATH | ||
| RUN mkdir -p /.cache/uv; chmod 777 /.cache/uv | ||
| ENV UV_CACHE_DIR=/.cache/uv | ||
| RUN SHELL=/usr/bin/bash uv tool update-shell | ||
|
|
||
| # The build stage installs the context into the venv | ||
| FROM developer AS build | ||
| RUN mkdir -p /.cache/pip; chmod o+wrX /.cache/pip | ||
|
|
||
| # Requires buildkit 0.17.0 | ||
| COPY --chmod=o+wrX . /workspaces/blueapi | ||
|
|
||
| WORKDIR /workspaces/blueapi | ||
| RUN touch dev-requirements.txt && pip install --upgrade pip && pip install -c dev-requirements.txt . | ||
| RUN uv sync --frozen | ||
|
|
||
|
|
||
| FROM build AS debug | ||
|
|
@@ -43,8 +45,8 @@ RUN DEBIAN_FRONTEND=noninteractive apt install libnss-ldapd -y | |
| RUN sed -i 's/files/ldap files/g' /etc/nsswitch.conf | ||
|
|
||
| # Make editable and debuggable | ||
| RUN pip install debugpy | ||
| RUN pip install -e . | ||
| RUN uv tool install debugpy | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is why you need uv tools to be on your path, but I don't see why you need it in the devcontainer.json too?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might not, I don't use devcontainers so might have been holding it wrong when I tested it. I'll try again without. |
||
| RUN uv tool install --editable . | ||
|
|
||
| # Alternate entrypoint to allow devcontainer to attach | ||
| ENTRYPOINT [ "/bin/bash", "-c", "--" ] | ||
|
|
@@ -58,9 +60,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ | |
| # Git required for installing packages at runtime | ||
| git \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
| COPY --from=build --chmod=o+wrX /venv/ /venv/ | ||
| COPY --from=build --chmod=o+wrX /.cache/pip /.cache/pip | ||
| ENV PATH=/venv/bin:$PATH | ||
| COPY --from=build --chmod=777 /.cache/uv /.cache/uv | ||
| COPY --from=build --chmod=777 /workspaces/blueapi /workspaces/blueapi | ||
| ENV PATH=/workspaces/blueapi/.venv/bin:$PATH | ||
| ENV UV_CACHE_DIR=/.cache/uv | ||
| ENV PYTHONPYCACHEPREFIX=/tmp/blueapi_pycache | ||
|
|
||
| # For this pod to understand finding user information from LDAP | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions:
--frozen? I would argue it is safer to do it without, as that will potentially update your lockfile if you added a dependency topyproject.tomlwhich you should at least know about before deciding whether it was intentional or not and committing the resultuvxrather thanuv run? Pre-commit should be listed in the dev dependencies, and this will ensure we get a consistent version of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the frozen comment.
--frozendoesn't update the lockfile and ensures the environment you have is the same as the one expected. Without it I couldn't get it to not need--prerelease=allowto deal with dodal et al depending on alpha versions of things and I didn't want that to be something we encourage.I can't remember the motivation for
uvxrather thanuv run, might have been related to making pre-commit available everywhere without setting the$PATHto the venv as you did for ophyd-async. I will try it without and see what fails.I'm assuming there was meant to be more to this question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, didn't look before clicking comment, edited the original question