-
Notifications
You must be signed in to change notification settings - Fork 3
Use lowest-direct resolution by default #15
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
Conversation
pyproject.toml
Outdated
| "pytest>=8.4.2", | ||
| "pytest-asyncio>=1.1.0", | ||
| "pytest-cov>=7.0.0", | ||
| "ruff~=0.12.12", # Stay compatible with 0.12 until we want to bump to 0.13, where formatting may change |
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.
I wonder if we should use strict versions for all the dev dependencies (maybe not the optional runtime ones like brotli, zstandard)? I wouldn't want someone (especially an external contributor) to have their PR fail because highest ran with some newer version of pytest that broke something for example - we basically lose a lot of the reason for having a lock file.
Ideally we could leave that to CI running on an auto-update workflow but I'm not sure dependabot handles pyproject.toml updates for uv projects, renovate would.
I think we can follow up on this though instead of requiring for this PR
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.
if we should use strict versions for all the dev dependencies
do you mean just pin the versions for dev dependencies? I'd be happy with that.
Ideally we could leave that to CI running on an auto-update workflow but I'm not sure dependabot handles pyproject.toml updates for uv projects, renovate would.
yeah, dependabot isn't great for uv yet :(
I wouldn't want someone (especially an external contributor) to have their PR fail because
highestran with some newer version of pytest that broke something for example - we basically lose a lot of the reason for having a lock file.
👍
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.
do you mean just pin the versions for dev dependencies? I'd be happy with that.
Yeah that's what I was thinking. BTW out of curiosity any thoughts on using renovate? I guess it may not make sense to run on a single repo in the connectrpc org but not sure.
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.
I think it'd be ok if we switch to renovate; I have heard that it's a lot better for uv at this point, especially if it helps out with syncing the version pins across the pyproject.tomls. (I think we're also the only connectrpc project using codecov and that's fine as well.)
And cool, I can just update these to direct pins for now and see if renovate can bump both the pins and uv.lock for dev dependencies?
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.
Pinned the dev deps in b8b5cf9.
| ] | ||
| requires-python = ">= 3.10" | ||
| dependencies = ["httpx", "protobuf>=5.28"] | ||
| dependencies = ["httpx>=0.28.1", "protobuf>=5.28"] |
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.
Thanks for finding the floor probably was pretty annoying :)
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.
hah - I was going to comment that this is almost certainly not the floor, actually just the latest release, but was going to see if we could start with being strict about this dep and loosen it if someone asks (not a breaking change).
I think with httpx being pre-1.0 and this release being 9 months old, I imagine most projects using it are probably pretty close to the bleeding edge of releases. Happy to try to loosen it here, though.
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.
Ah oops - got it. Yeah that strategy makes sense to me
| build-backend = "uv_build" | ||
|
|
||
| [tool.ruff] | ||
| extend = "../pyproject.toml" |
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.
Thanks always forget this
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 just moved down in the diff, don't worry, you didn't forget :).
my linter was just complaining about the toml ordering of keys; I'll leave this change but not bother with it in the future.
| version: v2 | ||
| plugins: | ||
| - remote: buf.build/protocolbuffers/python:v32.0 | ||
| # NOTE: v26.0 is the earliest version supporting protobuf==5. |
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.
IIRC, this causes a warning when running with the latest protobuf, which would happen on CI for highest, and eventually it might fail completely if such compatibility ends.
This is probably fine, especially since I can't think of anyway to improve it... Maybe we should copy the above content into the 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.
I think the newest versions of protobuf removed a lot of those warnings as they were noisy; this is pretty much the same setup as protovalidate-python and while it has some warnings in tests from protobuf when running on highest, I don't think we'll hit them in connect-python.
Separately, I just noticed that they added a section to the compatibility page on protobuf.dev for python — basically, my read of that is that in theory, someone running protobuf<8 could use gencode all the way back to the 3.20 plugin release. I think this is a reasonable middle ground, though.
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.
Oh nice - I didn't expect protobuf team to ever provide such wide compatibility guarantees ;) But then this looks even better
| ] | ||
|
|
||
| [tool.uv.workspace] | ||
| members = ["conformance", "example", "noextras"] |
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.
I think the main reason I had it as a workspace member was I didn't think pytest would behave well otherwise, but apparently it does.
I also have a longish term idea of running tests with more app servers, HTTP/1 uvicorn, WSGI gunicorn, etc. It's probably fine to add them all in dev-dependencies here but what do you think?
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.
yeah, to be honest this is my first experience w/ uv workspaces so I wasn't sure how best to use them; with adding all the dependency specifiers, I thought it would be nice if we just consolidated on basically two pyproject.tomls for the implementation and the plugin. I agree that I think it would be best if we just had them all in the top-level dev dependencies of pyproject.toml.
Not going to try to remove the other two workspace members in this PR, but something to think about?
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.
I think conformance was definitely the least compelling usage of workspaces as we see here. But the others have more technical reasons
-
example as a built project becomes trivial to import for "testable examples" while still being separate enough to be an example. I believe accomplishing it without the project would require pythonpath manipulation which can get tricky
-
noextras needs to exclude the optional runtime dependencies, and uv provides an easy way to do that with
--exacton the smaller pyproject.toml
Both can probably be worked around if we need to. IIUC the biggest problem with multiple projects is having to propagate version pins across all the projects which is indeed tedious. uv provides dependency management at the top level with for example
[tool.uv]
override-dependencies = ["pytest ==8.42"]
Would this be a reasonable approach to keep the above? Maybe it's too weird though
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.
I'm fine with keeping them around if there's a good technical reason, honestly I just didn't grok all of it at first :). Might be less of a big deal if renovate helps with keeping the pins in sync.
|
FWIW I think I have one or two more things to fix here and then going to probably squash it all down to a single signed-off commit; hoping to get some time this week to wrap it up. |
Trying to keep our production dependencies as wide as is reasonable, but fine to pin our development dependencies tight. Dropped the gencode down to the plugin version that works with protobuf==5 (from protovalidate-python). Worked around a couple of minor issues. Fixes #13. Signed-off-by: Stefan VanBuren <svanburen@buf.build>
7871e57 to
361e746
Compare
|
Ready for looks; open to thoughts on the approach and otherwise! |
anuraaga
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.
Sorry if my previous review jumped the gun!
| version: v2 | ||
| plugins: | ||
| - remote: buf.build/protocolbuffers/python:v32.0 | ||
| # NOTE: v26.0 is the earliest version supporting protobuf==5. |
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.
Oh nice - I didn't expect protobuf team to ever provide such wide compatibility guarantees ;) But then this looks even better
| ] | ||
|
|
||
| [tool.uv.workspace] | ||
| members = ["conformance", "example", "noextras"] |
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.
I think conformance was definitely the least compelling usage of workspaces as we see here. But the others have more technical reasons
-
example as a built project becomes trivial to import for "testable examples" while still being separate enough to be an example. I believe accomplishing it without the project would require pythonpath manipulation which can get tricky
-
noextras needs to exclude the optional runtime dependencies, and uv provides an easy way to do that with
--exacton the smaller pyproject.toml
Both can probably be worked around if we need to. IIUC the biggest problem with multiple projects is having to propagate version pins across all the projects which is indeed tedious. uv provides dependency management at the top level with for example
[tool.uv]
override-dependencies = ["pytest ==8.42"]
Would this be a reasonable approach to keep the above? Maybe it's too weird though
| ] | ||
| requires-python = ">= 3.10" | ||
| dependencies = ["httpx", "protobuf>=5.28"] | ||
| dependencies = ["httpx>=0.28.1", "protobuf>=5.28"] |
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.
Ah oops - got it. Yeah that strategy makes sense to me
pyproject.toml
Outdated
| "pytest>=8.4.2", | ||
| "pytest-asyncio>=1.1.0", | ||
| "pytest-cov>=7.0.0", | ||
| "ruff~=0.12.12", # Stay compatible with 0.12 until we want to bump to 0.13, where formatting may change |
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.
do you mean just pin the versions for dev dependencies? I'd be happy with that.
Yeah that's what I was thinking. BTW out of curiosity any thoughts on using renovate? I guess it may not make sense to run on a single repo in the connectrpc org but not sure.
We'll have these upgraded by Dependabot/Renovate, so that we don't unexpectedly break things when running on 'highest' resolution in PRs. Signed-off-by: Stefan VanBuren <svanburen@buf.build>
We'll inherit this from pytest-cov instead. Signed-off-by: Stefan VanBuren <svanburen@buf.build>
anuraaga
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.
Thanks! Let's follow up on renovate
pyproject.toml
Outdated
| "Topic :: Internet :: WWW/HTTP", | ||
| "Topic :: Software Development :: Libraries :: Python Modules", | ||
| "Typing :: Typed", | ||
| "Development Status :: 3 - Alpha", |
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.
Just a double check, FWICT the indent change is from bumping ruff to 0.13. Not a problem, I get the same formatting when checking out the PR and just wanted to make sure it's not an environment specific change.
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.
ugh, no, not a ruff change. sorry, have tombi enabled locally and didn't even notice it was changing things 🤦 . Let me revert that to not confuse things.
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.
Interesting - I guess my attempts to break the formatting must have been thwarted by some heuristics on the indents within the files.
I'm a fan of auto formatting so let's follow up on whether we can add tombi to the CI
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.
sounds good. I know tombi is pre-1.0 but it's been pretty good so far. I've also seen projects like https://github.com/tox-dev/toml-fmt that are more pyproject-specific that we could look at. Open to whatever; agree that auto-formatting is nice.
Need to disable tombi's formatting :) Signed-off-by: Stefan VanBuren <svanburen@buf.build>
Trying to keep our production dependencies as wide as is reasonable, but fine to pin our development dependencies tight. Dropped the gencode down to the plugin version that works with protobuf==5 (from protovalidate-python). Worked around a couple of minor issues.
Fixes #13.