Skip to content

Conversation

@tpoliaw
Copy link
Contributor

@tpoliaw tpoliaw commented Jun 27, 2025

  • Use dependency-group instead of optional-dependencies
  • Specify python version upper bound
  • Initial workflow rewrite

Copy link
Contributor

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

Please can you change/add to the docs to reflect the new uv commands?

@tpoliaw tpoliaw force-pushed the uv_trial branch 2 times, most recently from 1fd9b2f to 79dc252 Compare July 4, 2025 11:00
@tpoliaw tpoliaw requested a review from coretl July 4, 2025 11:04
DiamondJoseph added a commit that referenced this pull request Jul 14, 2025
Update dependencies manually while awaiting #1089 to pick up updates to
ScanSpec that allow us to use Pydantic 2.11 and changes to our own
observability-utils and bluesky-stomp.
@tpoliaw tpoliaw force-pushed the uv_trial branch 2 times, most recently from 9ef69b0 to 91bf416 Compare September 3, 2025 13:35
@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.54%. Comparing base (6136b90) to head (2ef4662).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1089   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files          41       41           
  Lines        2566     2566           
=======================================
  Hits         2426     2426           
  Misses        140      140           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tpoliaw tpoliaw force-pushed the uv_trial branch 4 times, most recently from 0724855 to 102809e Compare September 3, 2025 14:52
@tpoliaw tpoliaw linked an issue Sep 3, 2025 that may be closed by this pull request
3 tasks
@tpoliaw tpoliaw marked this pull request as ready for review September 10, 2025 13:50
@tpoliaw tpoliaw requested a review from a team as a code owner September 10, 2025 13:50
"workspaceMount": "source=${localWorkspaceFolder}/..,target=/workspaces,type=bind",
// After the container is created, install the python project in editable form
"postCreateCommand": "pip install $([ -f dev-requirements.txt ] && echo '-c dev-requirements.txt') -e '.[dev]' && pre-commit install"
"postCreateCommand": "uv sync --frozen && uvx pre-commit install && SHELL=bash uv tool update-shell"
Copy link
Contributor

@coretl coretl Sep 10, 2025

Choose a reason for hiding this comment

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

Some questions:

  • Why --frozen? I would argue it is safer to do it without, as that will potentially update your lockfile if you added a dependency to pyproject.toml which you should at least know about before deciding whether it was intentional or not and committing the result
  • Why uvx rather than uv run? Pre-commit should be listed in the dev dependencies, and this will ensure we get a consistent version of it.
  • Do we use any uv tools? Why did you need to run this command to add the tool dir to the path?

Copy link
Contributor Author

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. --frozen doesn'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=allow to 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 uvx rather than uv run, might have been related to making pre-commit available everywhere without setting the $PATH to the venv as you did for ophyd-async. I will try it without and see what fails.

Also what does

I'm assuming there was meant to be more to this question

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what does

I'm assuming there was meant to be more to this question

Hah, didn't look before clicking comment, edited the original question

Copy link
Contributor

Choose a reason for hiding this comment

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

I've deleted this action in my version as I use a .python-version to determine the default python, then uv run everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be useful, I'll see if that's a viable option here

- name: Install produced wheel
uses: ./.github/actions/install_requirements
with:
# TODO: figure out what's going on
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked dists yet, what problems do you see?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.


- name: Run tests
run: tox -e system-test
run: uv run --frozen tox -e system-test
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe --frozen does make sense here, but maybe --locked would make even more sense? What do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--locked fails if any packages are updatable so would fail pretty much every run, --frozen makes it use the versions in the lock file.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not my reading of it:
https://docs.astral.sh/uv/concepts/projects/sync/#checking-the-lockfile

uv will not consider lockfiles outdated when new versions of packages are released — the lockfile needs to be explicitly updated if you want to upgrade dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

❯ git status
On branch uv_trial
Your branch is up to date with 'origin/uv_trial'.

nothing to commit, working tree clean

❯ uv sync --locked
Resolving despite existing lockfile due to change in pre-release mode: `allow` vs. `if-necessary-or-explicit`
Resolved 224 packages in 65ms
The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.

❯ echo $?
1

❯ uv sync --frozen
Audited 221 packages in 1ms

❯ echo $?
0

# or docker with user namespaces.
# Version SHA has been removed, see: https://github.com/DiamondLightSource/blueapi/issues/1053
ARG PYTHON_VERSION=3.11
FROM python:${PYTHON_VERSION} AS developer
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

# Make editable and debuggable
RUN pip install debugpy
RUN pip install -e .
RUN uv tool install debugpy
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

license.file = "LICENSE"
readme = "README.md"
requires-python = ">=3.11"
requires-python = ">=3.11,<3.13"
Copy link
Contributor

Choose a reason for hiding this comment

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

The current best practice appears to be never to cap python version for libraries:
https://iscinumpy.dev/post/bound-version-constraints/#tldr

I would argue it probably makes sense here too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was probably a hang over from epics(?) not supporting 3.13 and can be removed now, especially if we're using .python-version. I'm pretty sure uv ignores upper bounds anyway.

dodal, ophyd-async etc no longer need it.
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.

Trial-adopt migration to uv from copier template

4 participants