chore: migrate dependency management to uv#507
Conversation
4b68ff4 to
6850dfe
Compare
rpocase
changed the title
There was a problem hiding this comment.
Pull request overview
Migrates the project from legacy requirements/setup.cfg packaging to a pyproject.toml-driven build (Hatchling) and dependency management aligned with uv, while updating type hints/signatures to satisfy a newer mypy.
Changes:
- Replaced
setup.py/setup.cfg+requirements*.txtwith PEP 621 metadata and extras inpyproject.toml(Hatchling backend). - Updated tox, Read the Docs config, and a CI workflow step to install dependencies via extras/uv instead of requirements files.
- Adjusted several cloud/instance method signatures to accept
**kwargsand tightened example type annotations for mypy.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Switches tox deps to install via project extras rather than requirements files; updates lint/typecheck targets. |
| test-requirements.txt | Removes legacy test requirements list (moved into pyproject.toml extras). |
| setup.py | Removes legacy setuptools entry point. |
| setup.cfg | Removes legacy package metadata and dependency declarations. |
| requirements.txt | Removes legacy “-e .” requirements indirection. |
| pyproject.toml | Adds PEP 621 project metadata, dependencies, optional extras, and tool configs; switches build backend to Hatchling. |
| pycloudlib/openstack/instance.py | Updates method signature to accept **kwargs for interface compatibility. |
| pycloudlib/oci/cloud.py | Adds **kwargs to selected APIs for signature compatibility. |
| pycloudlib/lxd/instance.py | Adds **kwargs to lifecycle methods; threads get_pty through _run_command. |
| pycloudlib/lxd/cloud.py | Adds **kwargs to snapshot signature for interface compatibility. |
| pycloudlib/gce/instance.py | Adds **kwargs to wait_for_delete signature for interface compatibility. |
| pycloudlib/ec2/cloud.py | Adds **kwargs to snapshot signature for interface compatibility. |
| examples/oracle/oracle-cluster-demo.py | Uses Optional[str] for nullable parameters to satisfy mypy. |
| examples/ibm_classic.py | Uses Optional[str] for nullable parameters to satisfy mypy. |
| docs/requirements.txt | Removes legacy docs requirements list (moved into pyproject.toml extras). |
| ci-requirements.txt | Removes legacy CI requirements list. |
| .readthedocs.yaml | Switches RTD install method to uv and uses the docs extra. |
| .github/workflows/version_check.yaml | Installs uv and uses it to install packaging for version checking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5eaab84 to
cef1487
Compare
|
@blackboxsw if you have a moment |
cef1487 to
5260eb3
Compare
|
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thank you @rpocase for this proposal this looks like a good direction to me. Let's iron out a few of these concerns before moving forward please.
-
I think we are missing changes to
docs/Makefilerelated topip install -rrequirements.txtwhich no longer exists.cd docs; make depsfails.2. I'd like to use your separate commit messages as the produce more useful context for each commit, but the uv.lock file is introduced in the first commit and removed in the third commit. Can we drop that delta from both first as last commits -
I left inline comments about changes in the supported floor version of dependencies, can we ensure we aren't getting a lower/older supported minimum version than we already had in main?
-
inline comment about using github issues instead of a launchpad project that doesn't exist.
-
I'm seeing with CI that is erroring at the moment with this uv branch on 'new prs' is the following lint errors:```
pycloudlib/azure/instance.py:288: error: Unused "type: ignore" comment [unused-ignore]
pycloudlib/azure/instance.py:311: error: Unused "type: ignore" comment [unused-ignore]
pycloudlib/azure/instance.py:362: error: Unused "type: ignore" comment [unused-ignore]
Note: I can confirm though both integration tests in ubuntu-pro-client and in cloud-init that this changeset doesn't introduce issues with CICD there.
Also, the general dep installation improvement time in cold/empty cache is a significant improvement:
- uv-based cold : ~13.5 seconds
- uv-based warm: ~1.5 seconds
- pip-based cold: ~64 seconds
- pip-based warm: ~43 seconds
I like the improvements this represents for the project. thank you.
| "types-mock>=5.0.0", | ||
| "types-pyyaml>=6.0.12", |
There was a problem hiding this comment.
Now that we are unpinning specific versions, we should watch pycloudlib CI more often to ensure we aren't getting affected by changes in remote unpinnned dependencies. If this becomes a problem we can attempt to place a version ceiling to avoid external breaking changes where possible. I don't think we need a change to this PR, just an agreement to look at adding dependency ceilings if we start seeing breakage due to upstream external dependency changes.
25b0dfe to
dce86a2
Compare
| def _run_command(self, command, stdin, get_pty=False): | ||
| """Run command in the instance.""" | ||
| if self.execute_via_ssh: | ||
| return super()._run_command(command, stdin) | ||
| return super()._run_command(command, stdin, get_pty=get_pty) | ||
|
|
||
| if self.series == "xenial": | ||
| self._log.warning(MISSING_AGENT_MSG, "lxc exec") | ||
|
|
||
| return super()._run_command(command, stdin) | ||
| return super()._run_command(command, stdin, get_pty=get_pty) |
There was a problem hiding this comment.
@blackboxsw how do you want to handle this? this is another case where the superclass explicitly supports it and the subclass has broken the subclass contract.
There was a problem hiding this comment.
@rpocase Sorry, I'm not getting what you mean here. The superclass explicitly supports exactly this same call signature with get_pty param. How is the subclass breaking that contract? I must be misreading something.
There was a problem hiding this comment.
the superclass supported the keyword argument, but the subclass didn't expose the argument. i wasn't sure if this was intentional or not.
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
After testing with both ubuntu-pro-client and cloud-init integration tests I feel comfortable that this tooling update isn't going to negatively impact CI in either case.
Also integration test runs locally appear to work fine for new PRs in pycloudlib with these new workflow changes.
While generally I wouldn't want to replace tooling unless dependency feature deprecations are imminent or projects are becoming abandoned I do see an easy path toward other tooling integration with workshop as @rpocase mentioned (as well as faster dep install time uv vs pip). Given that pycloudlib import as set is being used so broadly by CPC, cloud-init and ubuntu-pro-client CICD, that time/setup saving adds up fairly quickly.
will follow up on get_ply and lxd
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Additionally I think we need to:
- Adapt the
depstarget from docs/Makefile to avoid pip references to requirements.txt which is now deleted. - drop *requirements.txt from MANIFEST.in
| def _run_command(self, command, stdin, get_pty=False): | ||
| """Run command in the instance.""" | ||
| if self.execute_via_ssh: | ||
| return super()._run_command(command, stdin) | ||
| return super()._run_command(command, stdin, get_pty=get_pty) | ||
|
|
||
| if self.series == "xenial": | ||
| self._log.warning(MISSING_AGENT_MSG, "lxc exec") | ||
|
|
||
| return super()._run_command(command, stdin) | ||
| return super()._run_command(command, stdin, get_pty=get_pty) |
There was a problem hiding this comment.
@rpocase Sorry, I'm not getting what you mean here. The superclass explicitly supports exactly this same call signature with get_pty param. How is the subclass breaking that contract? I must be misreading something.
the current setup is unexpected given the test runner is using uv. this migrates the setup to uv. mypy and some typed dependencies needed to be upgrade due to messaging around "typed-ast" not being installed with seemingly no workaround.
- Add [tool.uv] python-preference = "only-system" to pyproject.toml so uv never downloads a managed Python and always uses the system interpreter - Add tox-uv to tox requires so tox uses uv as its venv/install backend - Replace apt tox installs with astral-sh/setup-uv in all workflows - Collapse separate py310/py312 CI jobs into a single matrix job that selects the Ubuntu runner (22.04/24.04) based on the target Python version, making the Python→runner mapping explicit and easy to extend - Simplify version_check workflow: drop actions/setup-python and pip installs in favour of uv run --with packaging
the migration from launchpad to github did not correct the issue tracker link.
migrating to mypy 2.x resulted in a larger amount of type failures being detected. this resolves all outstanding type hinting failures
this converts all tag based action checkouts to the latest corresponding shasum.
dce86a2 to
14ec0c1
Compare
|
@blackboxsw latest push addresses the commit pins and ensures that the recent azure-network-mgmt pin stays in place |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks @rpocase looks good with two exceptions:
- pinning correction on azure-mgmt-network
- I don't think we want to commit
uv.lockto this project repo do we? I may be mistaken, but thought when you have a library you are building, you may not want to provide uv.lock because that library may be used in other environments which may be created in other releases of Ubuntu where the python versions of dependencies may differ from our upstream repo. Again, I may be mistaken as I haven't used uv too often with distributed libraries.
If you think we need to keep uv.lock, how do you envision cloud-init, cpc tests, ubuntu pro attempting to run those project test suites. do they perform a uv sync to get the same uv.lock version from pycloudlib prior to running tests?
The pinning suggestion doesn't matter for too long as I just put up #516 which may help drop this pin in favor of the new Azure SDK model-based hybrid requests. I'll rebase 516 after this PR lands to fix the pinning minor version and avoid delaying your work any longer.
| "azure-cli-core >= 2.21.0", | ||
| "azure-identity", | ||
| "azure-mgmt-compute >= 17", | ||
| "azure-mgmt-network <= 31", |
There was a problem hiding this comment.
This should be >= 16, < 31 not <= 31.
| "azure-mgmt-network <= 31", | |
| "azure-mgmt-network >=16, < 31", |
PR Checklist
To ease the process of reviewing your PR, do make sure to complete the following checklist before submitting a pull
request.
tox -e formatlocally to automatically format my code before submittingtoxlocally ensuring that it passes before submittingOtherwise, please leave the PR as a draft to indicate that it is still a work in progress.
Description
this change migrates dependencies to be properly managed by uv instead of specified in requirements files,
upgrades mypy, and fixes newly found type hinting issues. this is in preparation for upgrading
openstack-sdk as netiface is orphaned and should be migrated to netiface2
Additional Context and Relevant Issues
this was triggered in part because running mypy was failing because of a missing typed-ast package.
at the same time, i've upgraded mypy and resolved all new type hinting issues.
Test Steps
N/A? i think integration tests should be run, but i don't have a proper setup to facilitate this