You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "jorisvandenbossche (via GitHub)" <gi...@apache.org> on 2024/05/01 12:15:49 UTC
[I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
jorisvandenbossche opened a new issue, #41480:
URL: https://github.com/apache/arrow/issues/41480
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 https://github.com/apache/arrow/pull/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 https://github.com/apache/arrow/pull/37822#discussion_r1344887227 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:
https://github.com/apache/arrow/blob/22f88fa4a8f5ac7250f1845aace5a78d20006ef2/python/setup.py#L330-L338
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2088423982
> 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 (https://github.com/apache/arrow/pull/132 and https://github.com/apache/arrow/pull/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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2092881873
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 ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090518512
And that is still possible by setting `PYARROW_WITH_...=0` regardless of the "failure_permitted" logic being removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2115737154
Thanks for doing this @jorisvandenbossche!
Should development docs be updated to reflect the new behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2089148878
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`: https://github.com/apache/arrow/pull/14925
I should have removed the logic in the PR. Sorry...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche closed issue #41480: [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++
URL: https://github.com/apache/arrow/issues/41480
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2088573991
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):
https://github.com/apache/arrow/blob/22f88fa4a8f5ac7250f1845aace5a78d20006ef2/r/configure#L387-L390
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090458188
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2117094546
Yes, I had started on that yesterday, but didn't yet open a PR. Did that now -> https://github.com/apache/arrow/pull/41705
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2091653457
> 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: https://github.com/apache/arrow/blob/49bf3d9bf2ca266fcf63dca1e57bdb83c9559b72/ci/scripts/python_wheel_unix_test.sh#L64-L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090551554
Ah, ok. Well, that clears things up then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2115094129
Issue resolved by pull request 41494
https://github.com/apache/arrow/pull/41494
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090579544
> 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090052506
@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: https://github.com/apache/arrow/pull/41494. Your review would be very welcome!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090593455
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Python] Building PyArrow: enable/disable python components by default based on availability in Arrow C++ [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on issue #41480:
URL: https://github.com/apache/arrow/issues/41480#issuecomment-2090622276
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org