-
Notifications
You must be signed in to change notification settings - Fork 76
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
Replace poetry
with pip
, build with hatchling
, dynamically calculate minimum dependencies with uv
#733
base: main
Are you sure you want to change the base?
Conversation
439cb98
to
faa4571
Compare
.pre-commit-config.yaml
Outdated
- repo: https://github.com/python-poetry/poetry | ||
rev: '1.8.3' | ||
hooks: | ||
- id: poetry-check |
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 will fail if the lockfile hasn't been updated, or if there's a typo in a package name in poetry config.
I have been exploring hatch w/ hatchling, and it is both PEP compliant and also (like Poetry) avoids using extras (a.k.a optional-dependencies) for managing development environments. Information about extras is built into the distribution metadata and primarily provides extra packages users might want (i.e. |
Just to clarify, I'm for including these optional dependency groups in the package metadata, as that's in my experience a very common method and is compatible with all environment / package managers, and as far as I know there is no standard way to specify dependency groups that do not go in the package metadata. I'd like to avoid pros and cons of environment / package managers here (i.e. Hatch, Poetry), as we can isolate that concern from our build tooling and configuration. Regardless of whether we decide down the road to document / encourage Hatch or Poetry or something else for package management, if our build configuration is PEP621 compliant, contributors can choose any tool. |
To make things more confusing, PDM has "dev dependencies", their own uniquely named version of Poetry's "dependency groups" and Hatch's "environment dependencies". Wow! The "same" thing three times with three names and three config keys. I'd really like to treat this Python environment management feature as non-standard and avoid it until there's a unified interface, which I'm sure there's already an open PEP for 😆 |
Found it: https://peps.python.org/pep-0735/ |
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.
@mfisher87, sorry this has remained unreviewed for so long! LGTM!
I'm approving, but do you need to update any "contributing" instructions regarding commands that a contributor needs to run to get their environment setup with dev/test deps?
Thanks, Chuck! Good point, I do imagine I missed some stuff there. TODO:
|
Poetry is required to use dependency groups. To make things compatible with other ways of dependency management, switch to PEP621 optional dependencies, but specified using the Poetry configuration aliases to retain support for poetry. TODO: Docs
As long as we're using it we should be validating! :)
Maybe we could use Nox instead of scripts at some point? Then the behaviors can be coupled to the correct environments.
These settings are already specified in pyproject.toml
9f18f69
to
24a5191
Compare
@chuckwondo @danielfromearth I committed the changes we were working on in the call (24a5191), and clearly they are no longer poetry compatible :) I didn't have time to debug yet! |
So, where do we want to go from here? If there's no way to remove the redundant dependency specifications, then get this working with another manager, like hatch, and then drop the poetry usage? |
hmm, hadn't seen It does look like the particular poetry PR for the |
While it would be nice to not have to do the work to rip out poetry given this change ☝️ I still think we should rip out poetry. I see poetry as purely a source of complexity. E.g. why do we need Anthony Sottile (pre-commit): why I will never use python-poetry |
This probably never worked? Kerchunk doesn't use calver.
- Replace Poetry - For dev, use `pip` & `venv` (users can of course use anything else that's standards-compliant, like `uv`) - For calculating minimum dependencies and reproducible locking, use `uv` - For build, use hatchling - Document usage of `pip` and `venv` in developer documentation. - Remove Makefile -- it's undocumented anyway! - Add Nox for task-running - Remove redundant scripts - Linting is run with pre-commit - Type checking is run with Nox - Update CI - Publish with PyPA official action
jobs: | ||
integration-tests: | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.9", "3.10", "3.11", "3.12"] | ||
python-version: ["3.9", "3.12"] |
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 felt it was a bit wasteful to run integration tests for every supported python version.
.github/workflows/test-mindeps.yml
Outdated
- name: Install | ||
run: python -m pip install --no-deps -e . | ||
- name: Install minimum-compatible dependencies | ||
run: uv sync --resolution lowest-direct |
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.
lowest-direct
matches what we were already doing with environment-mindeps.yml
. lowest
doesn't resolve successfully.
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.
There are things other than uv
that can do "lowest" resolution and lockfiles, I just reached for something I knew was fast and had single-file multi-platform lockfiles and wasn't poetry.
Makefile
Outdated
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 file wasn't documented or used anywhere.
If you don't have pipx (pip for applications), then you can install with | ||
`pip install pipx` (the only case were installing an application with regular | ||
pip is reasonable). If you use macOS, then pipx and nox are both in brew, use | ||
`brew install pipx nox`. |
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.
From Scientific Python project template's documentation
`pyproject.toml`. | ||
If you need to add a new dependency, edit `pyproject.toml` and insert the dependency in | ||
the correct location (either in the `dependencies` array or | ||
`[project.optional-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.
uv
and pixi
provide good add
user experiences, without the need to do the weird replacement of ^
s. If we want that UX, let's chat about which tool we want to endorse.
0e0549f
to
20073a2
Compare
20073a2
to
c0cf028
Compare
6fd31a8
to
9818907
Compare
poetry
with pip
, build with hatchling
, dynamically calculate minimum dependencies with uv
ci/environment-dev.yml
Outdated
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.
To me, it doesn't make sense for this to be in the ci
directory.
- repo: https://github.com/astral-sh/uv-pre-commit | ||
rev: "0.4.7" | ||
hooks: | ||
- id: uv-lock |
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.
Lockfile stays up-to-date without user needing to learn to use any new tooling
"Dependency groups" are a poetry-specific feature that's different from PEP621 "optional dependencies", aka "extras". To make things compatible with other ways of dependency management, switch to PEP621 optional dependencies, but specified using the Poetry configuration aliases to retain support for poetry. While the Poetry "dependency groups" functionality may be more aimed at development workflows, it's not compatible with other tools and in my experience the dominant convention in the ecosystem is to use optional dependencies. As @itcarroll points out below, Hatch supports a similar concept called "environment dependencies" and specifies them with a different tool-specific configuration key. PDM also calls it something different, "dev dependencies", and uses another unique config table. 😓
I really don't like the way poetry requires optional dependencies to be specified. It requires duplication.
Thoughts? I don't want to add all this duplication to our codebase. Can we go forward with replacing Poetry with good ol' setuptools or maybe hatchling for builds and switch the optional dependency groups to the standard format? Anything PEP621 compatible would be a step forward IMO. I can take that on as part of this PR. The PEP621 example shows our use case, test dependencies, implemented in the conventional way.
Closes #734
Closes #619
Closes #374
📚 Documentation preview 📚: https://earthaccess--733.org.readthedocs.build/en/733/
TODO: