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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #529 +/- ##
==========================================
- Coverage 97.31% 94.92% -2.39%
==========================================
Files 34 30 -4
Lines 1304 1282 -22
Branches 0 77 +77
==========================================
- Hits 1269 1217 -52
- Misses 35 37 +2
- Partials 0 28 +28 ☔ 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?
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
Co-authored-by: Alyssa Dai <alyssa.dai@mail.mcgill.ca>
Used the bot for this.
This needs to be removed in a separate PR.
There was a problem hiding this comment.
Thanks @surchs for the changes! 🎉
Everything is looking good, and I can confirm the uv commands now work for me locally without dependency conflicts.
I left some final minor fixes and README suggestions, otherwise 🧑🍳
Before you merge, can you double check the failing codecov check to see if there's anything we can do to make it green?
Co-authored-by: Alyssa Dai <alyssa.dai@mail.mcgill.ca>
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: