-
Notifications
You must be signed in to change notification settings - Fork 12
Update copier template #1809
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
Update copier template #1809
Conversation
dcd2788 to
2853236
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1809 +/- ##
=======================================
Coverage 99.14% 99.14%
=======================================
Files 288 288
Lines 10957 10957
=======================================
Hits 10863 10863
Misses 94 94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
259ad42 to
ced2709
Compare
1ae1d48 to
45d796d
Compare
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.
Need to also update dev-install.md - remove the stuff that talks about pip.
Are we going down the route of devcontainers as the official recommended way?
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.
Also should go through the docs and update all the places where it asks you to run tox and replace with uv run tox
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.
Have updated the docs, also gotten rid of uv run tox -e unit-report. uv run tox -e tests now only does unit tests.
Are we going down the route of devcontainers as the official recommended way?
This I'm not sure on, the docs make it sound like it's an option but not essential
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.
Actually I think I may have sent you on a bit of a wild goose chase - I think actually you only need uv run if the dev environment isn't activated, therefore uv run isn't strictly needed.
a053727 to
da867c8
Compare
pyproject.toml
Outdated
| [ | ||
| "pyright", | ||
| "--pythonpath", | ||
| ".venv/bin/python", |
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.
is there a better way of doing this? I don't like to assume the location of the venv, since often I have multiple different venvs in the same project, both if working across multiple projects (think dodal and mx-bluesky at the same time) or if testing behaviour on different python versions.
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.
Well, it seems like uv behaviour is very unexpected when your virtual environment is not installed in .venv, I got this cryptic message:
(.venv-uv) [yqf46943@ws502 mx-bluesky]$ uv sync
warning: `VIRTUAL_ENV=.venv-uv` does not match the project environment path `.venv` and will be ignored; use `--active` to target the active environment instead
lovely comment
It seems kind of wild to not respect VIRTUAL_ENV, for any command honestly. I'm not sure I can imagine a drawback. in the happy path, no one is setting it, or otherwise activating the virtualenv. but in every other case, it's the path of least surprise to me.
seems like there is a workaround
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.
Setting runner = "uv-venv-lock-runner" seems to work without the --pythonpath being set. @coretl was there a reason for using the python path when converting the copier template to uv?
As a side note, using uv is quick enough that it removes much if not all the need for creating multiple environments.
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.
@tpoliaw what is the workflow when you edit two projects in the same env?
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.
To run something I use uv run --with /path/to/other/project ...
To make the editor LSP stuff work, I'd use uv pip install -e /path/to/other project
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.
It does rather depend though on you being in the right directory, otherwise it is liable to find the wrong .venv
Either that or I have to set UV_PROJECT_ENVIRONMENT.
Perhaps we should consider for mx-bluesky moving the .venv up one level. Although that would create its own problems as at least on my workstation all the projects are at that level so it would require rearranging the project hierarchy
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.
Have changed that command so that by default it still use ./venv/bin/python, but you can override it like this
tox -e type-checking -- /path/to/python
Should I add this option to all tox commands?
Also could use the UV_PROJECT_ENVIRONMENT if you think that's better.
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.
This was added here:
DiamondLightSource/python-copier-template@73c31e3
The test case that was added was broken initially, and fixed by adding --pythonpath. If the test in the copier template works without --pythonpath because of an updated pyright then happy to see it removed
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.
There was a bug in that, please can you see if DiamondLightSource/python-copier-template#320 fixes this for you? If so I can make a new copier template release
rtuck99
left a comment
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.
Think everything is working now, have pushed a change for pyright virtualenv as suggested by @coretl if that works then I think this is now good.
In terms of developer workflow, I am currently trialling this plus the mx-bluesky change in pycharm with a single uv workspace across mx-bluesky and a bunch of dependencies. Will see how that goes
Fixes #1807
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}