ARROW-17016: [C++][Python] Move Arrow Python C++ tests into Cython #14117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
AlenkaF
force-pushed
the
cython_test_example
branch
from
8ff5341 to
afe5866
Compare
pitrou
reviewed
Sep 26, 2022
|
Should we wait for ARROW-17843 so that the PyArrow C++ code is checked or we can correct that within the linter issue itself? |
|
We can correct the C++ linting later IMHO. |
|
The failures on the CI are not new, have seen them on the nightly builds. I think this PR is ready, totally approved from my side :) |
|
@AlenkaF can you merge in or rebase on latest master? That should resolve those CI failures |
AlenkaF
force-pushed
the
cython_test_example
branch
from
b8bffc2 to
ebeee04
Compare
|
@github-actions crossbow submit -g python -g wheel |
|
Revision: 7e4e2e6 Submitted crossbow builds: ursacomputing/crossbow @ actions-6dfbce193c
|
|
I think the failures of the wheels are related to the fact that |
|
Yes, we will probably need to enable all desired components manually. |
|
Yes, that is getting fixed in #14273 |
|
You think I should wait for #14273 to get merged and then rebase? |
…d InferPrecisionAndNegativeScale
…return a Status and run them from Pytest
AlenkaF
force-pushed
the
cython_test_example
branch
from
7e4e2e6 to
be21035
Compare
|
@pitrou @jorisvandenbossche this PR is now ready for final round of review/merge. |
|
@github-actions crossbow submit -g python -g wheel |
|
Revision: be21035 Submitted crossbow builds: ursacomputing/crossbow @ actions-f48ddf2e66
|
|
Nothing seems to have changed since my last review, so I'm simply gonna merge! |
|
Thank you for running the wheels build - totally forgot that was what I was waiting to check :) |
|
Benchmark runs are scheduled for baseline = 441af34 and contender = 5fd54bc. 5fd54bc is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
This was referenced Oct 17, 2022
pitrou
pushed a commit
that referenced
this pull request
Oct 17, 2022
…14435) As #14117 is merged separate run of `ctest` for `PyArrow C++` tests is not needed anymore and is removed in this PR. Authored-by: Alenka Frim <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
fatemehp
pushed a commit
to fatemehp/arrow
that referenced
this pull request
Oct 17, 2022
…pache#14117) This PR tries to connect the PyArrow C++ tests with PyArrow tests so they can all be run from `pytest`. This will remove GoogleTest as a dependency for PyArrow and therefore a change is needed in the C++ tests so they return a Status which can then be checked through Cython/Python. Example of pytest run: ``` pyarrow/tests/test_cpp_internals.py::test_owned_ref_moves PASSED pyarrow/tests/test_cpp_internals.py::test_owned_ref_nogil_moves PASSED pyarrow/tests/test_cpp_internals.py::test_check_pyerror_status PASSED pyarrow/tests/test_cpp_internals.py::test_check_pyerror_status_nogil PASSED pyarrow/tests/test_cpp_internals.py::test_restore_pyerror_basics PASSED pyarrow/tests/test_cpp_internals.py::test_pybuffer_invalid_input_object PASSED pyarrow/tests/test_cpp_internals.py::test_pybuffer_numpy_array PASSED pyarrow/tests/test_cpp_internals.py::test_numpybuffer_numpy_array PASSED pyarrow/tests/test_cpp_internals.py::test_python_decimal_to_string PASSED pyarrow/tests/test_cpp_internals.py::test_infer_precision_and_scale PASSED pyarrow/tests/test_cpp_internals.py::test_infer_precision_and_negative_scale PASSED pyarrow/tests/test_cpp_internals.py::test_infer_all_leading_zeros PASSED pyarrow/tests/test_cpp_internals.py::test_infer_all_leading_zeros_exponential_notation_positive PASSED pyarrow/tests/test_cpp_internals.py::test_infer_all_leading_zeros_exponential_notation_negative PASSED pyarrow/tests/test_cpp_internals.py::test_object_block_write_fails PASSED pyarrow/tests/test_cpp_internals.py::test_mixed_type_fails PASSED pyarrow/tests/test_cpp_internals.py::test_from_python_decimal_rescale_not_truncateable PASSED pyarrow/tests/test_cpp_internals.py::test_from_python_decimal_rescale_truncateable PASSED pyarrow/tests/test_cpp_internals.py::test_from_python_negative_decimal_rescale PASSED pyarrow/tests/test_cpp_internals.py::test_decimal128_from_python_integer PASSED pyarrow/tests/test_cpp_internals.py::test_decimal256_from_python_integer PASSED pyarrow/tests/test_cpp_internals.py::test_decimal128_overflow_fails PASSED pyarrow/tests/test_cpp_internals.py::test_decimal256_overflow_fails PASSED pyarrow/tests/test_cpp_internals.py::test_none_and_nan PASSED pyarrow/tests/test_cpp_internals.py::test_mixed_precision_and_scale PASSED pyarrow/tests/test_cpp_internals.py::test_mixed_precision_and_scale_sequence_convert PASSED pyarrow/tests/test_cpp_internals.py::test_simple_inference PASSED pyarrow/tests/test_cpp_internals.py::test_update_with_nan PASSED ``` Lead-authored-by: Alenka Frim <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
jorisvandenbossche
pushed a commit
that referenced
this pull request
Oct 19, 2022
…pers/python page (#14350) This PR updates Windows section of the Python Development page. Main changes: - use Python 3.10 (also in instructions for Linux/MacOs) - definition of `PATH` not needed as Python doesn't search in `PATH` for dlls anymore ([3.8 +](https://bugs.python.org/issue43173)) - use `CONDA_PREFIX` to define `ARROW_HOME` as in other parts of the docs - remove **Running C++ unit tests for Python integration** section (C++ unit tests are part of `pytest`-based test module as of #14117) cc @wjones127 @jorisvandenbossche Authored-by: Alenka Frim <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
kou
pushed a commit
that referenced
this pull request
Oct 20, 2022
…14435) As #14117 is merged separate run of `ctest` for `PyArrow C++` tests is not needed anymore and is removed in this PR. Authored-by: Alenka Frim <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
kou
pushed a commit
that referenced
this pull request
Oct 20, 2022
…pers/python page (#14350) This PR updates Windows section of the Python Development page. Main changes: - use Python 3.10 (also in instructions for Linux/MacOs) - definition of `PATH` not needed as Python doesn't search in `PATH` for dlls anymore ([3.8 +](https://bugs.python.org/issue43173)) - use `CONDA_PREFIX` to define `ARROW_HOME` as in other parts of the docs - remove **Running C++ unit tests for Python integration** section (C++ unit tests are part of `pytest`-based test module as of #14117) cc @wjones127 @jorisvandenbossche Authored-by: Alenka Frim <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
This was referenced Sep 18, 2023
kevingurney
added a commit
that referenced
this pull request
Sep 19, 2023
…ke build system for the MATLAB interface (#37784) ### Rationale for this change This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface. 1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code. 2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows. 3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in #37532 (comment). 4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](#14117). 5. On a somewhat related note - removing `GoogleTest` support will help unblock #37773 as discussed in #37773 (comment). ### What changes are included in this PR? 1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build. 2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`. 3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests. 4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead. 5. Removed placeholder C++ test (i.e. `placeholder_test.cc`). ### Are these changes tested? Yes. The MATLAB CI workflow is passing on all platforms. ### Are there any user-facing changes? Yes. There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake. ### Future Directions 1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB. ### Notes 1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in #37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach. 2. Thank you @ sgilmore10 for your help with this pull request! * Closes: #37532 Lead-authored-by: Kevin Gurney <[email protected]> Co-authored-by: Sarah Gilmore <[email protected]> Signed-off-by: Kevin Gurney <[email protected]>
loicalleyne
pushed a commit
to loicalleyne/arrow
that referenced
this pull request
Nov 13, 2023
…he CMake build system for the MATLAB interface (apache#37784) ### Rationale for this change This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface. 1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code. 2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows. 3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in apache#37532 (comment). 4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](apache#14117). 5. On a somewhat related note - removing `GoogleTest` support will help unblock apache#37773 as discussed in apache#37773 (comment). ### What changes are included in this PR? 1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build. 2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`. 3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests. 4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead. 5. Removed placeholder C++ test (i.e. `placeholder_test.cc`). ### Are these changes tested? Yes. The MATLAB CI workflow is passing on all platforms. ### Are there any user-facing changes? Yes. There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake. ### Future Directions 1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB. ### Notes 1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in apache#37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach. 2. Thank you @ sgilmore10 for your help with this pull request! * Closes: apache#37532 Lead-authored-by: Kevin Gurney <[email protected]> Co-authored-by: Sarah Gilmore <[email protected]> Signed-off-by: Kevin Gurney <[email protected]>
dgreiss
pushed a commit
to dgreiss/arrow
that referenced
this pull request
Feb 19, 2024
…he CMake build system for the MATLAB interface (apache#37784) ### Rationale for this change This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface. 1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code. 2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows. 3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in apache#37532 (comment). 4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](apache#14117). 5. On a somewhat related note - removing `GoogleTest` support will help unblock apache#37773 as discussed in apache#37773 (comment). ### What changes are included in this PR? 1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build. 2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`. 3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests. 4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead. 5. Removed placeholder C++ test (i.e. `placeholder_test.cc`). ### Are these changes tested? Yes. The MATLAB CI workflow is passing on all platforms. ### Are there any user-facing changes? Yes. There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake. ### Future Directions 1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB. ### Notes 1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in apache#37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach. 2. Thank you @ sgilmore10 for your help with this pull request! * Closes: apache#37532 Lead-authored-by: Kevin Gurney <[email protected]> Co-authored-by: Sarah Gilmore <[email protected]> Signed-off-by: Kevin Gurney <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to connect the PyArrow C++ tests with PyArrow tests so they can all be run from
pytest. This will remove GoogleTest as a dependency for PyArrow and therefore a change is needed in the C++ tests so they return a Status which can then be checked through Cython/Python.Example of pytest run: