Conversation
Reviewer's GuideThis PR introduces uv-based dependency management for the project, wiring CI to use uv and adding a pyproject.toml with project metadata, dependency groups, and tooling configuration, plus a generated uv.lock lockfile. Flow diagram for uv dependency groups in pyproject.tomlgraph TD
Project[project_neurobagel-api]
Core_Dependencies[core_runtime_dependencies]
Dependency_Groups[dependency_groups]
Dev_Group[dev_group]
Test_Group[test_group]
Coverage_Group[coverage_group]
Lint_Group[lint_group]
Project --> Core_Dependencies
Project --> Dependency_Groups
Dependency_Groups --> Dev_Group
Dependency_Groups --> Test_Group
Dependency_Groups --> Coverage_Group
Dependency_Groups --> Lint_Group
Dev_Group --> Test_Group
Dev_Group --> Coverage_Group
Dev_Group --> Lint_Group
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #529 +/- ##
=======================================
Coverage 97.31% 97.31%
=======================================
Files 34 34
Lines 1304 1304
=======================================
Hits 1269 1269
Misses 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Will have to be replaced by updated global recipe from workflows repo
|
@surchs, initial comment here before I have time for a full review. I noticed in this PR that you added a dependency group for some linters/formatters that we already run automatically as part of pre-commit hooks (and don't need to be installed locally) as well as pre-commit CI on PRs. What does this add to our existing pre-commit setup? |
There was a problem hiding this comment.
Thanks a lot @surchs for implementing this and for your patience on my review! I've tested out uv locally using your branch and I think it will simplify dependency management considerably - happy to be rid of these restrictive requirements.txt files 🎉
Since we haven't updated dependencies in a while, I think the main issue atm is incompatibilities between locked package versions in uv.lock and the newer Python versions we want to support. Specifically:
- uv.lock currently specifies some pretty old versions of tools
- that don't have wheels for e.g., 3.12 or 3.13
- meanwhile, the new pyproject.toml allows 3.11-3.13
- but the test.yaml only runs tests against 3.10 (so the issues with newer Pythons are not caught in CI)
Left more details below and and a suggestion of how we can resolve this!
Two other high-level comments (with details below):
- I think we can include a bit more guidance in the README on using
uv. Given how muchuvabstracts away and the many commands available, I find that things like knowing which env I'm installing things into, how to add or update a dependency, etc. aren't obvious coming from pip + venv where each step is explicit. I think a few brief explainers of commands could go a long way in this regard (also to save ourselves confusion during this transition period). As mentioned below, we might want to eventually move this info into to our CONTRIBUTING.md - I don't follow the NOTE in your PR description about not using
uvin the test.yaml and Dockerfile. From what I can see those files have been updated anyways in this PR, but I find the temporary solution harder to review than if we were to fully useuvfor the Docker image and test wf. Can we consider making those changes here to avoid a second PR that reverts the changes?
| "pytest-asyncio", | ||
| "pytest-env", | ||
| ] | ||
| coverage = ["coverage"] |
There was a problem hiding this comment.
why not include this as part of the test group? to me that feels more intuitive rather than having a separate dependency group just for coverage alone
There was a problem hiding this comment.
side node: I think this is a good time to migrate these older repos to using pytest-cov for code coverage reports, which is the more modern/streamlined approach than using the standalone coverage,
for an example, see the pyproject.toml and test.yaml for configure-nb.
will leave up to you if you want to include the migration in the uv PRs or in another issue!
| lint = [ | ||
| "pre-commit", | ||
| "flake8", | ||
| "black", | ||
| "isort" | ||
| ] |
There was a problem hiding this comment.
| lint = [ | |
| "pre-commit", | |
| "flake8", | |
| "black", | |
| "isort" | |
| ] |
since flake8, black, and isort are already configured as pre-commit hooks in pre-commit-config.yaml, we don't need a separate dependency group for them (pre-commit will handle running these tools locally & as part of pre-commit CI which is enabled on the repo).
instead, I'd recommend simply moving pre-commit to the dev group!
| [tool.black] | ||
| line-length = 79 |
There was a problem hiding this comment.
thanks for centralizing the linter/formatter configs! with this, we can remove these config args from .pre-commit-config.yaml since pre-commit can respect pyproject.toml (for an example, see our py-app-template .pre-commit-config.yaml)
mind updating .pre-commit-config.yaml as part of this PR?
| requires = ["setuptools"] | ||
| build-backend = "setuptools.build_meta" |
There was a problem hiding this comment.
any specific reason to use setuptools over hatch? we use hatch currently in the bagel-cli and configure-nb (mostly because I was inspired by Nipoppy) - to my understanding it's more modern and has more features (e.g., dynamic versioning without much additional config)
might be good to be consistent across repos with the build system
| @@ -0,0 +1,57 @@ | |||
| [project] | |||
| name = "neurobagel-api" | |||
| dynamic = ["version"] | |||
There was a problem hiding this comment.
I think this line tells the build system that the version will be provided dynamically, but we still need to configure the build backend to say where the version actually comes from. e.g., in our py-app-template repo, we use hatch-vcs for the source: https://github.com/neurobagel/py-app-template/blob/c6b729a4a7d996b2de2e523ddbd1d6849b1232af/pyproject.toml#L57-L62
(see also my comment about switching to hatch from setuptools)
| [](https://github.com/neurobagel/api/actions?query=branch:main) | ||
| [](https://github.com/neurobagel/api/actions/workflows/test.yaml) | ||
| [](https://app.codecov.io/gh/neurobagel/api) | ||
| [](https://www.python.org) |
There was a problem hiding this comment.
Should we update this to 3.10-3.13?
| [](https://www.python.org) |
| - name: Lint with flake8 | ||
| run: | | ||
| # stop the build if there are Python syntax errors or undefined names | ||
| flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics --per-file-ignores=./app/api/models.py:F722 | ||
| # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide | ||
| flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics |
There was a problem hiding this comment.
this step is leftover from before we had pre-commit AFAIK, I think this is a good time to delete it since flake8 is already run as part of pre-commit CI
| # Note: this configuration only has an effect in repositories that have | ||
| # a requirements.txt file / use python / pip. | ||
| - package-ecosystem: "pip" | ||
| # This is a temporary fix until we update the global sync workflow |
There was a problem hiding this comment.
good catch 👍
mind creating an issue for this in https://github.com/neurobagel/workflows and then adding it to the uv milestone so we don't forget?
There was a problem hiding this comment.
I'm not sure I follow your reasoning behind not 'fully' migrating to uv in the CI test and Dockerfile in this PR: is the plan to open a separate follow-up PR in each Python repo for the remaining changes?
If so, I think that actually makes the review harder rather than easier, since in the current PR I have to verify that the uv + requirements.txt approach makes sense for these two files, only for it to be replaced shortly after.
Would it be possible to do the full migration here instead, since both test.yaml and Dockerfile necessarily already have diffs in this PR anyways? I'm assuming we'd follow the guides here:
- https://docs.astral.sh/uv/guides/integration/github/#installation
- https://docs.astral.sh/uv/guides/integration/docker/#installing-a-project
which doesn't look too complicated from a first glance.
LMK if I'm missing something!
There was a problem hiding this comment.
see comment above for using uv to install the project here instead (the current diff to me looks similar in complexity to the changes we'd need to make for the full switch)
2 participants
Update workflows
Changes proposed in this pull request:
uvto handle dependency lockfile for the projectNote
We want to also have our local dev environments use the lockfile dependencies (to make sure local dev is the same as prod). Achieving this without installing
uvlocally is going to be quite hard / impossible with this setup. So our local setup guide will now include having uv available to handle local venv and installation.Note
In the CI commands and the docker build, we do not use uv now. This is mainly to keep the diff limited. We can do this by first asking uv to build a
requirements.txtlock file from theuv.lockfile - and then using pip to install from therequirements.txtfile as before.Checklist
This section is for the PR reviewer
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF],[MODEL],[DOC]) (see our Contributing Guidelines for more info)skip-release(to be applied by maintainers only)Closes #XXXXFor new features:
For bug fixes:
Summary by Sourcery
Adopt uv-based Python project and dependency management and update CI to use it for installing and running development dependencies.
Enhancements:
CI:
Chores: