-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
GH-37929: [Python] begin moving static settings to pyproject.toml #41041
base: main
Are you sure you want to change the base?
Conversation
|
I will try to investigate how other projects handle development versions with pyproject.toml! |
I found this! https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html#dynamic-metadata I'll get to work updating. |
006ce35
to
b0aa7e6
Compare
I am actively trying to figure out how to fix what's coming up in the build failures! |
b0aa7e6
to
f54d1ec
Compare
ab076e8
to
f9072a4
Compare
This is the current challenge for this PR. To migrate the static metadata to the pyproject.toml, we need to set a Setuptools_scm will only let you configure Callables in the setup.py. There are two callables we set, one for
As they currently are, we cannot configure these in the pyproject.toml, it will not accept a Python callable. The next part of the challenge is that if you move the version metadata to pyproject.toml, none of the configurations in I'm thinking that the next step is to contact the maintainers of setuptools_scm, and see if they have any advice. |
The setuptools_scm docs have an example of passing a callable in setup.py with using pyproject.toml: https://setuptools-scm.readthedocs.io/en/latest/customizing/#providing-project-local-version-schemes |
Also, it seems that we use |
I found open issues for the behaviour I am noticing: |
Locally, the test seems to be behaving decently:
The question is understanding why it is failing in CI. |
Note that we're not married to setuptools_scm. If we find out that something else would work better for us, then we can switch to it. Found this comparison using a quick search: jwodder/versioningit#46 (comment) |
|
9b0f5b4
to
b83e36a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g wheel |
Revision: 2c438b1 Submitted crossbow builds: ursacomputing/crossbow @ actions-5cafaa097c |
@github-actions crossbow submit example-python-minimal-build-* |
This comment was marked as outdated.
This comment was marked as outdated.
…lation Change pytest command
@github-actions crossbow submit example-python-minimal-build-* |
Revision: 410f0e0 Submitted crossbow builds: ursacomputing/crossbow @ actions-c55f6583b2
|
@github-actions crossbow submit -g python |
Revision: 960edf5 Submitted crossbow builds: ursacomputing/crossbow @ actions-5299b8a07f |
From my understanding the existing CI failures are unrelated:
This should be ready for a final review now @jorisvandenbossche @pitrou @anjakefala |
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.
Looks good!
FROM ubuntu:focal | ||
FROM ubuntu:jammy |
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.
Any specific reason for this change? (I don't care to be clear, but I don't know how our general coverage is of ubuntu versions, and we should probably still test somewhere with focal)
@@ -35,6 +35,7 @@ source $WORKDIR/venv/bin/activate | |||
git config --global --add safe.directory $ARROW_ROOT | |||
|
|||
pip install -r $ARROW_ROOT/python/requirements-build.txt | |||
pip install wheel |
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.
Is this addition still needed? (my understanding is that pip install
should not need it, since we also don't list it as a build dependency)
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.
Ah, I see we actually do list wheel
as a build requirement ;) But I think nowadays the recommendation is not list wheel
explicitly.
While I can't find an explicit reference for this, the original PEP text included wheel
in the simple example (https://peps.python.org/pep-0518/#build-system-table), but the examples for using setuptools in the packaging guide or on the setuptools documentation (https://setuptools.pypa.io/en/latest/userguide/quickstart.html#basic-use) no longer include wheel
.
So I think we should remove wheel
from build-system.requires
@@ -26,7 +26,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
{{ macros.github_set_env(env) }} | |||
steps: | |||
{{ macros.github_checkout_arrow(submodules=false)|indent }} | |||
{{ macros.github_checkout_arrow(fetch_depth=0, submodules=false)|indent }} |
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.
@raulcd I'm just curious! Why was this change needed?
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.
The default fetch depth for actions/checkout
is 1. That means it will checkout the specific commit and not fetch anything else, this is useful when dealing with large repos to speed up the checkout but it also leaves out all other branches and more importantly in this case tags (which are needed to calculate the dev version). 0 will fetch the full repo so all tags and branches are available.
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.
Yes, that's exactly the case. I realized while testing that the version was not generated correctly due to the missing tags, adding fetch_depth=0
pulls the tags and generates the correct version for pyarrow.
Rationale for this change
To migrate Arrow to modern Python packaging standards, see PEP-517 and PEP-518.
This PR focuses on migrating the static settings, the metadata and version, to pyproject.toml. Future PRs will migrate more of the build process to pyproject.toml.