Skip to content
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

Merged
merged 39 commits into from
Sep 17, 2024

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Jun 30, 2024

"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:

  • Update docs

@mfisher87 mfisher87 force-pushed the poetry-groups-to-pep621-optional-deps branch 3 times, most recently from 439cb98 to faa4571 Compare June 30, 2024 04:50
- repo: https://github.com/python-poetry/poetry
rev: '1.8.3'
hooks:
- id: poetry-check
Copy link
Collaborator Author

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.

@mfisher87 mfisher87 marked this pull request as ready for review June 30, 2024 14:09
@itcarroll
Copy link
Collaborator

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.

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. xarray[io] if you want netCDF4, etc.). Hatch (and Poetry) allow separate ways of providing packages for and setting up development environments; information not included in distribution metadata. I haven't been compelled to duplicate package lists for simple cases with Hatch. Note that the current potential downside with Hatch is a lack of lock files.

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Jun 30, 2024

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.

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Jun 30, 2024

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 😆

@mfisher87
Copy link
Collaborator Author

Found it: https://peps.python.org/pep-0735/

@mfisher87 mfisher87 mentioned this pull request Jul 6, 2024
chuckwondo
chuckwondo previously approved these changes Sep 3, 2024
Copy link
Collaborator

@chuckwondo chuckwondo left a 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?

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Sep 3, 2024

Thanks, Chuck! Good point, I do imagine I missed some stuff there.

TODO:

  • Ensure docs are up-to-date with dependency management changes

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
@mfisher87
Copy link
Collaborator Author

mfisher87 commented Sep 4, 2024

@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!

@danielfromearth
Copy link
Collaborator

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?

pyproject.toml Outdated Show resolved Hide resolved
Comment on lines +40 to +41
python -m venv .venv
source .venv/bin/activate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason this isn't exposed as a nox command?

Seems like pushing as much into nox as possible would further simplify things, and even make it easier to change things in the future (if desired) without requiring a contributor to do anything differently (i.e., I see "nox" as the interface and the stuff in the noxfile as the "implementation", such that "nox" encapsulates things, allowing use to change the implementation as necessary, without affecting the nox user).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't done or seen that done before -- I assumed that environment activation would not be possible to do within a nox command because the command doesn't execute in the current shell?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, yes, I was looking only at the venv creation. Activation, of course, must be done explicitly by the user, so that's one bit that "leaks" through to the user, which cannot be encapsulated/hidden by nox (or make).

I'm suggesting we put as much as possible behind nox, so that as much as possible, the user uses nox as the one way of doing things.

Copy link
Collaborator Author

@mfisher87 mfisher87 Sep 17, 2024

Choose a reason for hiding this comment

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

I don't know if I agree that we should position nox as the "one way" of doing things. I definitely think it's valuable as a quickstart to common tasks, though. E.g.: "I don't know how to install a dev environment but I would be embarassed if I pushed my simple error message change and it didn't pass tests".

That's getting in to the philosophy of what task runners are for and not for, so let's take that on separately.


```bash
poetry run jupyter lab
pip install --editable .[dev,test,docs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also expose this as a nox command instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure! What would this look like? It doesn't really map to how I normally use and think about nox. In my mental model I create "sessions" as independent and stateless things, e.g. running typechecking nox creates and can cache an environment with mypy and all the runtime dependencies, and for linting, nox can depend on an environment that solely contains ruff.

Maybe this is more common than I think, but using nox to do setup just doesn't seem natural to me :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I consider nox analogous to make, both of which I consider being able to wrap "complex" commands within simpler commands. So, rather than having to type the pip command above, someone might instead be able to type nox install and be done with it, without fat-fingering arguments.

To take this further, nox (and make et al.) can eliminate duplication. Another suggestion I have is to modify the github workflows to also use nox rather than the "bare" commands. This way, both a contributor and github are using the same commands (both using nox). Granted, a contributor might not run all of the things that github would, but by having both use nox (in our case), there's no need to update docs or workflows to match changes to the noxfile (for existing commands), thus eliminating (or greatly reducing) the likelihood of docs and files getting out of sync.

However, to expedite landing this PR, we can open a separate issue to address this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this should move into a separate PR. How we think about tasks runners, philosophically, is complicated :) This page in the scientific python library development guide provides some suggestions: https://learn.scientific-python.org/development/guides/tasks/#task-runners

Copy link
Collaborator

@itcarroll itcarroll left a comment

Choose a reason for hiding this comment

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

Fully approve, with a couple minor comments.

I haven't yet groked when the uv lock file is created or what knows to use it (i would probably have to dive into nox or uv or maybe hatchling docs to understand), but the great thing is I don't have to know because it's on autopilot now.

{ version = ">=3.6.1", python = ">=3.12" }
dev = [
"bump-my-version >=0.10.0",
"nox",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we both have nox included here and instruct contributors to install nox with pipx (or brew, etc.)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think? df578ed

Makefile Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better also remove the make test step from docs/contributing/index.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NICE FIND! How did I miss that!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

chuckwondo
chuckwondo previously approved these changes Sep 17, 2024
chuckwondo
chuckwondo previously approved these changes Sep 17, 2024
Co-Authored-By: Joseph H Kennedy <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Copy link
Collaborator

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

🎉 🎉 Looks great! 🎉 🎉

:shipit:

@mfisher87 mfisher87 merged commit 028b8fa into main Sep 17, 2024
11 of 12 checks passed
@mfisher87 mfisher87 deleted the poetry-groups-to-pep621-optional-deps branch September 17, 2024 17:54
@mfisher87
Copy link
Collaborator Author

Thanks all for your help! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants