-
Notifications
You must be signed in to change notification settings - Fork 392
Add support for UV dependency management #2601
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
Fokko
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.
This looks great @geruh, I hear a lot of good stuff around uv 🙌
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 working on this!
I tested a few things locally.
- all the
makecommand - side by side pyproject.toml comparison
- ran a few github workflows on my forked repo
python-ci.yml✅python-ci-docs.yml✅nightly-pypi-build.yml❌ (something weird with cibuildwheel)
We might want to rebase this PR to capture of of the recent changes, such as the 3.9 deprecation. I'm not too worried about the library versioning since the dependabot workflow will upgrade them again
btw, there are a still references to poetry in mkdocs/docs/contributing.md
|
i cant seem to get |
kevinjqliu
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.
generally lgtm.
i tested the github workflow changes on my fork
.github/workflows/nightly-pypi-build.yml
.github/workflows/python-release.yml
.github/workflows/pypi-build-artifacts.yml
.github/workflows/svn-build-artifacts.yml
.github/workflows/python-ci.yml
.github/workflows/python-ci-docs.yml
.github/workflows/python-release-docs.yml
kevinjqliu
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.
I checked all the github workflows, they all run as expected.
I also did a sanity check by comparing the new artifacts (sdist/wheel) against the nightly artifacts thats built with poetry. Noticed some differences
This looks pretty good! Thanks!!
Co-authored-by: Kevin Liu <[email protected]>
kevinjqliu
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.
LGTM
- Check Github Action (https://github.com/kevinjqliu/iceberg-python/actions/runs/19253283756, rest is already checked in CI)
- Check artifacts (sdist and wheel) built with fork above
- with sdist, ran RAT check and release process
- with wheel, install locally and verified cpython avro reader (CythonBinaryDecoder) is available
- Check all Makefile commands
- Check library versions poetry.lock vs uv.lock
Diff sdist and wheel between this branch and nightly from testpypi
sdist
- All the changes are as expected
- Newly added files are from setuptools, we can try to remove as a followup
- Removed files are as expected
> diff -qr ~/Downloads/branch/sdist ~/Downloads/nightly/sdist/ | sort
differ
Files /Users/kevinliu/Downloads/branch/sdist/Makefile and /Users/kevinliu/Downloads/nightly/sdist/Makefile differ
Files /Users/kevinliu/Downloads/branch/sdist/PKG-INFO and /Users/kevinliu/Downloads/nightly/sdist/PKG-INFO differ
Files /Users/kevinliu/Downloads/branch/sdist/dev/.rat-excludes and /Users/kevinliu/Downloads/nightly/sdist/dev/.rat-excludes differ
Files /Users/kevinliu/Downloads/branch/sdist/pyproject.toml and /Users/kevinliu/Downloads/nightly/sdist/pyproject.toml differ
new
Only in /Users/kevinliu/Downloads/branch/sdist: MANIFEST.in
Only in /Users/kevinliu/Downloads/branch/sdist: pyiceberg.egg-info
Only in /Users/kevinliu/Downloads/branch/sdist: setup.cfg
Only in /Users/kevinliu/Downloads/branch/sdist: setup.py
removed
Only in /Users/kevinliu/Downloads/nightly/sdist: build-module.py
Only in /Users/kevinliu/Downloads/nightly/sdist: poetry.lock
wheel
- Changed file is expected, for CythonBinaryDecoder
- Newly added file (
*dist-info) isnt new, just rename of the old dist-info - Removed files are as expected
- .c, .pyx not needed
- cpython-310, cpython-311 not needed, since this is the 3.12 wheel
- NOTICE file not needed, there's another one in dist-info
> diff -qr ~/Downloads/branch/wheel/ ~/Downloads/nightly/wheel/ | sort
differ
Files /Users/kevinliu/Downloads/branch/wheel/pyiceberg/avro/decoder_fast.cpython-312-darwin.so and /Users/kevinliu/Downloads/nightly/wheel/pyiceberg/avro/decoder_fast.cpython-312-darwin.so differ
new
Only in /Users/kevinliu/Downloads/branch/wheel: pyiceberg-0.10.0.dev20251111024930.dist-info
removed
Only in /Users/kevinliu/Downloads/nightly/wheel/pyiceberg/avro: decoder_basic.c
Only in /Users/kevinliu/Downloads/nightly/wheel/pyiceberg/avro: decoder_fast.cpython-310-darwin.so
Only in /Users/kevinliu/Downloads/nightly/wheel/pyiceberg/avro: decoder_fast.cpython-311-darwin.so
Only in /Users/kevinliu/Downloads/nightly/wheel/pyiceberg/avro: decoder_fast.pyx
Only in /Users/kevinliu/Downloads/nightly/wheel: NOTICE
Only in /Users/kevinliu/Downloads/nightly/wheel: pyiceberg-0.10.0.dev20251111003004.dist-info
|
Thanks for the PR @geruh, super exciting to start using uv. Thanks for the review @Fokko @rambleraptor |
# Rationale for this change Update contributing documentation to reflect the migration from Poetry to uv for dependency management in #2601. https://docs.astral.sh/uv/ ## Are these changes tested? `make lint && make docs-serve` <img width="625" height="600" alt="image" src="https://github.com/user-attachments/assets/5ff10875-ac2c-432e-b34d-11833968656d" /> ## Are there any user-facing changes? doc changes
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
Follow up to #2601
clean up .egg-info/ and setup.cfg from sdist
## Are these changes tested?
Yes
Check the new sdist
```
rm -rf dist build pyiceberg.egg-info
uv build --sdist
tar -tzf dist/pyiceberg-*.tar.gz | grep -E "(build/|setup.cfg|\.egg-info)"
```
Diffed too
```
diff -qr ./dist/pyiceberg-0.10.0 ~/Downloads/nightly/sdist/ | sort
differ
Files ./dist/pyiceberg-0.10.0/Makefile and /Users/kevinliu/Downloads/nightly/sdist/Makefile differ
Files ./dist/pyiceberg-0.10.0/PKG-INFO and /Users/kevinliu/Downloads/nightly/sdist/PKG-INFO differ
Files ./dist/pyiceberg-0.10.0/dev/.rat-excludes and /Users/kevinliu/Downloads/nightly/sdist/dev/.rat-excludes differ
Files ./dist/pyiceberg-0.10.0/pyproject.toml and /Users/kevinliu/Downloads/nightly/sdist/pyproject.toml differ
new
Only in ./dist/pyiceberg-0.10.0/tests: .DS_Store
Only in ./dist/pyiceberg-0.10.0: MANIFEST.in
Only in ./dist/pyiceberg-0.10.0: setup.py
removed
Only in /Users/kevinliu/Downloads/nightly/sdist: build-module.py
Only in /Users/kevinliu/Downloads/nightly/sdist: poetry.lock
```
In sdist, ran RAT check and tests
```
make check-license
make test-coverage
```
## Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label.
-->
No
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
Follow up to #2601
Apply `PYTHON_ARG` override to all `uv` commands`
## Are these changes tested?
Yes
Override to use python 3.13
```
export PYTHON=3.13
make setup-venv
make install
make docs-serve
```
Unset override and verify it uses system default, python 3.12
```
unset PYTHON
make setup-venv
make test
```
## Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label.
-->
No
Closes #2553
Rationale for this change
During the release process cleanup, I also had prototyped a migration from Poetry to UV. I used the https://github.com/mkniewallner/migrate-to-uv package to migrate to UV and ran a release workflow and some others like doc serve for validation.
This PR also migrates the Makefile to use UV. I'll still need to upgrade the docs to reflect the UV contribution workflow. I think it would be a good start to push up as a prototype, and have others checkout and give it a try.
Build System Changes
Build Backend
UV doesn't have a Poetry-equivalent build backend with build hooks, so I've switched to setuptools with native Cython support for the avro changes.
Packaging for release
PEP 639 License Compliance
By going to the test above the license is now properly included in the release metadata
Build Verification
Release builds verified:
Wheel vs Poetry:
The migration addresses a packaging issue where Poetry was building non-standard "fat" wheels. Binary comparison using diff revealed significant differences in wheel structure between Poetry and UV builds.
Poetry's wheels tagged for a specific Python version (e.g.,
cp312-cp312) contained compiled extensions for multiple Python versions simultaneously. The comparison shows that a Python 3.12 wheel includeddecoder_fast.cpython-310-darwin.soanddecoder_fast.cpython-311-darwin.soalongside the correct binary. These wheels also included source files and placed NOTICE at the wheel root instead ofdist-info/licenses/.The new build process with
cibuildwheelcreates standard PyPI-compliant wheels where each wheel contains only its corresponding Python version's binary. This reduces wheel size while ensuring proper PEP 639 license file placement indist-info/licenses/.Test builds are deployed also can be checked against the repo above.
Are these changes tested?
Yes
make installEnvironment setup and dependency installationmake test- Unit testsmake test-integration- Integration tests with all extrasmake lint- Linting and code quality checksmake doc-install && make doc-serve- DocumentationAre there any user-facing changes?
No user facing changes. Contributors will need to use UV instead of Poetry for development, but all make targets remain the same. So not really?