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

GH-43845: [Docs] Update Python Development Build Instructions #43617

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Aug 8, 2024

Rationale for this change

Following along the documentation as is will yield the errors described in these issues:

#42006 (comment)
#41992 (comment)

What changes are included in this PR?

The documentation now uses the pip front end, rather than invoking setuptools directly

Are these changes tested?

Locally

Are there any user-facing changes?

Yes...unfortunately when following these instructions, users will now see:

DEPRECATION: --build-option and --global-option are deprecated. pip 24.2 will enforce this behaviour change. A possible replacement is to use --config-settings. Discussion can be found at https://github.com/pypa/pip/issues/11859
WARNING: Implying --no-binary=:all: due to the presence of --build-option / --global-option. 

The pyarrow setup script seems to need --build-type and --bundle-arrow-cpp as global options. Unfortunately, there is a disconnect between what pip offers and what setuptools can handle, so the only current way to pass these options to setuptools is through the front-end that pip has deprecated. A larger discussion and recap of the issue can be seen here:

pypa/pip#11859 (comment)

@WillAyd
Copy link
Contributor Author

WillAyd commented Aug 8, 2024

@jorisvandenbossche @assignUser if you have a chance to look - might be something easy I am missing

@WillAyd WillAyd force-pushed the fix-python-docs branch 2 times, most recently from 5cbc0d8 to 4965f74 Compare August 8, 2024 18:57
@kou kou changed the title MINOR: Update Python Development Build Instructions MINOR: [Docs] Update Python Development Build Instructions Aug 9, 2024
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. Updating those instructions was long overdue ..

@@ -398,7 +398,7 @@ Now, build pyarrow:

$ pushd arrow/python
$ export PYARROW_PARALLEL=4
$ python setup.py build_ext --inplace
$ python -m pip install -e .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ python -m pip install -e .
$ python -m pip install -e . --no-build-isolation --no-deps -v

Those additional flags make it more similar to what we did before. It's probably debatable whether those should be added (disabling build isolation and installing deps), but since in this tutorial we have set up all build dependencies in advance, not doing that gives unnecessary pip overhead.
(this is also the command that I use locally, although for quick rebuilds I still use python setup.py build_ext --inplace)

.. note::
To install an editable PyArrow build run ``pip install -e . --no-build-isolation``
in the ``arrow/python`` directory.
$ python -m pip wheel . \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ python -m pip wheel . \
$ python -m build . \

Using https://build.pypa.io/en/stable/ might be the recommended way nowadays to build a wheel?

When using that, it might need --config-setting instead of --global-option (but haven't verified this works the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using that, it might need --config-setting instead of --global-option (but haven't verified this works the same)

I believe the --global-option is a requirement as long as setuptools is the backend. It's extremely murky, but here's the best conversation I could find on that topic:

pypa/pip#11859 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we need some way to pass down the option, but what I meant is that when using python -m build instead of python -m pip wheel, the name of the keyword to pass options might be different.

Because in the docs of build, I only see --config-setting and not --global-option

@@ -548,7 +545,7 @@ Now, we can build pyarrow:

$ pushd arrow\python
$ set CONDA_DLL_SEARCH_MODIFICATION_ENABLE=1
$ python setup.py build_ext --inplace
$ python pip install -e .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ python pip install -e .
$ python pip install -e . --no-build-isolation --no-deps -v

(depending on what is done above)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 20, 2024
@jorisvandenbossche jorisvandenbossche changed the title MINOR: [Docs] Update Python Development Build Instructions GH-43845: [Docs] Update Python Development Build Instructions Aug 27, 2024
Copy link

⚠️ GitHub issue #43845 has been automatically assigned in GitHub to PR creator.

@jorisvandenbossche
Copy link
Member

(opened an issue for this, because it's not actually a minor change anymore)

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

Successfully merging this pull request may close these issues.

2 participants