Skip to content

migration from poetry to uv #8249

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 13 commits into from
Aug 11, 2025

Conversation

king-11
Copy link
Contributor

@king-11 king-11 commented Apr 27, 2025

make use of standard keys for project and dependeny specification

  • provide sources to run uv build so that it can refer local packages
  • using hatchling for build as is stock build option
  • use optional-dependencies.dev for dev-dependencies
  • add hatch targets for packages and includes where unclear
  • update Makefiles to use uv when running commands that access python dependencies

Closes #7665

Important

25.05 FREEZE MAY 12TH: Non-bugfix PRs not ready by this date will wait for 25.08.

RC1 is scheduled on May 23rd, RC2 on May 26th, ...

The final release is on MAY 29TH.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@king-11 king-11 requested a review from cdecker as a code owner April 27, 2025 05:49
@king-11 king-11 marked this pull request as draft April 27, 2025 05:49
@king-11 king-11 changed the title refactor: pyproject.toml poetry to uv + hatch migration from poetry to uv Apr 27, 2025
@king-11 king-11 force-pushed the king-11/migrate-uv branch from a75084e to d041488 Compare April 27, 2025 05:51
@king-11
Copy link
Contributor Author

king-11 commented Apr 27, 2025

pending changes:

  • ci.yaml
  • macos.yaml
  • setup.sh

@vincenzopalazzo
Copy link
Collaborator

What are the benefit of uv over poetry?

@cdecker
Copy link
Member

cdecker commented Apr 27, 2025

It's much faster (5-10 seconds compared to minutes using poetry). While that doesn't sound too different, it means that we don't create long lived venvs that may drift from the metadata, causing fewer "it worked on my machine" kind of errors. It can also be used as a shebang, automatically wrapping for example a plugin, downloading dependencies on first invocation, creating the venv and only then passing control to python. This means we can make plugins self-contained and single file if we wanted to.

The speed, which unlocks all of these use cases, is done via caching, and interesting speedup ideas, that are quite fun to read up on. And finally, you don't need a system-wide python installation, since uv can also download and manage python versions (prevents you from accidentally polluting the system python and potentially break your OS).

@cdecker
Copy link
Member

cdecker commented Apr 27, 2025

Great initiative @king-11 , I had a branch with some initial steps, but you are much further along, so I'd rather help you than double-tracking

@king-11
Copy link
Contributor Author

king-11 commented Apr 27, 2025

Thanks @cdecker i will get the last few things in, just raised a draft PR given I saw some interest around it and wanted to have a pre-ready change look available.

@king-11
Copy link
Contributor Author

king-11 commented May 2, 2025

@cdecker do you have any suggestions I am currently encountering an issue where running tests shows pyln path not found.

After these changes I am setting things up as

uv venv --python 3.12
source .venv/bin/activate
uv sync --all-extras
uv run pytest tests/test_renepay.py

I ran the debug command

uv pip list

it shows pyln-testing is installed the only hunch I have here is maybe I need to set that as editable 🤔

@cdecker
Copy link
Member

cdecker commented May 3, 2025

Yeah, workspace-related options take a bit of getting used to.

For packages that we'd like uv to pull in from the repo, rather than the PyPI repository, we need to tell uv about the dependency:

[project]
...
dependencies = [
  "pyln-testing",
  ...
]

Then we need to tell uv that there is a member gl-testing in the workspace:

[tool.uv.workspace]
members = [
  "pyln-testing",
  ...
]

And then finally we need to tell uv to prefer the local copy from the repo, rather than the PyPI repo:

[tool.uv.sources]
pyln-testing = { workspace = true }

At least these are the three steps that would work for me whenever I ran into this issue.

@king-11
Copy link
Contributor Author

king-11 commented May 3, 2025

Got it thanks

@king-11 king-11 force-pushed the king-11/migrate-uv branch 2 times, most recently from 6e9cab3 to 3679ba4 Compare May 7, 2025 04:10
@king-11
Copy link
Contributor Author

king-11 commented May 7, 2025

Thanks for the workspace tip @cdecker that helped but one more thing was needed that is __init__.py files with PATH override for namespace sharing of pyln took some help from our ai friends going to get working on ci now I think

@king-11 king-11 force-pushed the king-11/migrate-uv branch 4 times, most recently from d1167b9 to 3fd0f8b Compare May 8, 2025 03:07
@king-11 king-11 marked this pull request as ready for review May 8, 2025 03:35
@king-11 king-11 force-pushed the king-11/migrate-uv branch from 5c0d170 to 888ff00 Compare May 8, 2025 03:35
@king-11
Copy link
Contributor Author

king-11 commented May 8, 2025

The flake8 fails for auto generated files not sure why but I have ran locally and tests are working fine now. Can you use some guidance on how to resolve the flake9 issue

@king-11 king-11 marked this pull request as draft May 8, 2025 03:39
@king-11 king-11 force-pushed the king-11/migrate-uv branch 7 times, most recently from dd3239d to 9d67e52 Compare May 10, 2025 04:43
@king-11 king-11 force-pushed the king-11/migrate-uv branch from de6266b to d8d4e6c Compare June 19, 2025 02:52
@king-11
Copy link
Contributor Author

king-11 commented Jun 19, 2025

@cdecker can you review now I have some changes in local for reckless they are around the case that we shouldn't create an additional venv while running reckless from one that should fix the remaining test failures. cc: @endothermicdev for reckless change recommendation

I have updated the readmes as well.

@king-11 king-11 force-pushed the king-11/migrate-uv branch from d8d4e6c to d4598e7 Compare June 21, 2025 01:33
@king-11 king-11 force-pushed the king-11/migrate-uv branch from 9435662 to caa004d Compare June 28, 2025 06:05
@cdecker
Copy link
Member

cdecker commented Jun 28, 2025

Please remove commit cf8588c. The whole idea of reckless managing venvs is to guarantee isolation. By reusing a random venv that is inherited from outside, we may end up polluting a user's venv, and multiple plugins with different dependencies will clobber each other, causing hard-to-debug failures.

The overhead, and additional disk space is intentional, because it buys us isolation, that we cannot get otherwise./

@king-11
Copy link
Contributor Author

king-11 commented Jun 28, 2025

By reusing a random venv that is inherited from outside, we may end up polluting a user's venv, and multiple plugins with different dependencies will clobber each other, causing hard-to-debug failures.

Got it now am more clear on the intent the issue is if there is an existing venv we don't know with what name the folder might be it can be .venv or something else so will have to add some sort of randomisation while creating venv with pip. Poetry should be fine as they use random location.

@king-11 king-11 force-pushed the king-11/migrate-uv branch 2 times, most recently from 9971be9 to 9de3de4 Compare August 3, 2025 02:12
king-11 added 11 commits August 3, 2025 08:01
make use of standard keys for project and dependeny specification

- provide sources to run uv build so that it can refer local packages
- using hatchling for build as is stock build option
- use optional-dependencies.dev for dev-dependencies
- add hatch targets for packages and includes where unclear

Changelog-Update: use uv with hatchling instead of poetry
allows for namespace sharing in a virtual environment otherwise gets overriden by pyln-proto-grpc's pyln folder
Add pytest.ini with pythonpath = . configuration to ensure modules 
are properly discovered when running tests. Also add empty 
__init__.py file to make the tests directory a proper Python package.
Replace poetry with uv for managing Python dependencies and running
commands across CI workflow.

- Add astral-sh/setup-uv@v5 action to install uv
- Replace all poetry run commands with uv run
- Remove poetry-specific installation steps
- Update Python setup in multiple jobs
add openssl installation and flags to install grpcio-tools
Remove unnecessary global declarations across multiple test files. 
This change improves code quality by eliminating redundant global 
statements for variables that are already accessible in their 
respective scopes. Add proper type annotation for fees_from_status 
in test_closing.py and import the required typing modules. These 
changes maintain the same functionality while making the code cleaner 
and more compliant with Python best practices.
Extract package versions from pyproject.toml directly
instead of using poetry commands. Use `uv run` to execute flake8,
pytest and other Python tools consistently.

Add new make commands for uv builds
add documentation for release pipeline changes
update commands and usage for uv
missing libffi fails coincurve installation
@king-11 king-11 force-pushed the king-11/migrate-uv branch from 9de3de4 to 81648a7 Compare August 3, 2025 02:31
Updates tests/rkls_github_canned_server.py to use markupsafe.escape 
instead of the deprecated flask.escape function. The flask.escape 
function is now deprecated and will be removed in a future Flask 
release, so this change ensures forward compatibility.
@king-11 king-11 force-pushed the king-11/migrate-uv branch from 81648a7 to 2d49a4d Compare August 3, 2025 15:57
@king-11
Copy link
Contributor Author

king-11 commented Aug 3, 2025

@cdecker I think this is good now there was this one sed change I made in PR for mac compatibility that was messing the tests everywhere but removed the reckless change wasn't required it doesn't conflict with uv venv as the directory is different so we are good I think

hatchling build requires license file but the build that coincurve 
21.0.0 uses doesn’t account for that in `build_hatch.py` this was added 
to prevent need of `cffi` as a runtime dependency but we can probably 
live without it until it gets fixed.

change MR: ofek/coincurve#176
 
reported issue: ofek/coincurve#187
@rustyrussell rustyrussell added this to the v25.09 milestone Aug 8, 2025
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 873cd0e

While ambitious, now is the time to put this in. I will be delighted to leave poetry behind us, and I like some of the other fixes in here too.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

@king-11 this is an amazing PR, thank you so much for purging poetry from the project (it should likely stay in reckless since we'd like to allow downstream devs to chose their own tooling). Also great work on hunting down those path issues and the cryptography/LICENSE issue 👏

Just some minor clarifications, and minor nits that can be addressed in a separate PR. This is gtg for me 🚀

@@ -0,0 +1,2 @@
# pyln is a namespace package
__path__ = __import__('pkgutil').extend_path(__path__, __name__)
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what this __init__.py file does? It seems like it changes its own import path somehow? And if the problem is pyln-grpc-proto why aren't we just fixing that?

Copy link
Contributor Author

@king-11 king-11 Aug 8, 2025

Choose a reason for hiding this comment

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

It allows for packages to create pyln.proto kind of packages by extending the parent pyln. Otherwise only one of them i.e. pyln-grpc-proto overwrites the pyln path

@@ -0,0 +1,2 @@
[pytest]
pythonpath = .
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that only the pytest.ini in the repo root is being loaded, and this should have no effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 there is no pytest.ini in repo root though

@@ -71,16 +71,14 @@ jobs:
- name: Set up Python 3.10
uses: actions/setup-python@v5
with:
python-version: '3.10'
python-version: "3.10"
Copy link
Member

Choose a reason for hiding this comment

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

Do we even still use the system python? I think uv will pull down a standalone python binary, completely ignoring the system one.

Copy link
Contributor Author

@king-11 king-11 Aug 8, 2025

Choose a reason for hiding this comment

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

I think uv will only do it if we explicitly ask it to create python venv with a different python version that what is available in system

@madelinevibes madelinevibes merged commit 0ca833f into ElementsProject:master Aug 11, 2025
75 of 78 checks passed
@king-11 king-11 deleted the king-11/migrate-uv branch August 12, 2025 02:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python dependencies are missing in release binaries
5 participants