Skip to content
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

feat(providers): add uv_provider #1351

Merged
merged 3 commits into from
Feb 28, 2025
Merged

feat(providers): add uv_provider #1351

merged 3 commits into from
Feb 28, 2025

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Jan 31, 2025

closes: #1349

Description

as title

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

uv.lock should also updated when running cz bump

Steps to Test This Pull Request

uv init
uv add commitizen
cz init
git add .
cz c
cz bump

Additional context

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.58%. Comparing base (120d514) to head (8bddb2d).
Report is 566 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1351      +/-   ##
==========================================
+ Coverage   97.33%   97.58%   +0.24%     
==========================================
  Files          42       56      +14     
  Lines        2104     2645     +541     
==========================================
+ Hits         2048     2581     +533     
- Misses         56       64       +8     
Flag Coverage Δ
unittests 97.58% <100.00%> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Lee-W Lee-W force-pushed the uv-provider branch 6 times, most recently from 3109dda to 0450542 Compare February 18, 2025 14:28
@Lee-W Lee-W marked this pull request as ready for review February 18, 2025 14:29
@Lee-W Lee-W mentioned this pull request Feb 18, 2025
@Lee-W
Copy link
Member Author

Lee-W commented Feb 20, 2025

@woile @noirbizarre just a gentle ping in case this is missed. thanks 🙏

Copy link
Member

@woile woile left a comment

Choose a reason for hiding this comment

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

It looks good, the only issue is that it doesn't cover workspace.

[tool.uv.workspace]
members = ["packages/*"]
exclude = ["packages/seeds"]

@Lee-W
Copy link
Member Author

Lee-W commented Feb 21, 2025

It looks good, the only issue is that it doesn't cover workspace.

[tool.uv.workspace]
members = ["packages/*"]
exclude = ["packages/seeds"]

I'm a bit confused why do we need to cover workspace (I've never used this feature so I might miss some context 🤦‍♂️)

@woile
Copy link
Member

woile commented Feb 21, 2025

It allows to have multiple python projects, as a monorepo. Each package probably (as in, I think it works quite similar to cargo) will add an entry to the lock. I'm working on a fix for cargo. If it works well we can port it to uv

Copy link
Member

@noirbizarre noirbizarre left a comment

Choose a reason for hiding this comment

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

Seems good to me but I wonder why for uv we handle the lock while we don't do it for poetry nor any other python provider.
I feel this is introducing a uv special treatment that could already be handled by relying on the generic PEP621 support we already have and using post_bump_hooks

Note

Maybe the pyproject.toml formatting should be splitted in a style commit in another PR as unrelated

@noirbizarre
Copy link
Member

noirbizarre commented Feb 21, 2025

I investigated a bit, and neither pdm nor poetry lock the root package itself in the lock which, to me, makes sense.
It is a weird choice from uv to lock the package itself, including the version, which doesn't have any sense outside pypi publication, but at lest I understand why this special treatment.

Given I always use dynamic version with scm, I need to check how uv handles it (cf. https://docs.astral.sh/uv/concepts/cache/#dynamic-metadata)
It is possible that using scm with uv requires the same lock update and then it might be more interesting to provide this as a reusable hook for both toml and scm provider.

WDYT ?

@noirbizarre
Copy link
Member

Also, as it has been a recurrent issue working with some lock files, I would add lock file version check (cf. https://docs.astral.sh/uv/concepts/resolution/#lockfile-versioning).
This would allow to:

  • add support for future lockfile versioning while continuing to support previous format and not having commitizen version tied to uv version and its lock versioning
  • warn when encountering an unsupported lock version, instead of failing or corrupting it

@woile
Copy link
Member

woile commented Feb 21, 2025 via email

@noirbizarre
Copy link
Member

noirbizarre commented Feb 22, 2025

But then it means that to have scm support with uv, we will need to have a separated UvScmProvider and then for each combination of provider + known lock file we would need a dedicated class.

My point does not imply that we shouldn't have explicit support for this (I am all for both explicitness and uv.lock support) but that after reviewing it, I don't think this should be a dedicated version provider but some kind of reusable pre_bump_hook.
It would still be explicit, but it would also work with the scm provider.
Plus, this is only true when the package is self installed, which is not always the case.
The uv.lock file looks more like a special version_file to handle and after thinking a lot about this, I believe having support for "special" files hook would be a great addition.

The way I see it, instead of having:

version_provider = "uv"

we should have something like:

pre_bump_hook = [
   "canonical.path.to.hook:uvlock",
]

or

pre_bump_hook = [
   "canonical.path.to.hooks:lock('uv.lock')",
]

or if we expose a new hook endpoint:

pre_bump_hook = [
   "uvlock:uv.lock",
   # or
   "hooks:uvlock",
]

Note

I don't know whether it should support parameters or not, that's why I imagine different syntaxes)

I see plenty of possibilities with this approach because sometimes, the version_files handling is not enough, and it would allow us and users to provide reusable complex files handling.
We already have the pre_bump_hooks working with script path, it would just mean supporting canonical reference to functions

WDYT ?

@Lee-W
Copy link
Member Author

Lee-W commented Feb 23, 2025

It allows to have multiple python projects, as a monorepo. Each package probably (as in, I think it works quite similar to cargo) will add an entry to the lock. I'm working on a fix for cargo. If it works well we can port it to uv

Got it. Yep, maybe we can do it in another PR, and I'll spend some time to look into how it works

@Lee-W
Copy link
Member Author

Lee-W commented Feb 23, 2025

I only have time to browse through the comments. But @noirbizarre 's seems to be a good alternative 🤔 I might need some time to think through it again. But I'll be out for the next 2 weeks 🤦‍♂️ We can merge it if we want this in earlier. Otherwise, I'll continue to work on it after I'm back. Thanks!

@woile
Copy link
Member

woile commented Feb 23, 2025

We might have to think this through a bit better.

On one hand, I'd like things to just work. I don't want users to have to configure many different chunks when using something like uv, it should just work, and not fail the next CI pipeline.

On the other hand, we have scm where the user chooses to use the version from git. I think here the user should update all the relevant files using the option version_files, but then we have the issue with multi-line files, specially lock files.

Right now, our provider cargo is also broken, because the lock is not being updated, and the same seems to be the case for uv.

To me, uv's lock file is not special, packages managers, which I usually never run issues with (unlike poetry), have the package version in the lock itself (I don't know if it's related or not to the lock though, so take it with a pinch of salt):

  • npm (js)
  • cargo (rust)
  • uv (python)

So I'd say it's a pretty standard feature.

Could we, perhaps, have both, a working uv and potentially "hooks"?
Compose the uv provider with "chunks", which an advanced user could use to compose its own "provider"? Is it too much? WDYT?


The discussion for PEP751 seems to lean towards not including the package version in the lockfile (can someone check that I understood properly?): https://discuss.python.org/t/pep-751-one-last-time/77293/68
I think it will be optional for auditing purposes, see pep

@Lee-W
Copy link
Member Author

Lee-W commented Feb 23, 2025

We might have to think this through a bit better.

On one hand, I'd like things to just work. I don't want users to have to configure many different chunks when using something like uv, it should just work, and not fail the next CI pipeline.

On the other hand, we have scm where the user chooses to use the version from git. I think here the user should update all the relevant files using the option version_files, but then we have the issue with multi-line files, specially lock files.

Right now, our provider cargo is also broken, because the lock is not being updated, and the same seems to be the case for uv.

To me, uv's lock file is not special, packages managers, which I usually never run issues with (unlike poetry), have the package version in the lock itself (I don't know if it's related or not to the lock though, so take it with a pinch of salt):

* npm (js)

* cargo (rust)

* uv (python)

So I'd say it's a pretty standard feature.

Could we, perhaps, have both, a working uv and potentially "hooks"? Compose the uv provider with "chunks", which an advanced user could use to compose its own "provider"? Is it too much? WDYT?

The discussion for PEP751 seems to lean towards not including the package version in the lockfile (can someone check that I understood properly?): https://discuss.python.org/t/pep-751-one-last-time/77293/68 I think it will be optional for auditing purposes, see pep

I'll try to spend some time rereading the PEP and think it through. This is a great discussion 🙌 really like it.

@noirbizarre
Copy link
Member

Agreed, I like this direction this is going.
I have no problem with merging the uv provider until we come with a more "standard" and scalable way to handle such kind of structured file formats.

Copy link
Member

@noirbizarre noirbizarre left a comment

Choose a reason for hiding this comment

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

LGTM until we have file-specific hooks 🚀

@Lee-W
Copy link
Member Author

Lee-W commented Feb 26, 2025

Ok, then I'll probably merge this later today. Another thing I'm thinking of is multiple provider? not sure whether it makes sense 🤔

@noirbizarre
Copy link
Member

I think it is easier to stick with the 1 provider only for the version source but support for multiple hooks for specific files.
The main purpose of the Version provider is to designate the source of trust for the version and extract it, setting it back is optional (like in scm where it is never set).
As a consequence, having multiple providers seems a bit complicated to handle and a big source of complexity.
However, this is already the purpose of version_files to spread the new version in multiple files as a side effect of bump.
We could say that we restrict version provider to read and handle all writes as version_files but I don't think it is necessary and would add unnecessary verbosity and redundancy.

@Lee-W Lee-W merged commit 9639da1 into master Feb 28, 2025
18 checks passed
@Lee-W Lee-W deleted the uv-provider branch February 28, 2025 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add uv provider
3 participants