[Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++

[jorisvandenbossche]

Currently, when building pyarrow from source, one needs to manually enable the optional components through setting PYARROW_WITH_... environment variables. The example from the docs (https://arrow.apache.org/docs/dev/developers/python.html#build-and-test):

export PYARROW_WITH_PARQUET=1
export PYARROW_WITH_DATASET=1
export PYARROW_PARALLEL=4
python setup.py build_ext --inplace

Nowadays, if you enable something that isn't actually built in the Arrow C++ library you are building against, our python CMake will give an error. For example, I have build Arrow C++ without ARROW_ORC enabled, so when building pyarrow with PYARROW_WITH_ORC=1, I get:

CMake Error at CMakeLists.txt:652 (message):
  You must build Arrow C++ with ARROW_ORC=ON

For development, I think it would be a lot more convenient to by default just enable those python components for which the equivalent Arrow C++ component was built. And we can still keep the option to override this default with the existing environment variables.

In #37822, @joemarshall implemented a solution for this because needing it for building pyarrow for emscripten / pyodide (although I would also just want this for the convenience).
See #37822 (comment) for some details, but the current solution there is to parse a json file generated by cmake to get those C++ build options into setup.py. @kou brought up that we could also do this inside python/CMakeLists.txt, however that is right now not possible because we use those variables in setup.py.

Looking a bit in more detail, it seems we only use those variables 1) to pass those to cmake, and 2) to raise a custom error if a certain cython extension module failed building. _failure_permitted function is using this, which is called from:

arrow/python/setup.py

Lines 330 to 338 in 22f88fa

	for name in self.CYTHON_MODULE_NAMES:
	built_path = pjoin(install_prefix, name + ext_suffix)
	if not os.path.exists(built_path):
	print(f'Did not find {built_path}')
	if self._failure_permitted(name):
	print(f'Cython module {name} failure permitted')
	continue
	raise RuntimeError('PyArrow C-extension failed to build:',
	os.path.abspath(built_path))

However, as I mentioned above, nowadays we already check for the equivalent Arrow C++ component being built from CMake, so I am not sure it is very common that a certain extension module would fail building when it should have been built (without this build error actually bubbling up already, because a failure from cython will just fail your build, without getting to this "failure permitted" check step).

If we remove this "failure_permitted" logic from setup.py, I think we can easily consolidate all component handling in python/CMakeLists.txt.

Does somebody know the history behind the "failure_permitted" logic? Does it still have its use today? (cc @pitrou)

[jorisvandenbossche]

Does somebody know the history behind the "failure_permitted" logic? Does it still have its use today?

The main thing I don't understand about our current setup is in which case you would actually encounter the situation of one the cython extension modules not being built without it erroring during the build.
(maybe in the past things worked differently that required this logic?)

Looking into when this logic was originally added (#132 and #194), it seems that at the time we were using PARQUET_FOUND to decide to build pyarrow with Parquet or not, so when something went wrong there, we were just silently not building pyarrow.parquet.
But now we build this module opt-in based on PYARROW_BUILD_PARQUET, and in that case require that cmake finds Parquet lib and Arrow C++ is built with Parquet (ARROW_PARQUET is ON), and only then build the cython module. So my understanding is that any error of not having/finding libparquet as a failure in building the cython module will already fail loudly. Making this "failure_permitted" logic superfluous?

[paleolimbot]

Just a note that I think we have been doing this in the R package for quite some time (with apologies if I missed part of the nuance here):

arrow/r/configure

Lines 387 to 390 in 22f88fa

	arrow_built_with() {
	# Function to check cmake options for features
	grep -i 'set('"$1"' "ON")' $ARROW_OPTS_CMAKE >/dev/null 2>&1
	}

[kou]

I think that we can remove the logic from python/setup.py.

The logic was borrowed from dynd-python: https://github.com/apache/arrow/pull/17/files#diff-eb8b42d9346d0a5d371facf21a8bfa2d16fb49e213ae7c21f03863accebe0fcfR83-R84

The check was for moving built extensions to proper locations: https://github.com/libdynd/dynd-python/blob/741d9d16c437f778d8c0d7259d4634c9af43962b/setup.py#L250-L266

The moving logic was already moved to python/CMakeLists.txt: #14925

I should have removed the logic in the PR. Sorry...

[jorisvandenbossche]

@kou I don't think it is about the moving logic, there was some additional logic to check if a certain cython extension module output existed (and this logic was only added after the initial implementation that was taken from dynd-python).

Anyway, I put up a PR for this: #41494. Your review would be very welcome!

[pitrou]

Does somebody know the history behind the "failure_permitted" logic? Does it still have its use today? (cc @pitrou)

The idea was probably to let people build a light-weight PyArrow even if a full-blown Arrow C++ is installed. That assumes people are interested in building PyArrow themselves while using a pre-built Arrow C++. I'm skeptical that this is a widespread use case.

[jorisvandenbossche]

And that is still possible by setting PYARROW_WITH_...=0 regardless of the "failure_permitted" logic being removed

[pitrou]

Ah, ok. Well, that clears things up then.

[paleolimbot]

I'm skeptical that this is a widespread use case.

I think that pretty much all pyarrow developers have to do this? (Although pyarrow developers also might have a more limited C++ build that they use as a default).

[jorisvandenbossche]

No (at least not me), I think typically pyarrow developers just build similar features sets for Arrow C++ and pyarrow (my default feature set I enable for Arrow C++ is indeed relatively limited).
But anyway, you don't "have" to do this, because even if you have an Arrow C++ with all features, pyarrow will still build just fine regardless of which pyarrow features you disable/enable (maybe I misunderstood your comment, though).

[paleolimbot]

I was worried that adding the extra modules by default would impact the iteration time (e.g., edit some cython file, rebuild to see if it worked), but I just tested and it doesn't make much of a difference (compute is the only module that dominates the rebuild time time).

[kou]

I don't think it is about the moving logic, there was some additional logic to check if a certain cython extension module output existed (and this logic was only added after the initial implementation that was taken from dynd-python).

The check logic also exists in dynd-python: https://github.com/libdynd/dynd-python/blob/741d9d16c437f778d8c0d7259d4634c9af43962b/setup.py#L265-L266

Anyway, I think that we don't need to do it here. We should do it as a general test. And we already has it. For example:

arrow/ci/scripts/python_wheel_unix_test.sh

Lines 64 to 88 in 49bf3d9

	if [ "${CHECK_IMPORTS}" == "ON" ]; then
	# Test that the modules are importable
	python -c "
	import pyarrow
	import pyarrow._hdfs
	import pyarrow.csv
	import pyarrow.dataset
	import pyarrow.fs
	import pyarrow.json
	import pyarrow.orc
	import pyarrow.parquet
	"
	if [ "${PYARROW_TEST_GCS}" == "ON" ]; then
	python -c "import pyarrow._gcsfs"
	fi
	if [ "${PYARROW_TEST_S3}" == "ON" ]; then
	python -c "import pyarrow._s3fs"
	fi
	if [ "${PYARROW_TEST_FLIGHT}" == "ON" ]; then
	python -c "import pyarrow.flight"
	fi
	if [ "${PYARROW_TEST_SUBSTRAIT}" == "ON" ]; then
	python -c "import pyarrow.substrait"
	fi
	fi

[jorisvandenbossche]

Ah, you're right, the logic to raise an error when a module wasn't build already existed (and was indeed related to the moving). But I was talking just about the feature to allow suppressing this error. Sorry for the confusion ;)

In any case, the PR is removing it.

[jorisvandenbossche]

Issue resolved by pull request 41494
#41494

[rok]

Thanks for doing this @jorisvandenbossche!
Should development docs be updated to reflect the new behavior?

[jorisvandenbossche]

Yes, I had started on that yesterday, but didn't yet open a PR. Did that now -> #41705