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

Migration to pyproject.toml #3338

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

hirosassa
Copy link
Contributor

@hirosassa hirosassa commented Jan 19, 2025

Description

In this PR, I migrated from setup.py to pyproject.toml using uv.
Fixes #3327.

In detail I did followings:

  • migrate setup.py features to equivalent pyproject.toml
  • introduce uv as a package manager
  • move dependency configurations for test from tox.ini to pyproject.toml
  • update tox version to use new features of tox and pyproject.toml

A part of codes are burrowed from @dlstadther 's branch.
ref master...dlstadther:luigi:migrate-setup-to-pyproject

Motivation and Context

python setup.py install is deprecated.

Have you tested this? If so, how?

CI worked.

@hirosassa hirosassa requested review from dlstadther and a team as code owners January 19, 2025 07:11
@hirosassa hirosassa changed the title Migration to uv Migration to pyproject.toml Jan 19, 2025
@hirosassa hirosassa marked this pull request as draft January 19, 2025 07:15
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Awesome start on this! I really appreciate your active involvement. My time is too unpredictable to spend significant dedicated time to this project.

@@ -2,7 +2,7 @@ For maintainers of Luigi, who have push access to pypi. Here's how you upload
Luigi to pypi.

#. Make sure [twine](https://pypi.org/project/twine/) is installed ``pip install twine``.
#. Update version number in `luigi/__meta__.py`.
#. Update version number in `luigi/__version__.py`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

When moving to uv, we can update the build and upload steps below to use uv build sdist and uv publish (rather than the setup.py and twine references).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Optionally, we can use uvx hatch version patch (or minor/major) to modify the project version, rather than manually updating the luigi/__version__.py file. Though, i think it's worth keeping the mention of where the version indicator lives.

Comment on lines +80 to +81
common = [
"pytest<7.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have 2 preferences here related to sorting:

  1. keep package names in alphabetic order
  2. keep group names in alphabetic order

This makes it easiest for humans to view and also to maintain order where new packages should be added.

Comment on lines +47 to +53
[project.optional-dependencies]
jsonschema = ["jsonschema"]
prometheus = ["prometheus-client>=0.5,<0.15"]
toml = ["toml<2.0.0"]

[dependency-groups]
dropbox = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

(This comment doesn't need to be addressed as part of this PR; mostly just thinking aloud)

Perhaps it makes sense in the future to move some of the dependency-groups content which is used for unit test execution for specific luigi modules (particularly the contrib and optional stuff) out of dependency-groups and into project.optional-dependencies to facilitate luigi installation with "extras". These extras can also easily control the cases where the luigi module may only support up to a max version.

Food for thought if it changes how you design/organize this file.

Comment on lines +2 to +4
requires =
tox>=4.22 # `dependency_groups` needed
tox-uv>=1.19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you help me understand why these requirements are here and not in pyproject?

@@ -67,7 +67,10 @@ def _warn_node(self, msg, node, *args, **kwargs):

sphinx.environment.BuildEnvironment.warn_node = _warn_node

if os.environ.get('READTHEDOCS', None) == 'True':
# on_rtd is whether we are on readthedocs.org, this line of code grabbed from docs.readthedocs.org
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before merging, we'll need to identify how to verify the merging of the final version of this branch doesn't break Luigi's readthedocs.

@hirosassa hirosassa force-pushed the migration-to-uv branch 7 times, most recently from 81d9cae to 1fef047 Compare January 25, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate setup.py to pyproject.toml
2 participants