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

ARROW-17579: [Python] PYARROW_CXXFLAGS ignored? #14074

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

AlenkaF
Copy link
Member

@AlenkaF AlenkaF commented Sep 8, 2022

This PR adds:

  • PYARROW_CXXFLAGS variable to PyArrow C++ and Pyarrow cmake builds (both defined in setup.py)
  • a common flags section to pyarrow/src/CMakeLists.txt where CXX_COMMON_FLAGS and PYARROW_CXXFLAGS are added to the CMAKE_CXX_FLAGS variable.

Other flags defined in Arrow C++ should be included in PyArrow C++ and PyArrow with include(SetupCxxFlags).

I am not sure if additional flags that are defined in PyArrow, see:

arrow/python/CMakeLists.txt

Lines 134 to 174 in 43670af

if(MSVC)
# MSVC version of -Wno-return-type-c-linkage
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4190")
# Cython generates some bitshift expressions that MSVC does not like in
# __Pyx_PyFloat_DivideObjC
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4293")
# Converting to/from C++ bool is pretty wonky in Cython. The C4800 warning
# seem harmless, and probably not worth the effort of working around it
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4800")
# See https://github.com/cython/cython/issues/2731. Change introduced in
# Cython 0.29.1 causes "unsafe use of type 'bool' in operation"
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4804")
else()
# Enable perf and other tools to work properly
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer")
# Suppress Cython warnings
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-variable -Wno-maybe-uninitialized")
if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL
"Clang")
# Cython warnings in clang
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-parentheses-equality")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-constant-logical-operand")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing-declarations")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-sometimes-uninitialized")
# We have public Cython APIs which return C++ types, which are in an extern
# "C" blog (no symbol mangling) and clang doesn't like this
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-return-type-c-linkage")
endif()
endif()
# For any C code, use the same flags.
set(CMAKE_C_FLAGS "${CMAKE_CXX_FLAGS}")
# Add C++-only flags, like -std=c++11
set(CMAKE_CXX_FLAGS "${CXX_ONLY_FLAGS} ${CMAKE_CXX_FLAGS}")
are needed in PyArrow C++ as they were not used when PyArrow C++ was included in Arrow C++?
cc @pitrou

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

@pitrou
Copy link
Member

pitrou commented Sep 13, 2022

@AlenkaF You might try this patch to fix the AppVeyor error:

diff --git a/python/pyarrow/_compute.pyx b/python/pyarrow/_compute.pyx
index b9594d90e..2aa65e75c 100644
--- a/python/pyarrow/_compute.pyx
+++ b/python/pyarrow/_compute.pyx
@@ -2613,7 +2613,7 @@ def register_scalar_function(func, function_name, function_doc, in_types,
         raise TypeError(
             "in_types must be a dictionary of DataType")
 
-    c_arity = CArity(num_args, func_spec.varargs)
+    c_arity = CArity(<int> num_args, func_spec.varargs)
 
     if "summary" not in function_doc:
         raise ValueError("Function doc must contain a summary")

@pitrou pitrou requested a review from kou September 13, 2022 13:12
python/setup.py Outdated Show resolved Hide resolved
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@AlenkaF
Copy link
Member Author

AlenkaF commented Sep 14, 2022

@pitrou I have added the suggestions and will merge this PR later today if there is no objection.

Note: Travis issue has a JIRA already.

@AlenkaF AlenkaF merged commit 9c628b7 into apache:master Sep 14, 2022
@AlenkaF AlenkaF deleted the ARROW-17579 branch September 14, 2022 16:42
@ursabot
Copy link

ursabot commented Sep 15, 2022

Benchmark runs are scheduled for baseline = 81f3945 and contender = 9c628b7. 9c628b7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.24% ⬆️0.03%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 9c628b70 ec2-t3-xlarge-us-east-2
[Finished] 9c628b70 test-mac-arm
[Failed] 9c628b70 ursa-i9-9960x
[Finished] 9c628b70 ursa-thinkcentre-m75q
[Finished] 81f3945d ec2-t3-xlarge-us-east-2
[Failed] 81f3945d test-mac-arm
[Failed] 81f3945d ursa-i9-9960x
[Finished] 81f3945d ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Sep 15, 2022

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
This PR adds:
- `PYARROW_CXXFLAGS` variable to PyArrow C++ and Pyarrow cmake builds (both defined in `setup.py`)
- a common flags section to `pyarrow/src/CMakeLists.txt` where `CXX_COMMON_FLAGS` and `PYARROW_CXXFLAGS` are added to the `CMAKE_CXX_FLAGS` variable.

Other flags defined in Arrow C++ should be included in PyArrow C++ and PyArrow with `include(SetupCxxFlags)`.

I am not sure if additional flags that are defined in PyArrow, see: https://github.com/apache/arrow/blob/43670af02f0913580fd20e26006fd550d6fdf2da/python/CMakeLists.txt#L134-L174 are needed in PyArrow C++ as they were not used when PyArrow C++ was included in Arrow C++?
cc @pitrou 

Authored-by: Alenka Frim <[email protected]>
Signed-off-by: Alenka Frim <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
This PR adds:
- `PYARROW_CXXFLAGS` variable to PyArrow C++ and Pyarrow cmake builds (both defined in `setup.py`)
- a common flags section to `pyarrow/src/CMakeLists.txt` where `CXX_COMMON_FLAGS` and `PYARROW_CXXFLAGS` are added to the `CMAKE_CXX_FLAGS` variable.

Other flags defined in Arrow C++ should be included in PyArrow C++ and PyArrow with `include(SetupCxxFlags)`.

I am not sure if additional flags that are defined in PyArrow, see: https://github.com/apache/arrow/blob/43670af02f0913580fd20e26006fd550d6fdf2da/python/CMakeLists.txt#L134-L174 are needed in PyArrow C++ as they were not used when PyArrow C++ was included in Arrow C++?
cc @pitrou 

Authored-by: Alenka Frim <[email protected]>
Signed-off-by: Alenka Frim <[email protected]>
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.

4 participants