-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: master
Are you sure you want to change the base?
Conversation
6c32e93
to
cb3d1c1
Compare
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.
Awesome start on this! I really appreciate your active involvement. My time is too unpredictable to spend significant dedicated time to this project.
pyproject.toml
Outdated
common = [ | ||
"pytest<7.0", |
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 have 2 preferences here related to sorting:
- keep package names in alphabetic order
- 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.
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.
fixed in 9616953
pyproject.toml
Outdated
[project.optional-dependencies] | ||
jsonschema = ["jsonschema"] | ||
prometheus = ["prometheus-client>=0.5,<0.15"] | ||
toml = ["toml<2.0.0"] | ||
|
||
[dependency-groups] | ||
dropbox = [ |
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 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.
requires = | ||
tox>=4.22 # `dependency_groups` needed | ||
tox-uv>=1.19 |
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.
Could you help me understand why these requirements are here and not in pyproject?
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.
In general case, tox
version is not managed in pyproject.toml, requirements.txt, etc.
https://github.com/tox-dev/tox-uv/blob/main/.github/workflows/check.yaml#L40
https://github.com/tox-dev/tox-uv/blob/main/tox.ini#L2-L4
I think this is because tox
is not related to actual implementation of tests.
In our CI, we use dependency_groups
feature of pyproject.toml and tox, so I added this requirements on tox's configuration.
@@ -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 |
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.
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.
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.
@hirosassa , how you thought how we can be sure that we don't break RTD when merging this PR?
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 think it is a little bit difficult because it depends on webhook of this repo.
I confirmed that a local build of documents using Sphinx is succeeded.
2ec1144
to
844d94d
Compare
@dlstadther I confirmed CIs are all green without Python 3.7. With Python 3.7, test fails in code below: I think the root cause of this problem is Python 3.7.9 runtime. The reason why the CI in this PR for Python 3.7 run in 3.7.9 is We might need to consider dropping support for Python 3.7. |
With 3.7 already EOL for awhile I think it's fine dropping support. |
Looking at https://pypistats.org/packages/luigi , there are still 3.7 downloads, but fewer than 3.8+. I'm in favor of removing 3.7 support |
@dlstadther @RRap0so Thanks for your comment! |
9d5181b
to
99ac166
Compare
99ac166
to
cd6fee3
Compare
@dlstadther This PR is ready for review! |
We probably need to drop the checks for 3.7 but I'm wondering why there's 3.8,3.9 even 3.10 checks not being reported? @hirosassa can you take a look at the changes? Something is up, some checks haven't ran. I can remove the 3.7 checks once this is good to go. |
@RRap0so Thank you for your 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.
A few comments/questions.
Also, we'll need to have the merge approval settings updated by @RRap0so .
@@ -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 |
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.
@hirosassa , how you thought how we can be sure that we don't break RTD when merging this PR?
@dlstadther ping? |
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 think we're good to go here, @hirosassa . While i'm unsure what the behavior of the RTD will be since it failed a few years ago, but everything i can find related to RTD suggest they're on recent ubuntu now (which should support luigi installation just fine).
Lastly, we will need @RRap0so 's help to update the required CI checks to allow merge.
@RRap0so Could you check this comment? |
Description
In this PR, I migrated from
setup.py
topyproject.toml
usinguv
.Fixes #3327.
In detail I did followings:
setup.py
features to equivalentpyproject.toml
uv
as a package managertox.ini
topyproject.toml
tox
version to use new features oftox
andpyproject.toml
pyproject.toml
#3338 (comment)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.