Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou merged PR #46755: URL: https://github.com/apache/arrow/pull/46755 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2207758663 ## python/pyarrow/tests/test_misc.py: ## @@ -85,13 +85,17 @@ def run_with_env_var(env_var): def test_build_info(): -assert isinstance(pa.cpp_build_info, pa.BuildInfo) +assert isinstance(pa.build_info.cpp_build_info, pa.CppBuildInfo) assert isinstance(pa.cpp_version_info, pa.VersionInfo) assert isinstance(pa.cpp_version, str) assert isinstance(pa.__version__, str) -assert pa.cpp_build_info.version_info == pa.cpp_version_info +assert pa.build_info.cpp_build_info.version_info == pa.cpp_version_info Review Comment: ```suggestion assert pa.build_info.cpp_build_info.version == pa.cpp_version assert pa.build_info.cpp_build_info.version_info == pa.cpp_version_info assert pa.build_info.cpp_build_info is pa.cpp_build_info ``` -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2207755435
##
python/pyarrow/config.pxi:
##
@@ -77,6 +46,57 @@ def runtime_info():
detected_simd_level=frombytes(c_info.detected_simd_level))
+BuildInfo = namedtuple(
+'BuildInfo',
+('build_type', 'cpp_build_info'))
+
+CppBuildInfo = namedtuple(
+'CppBuildInfo',
+('version', 'version_info', 'so_version', 'full_so_version',
+ 'compiler_id', 'compiler_version', 'compiler_flags',
+ 'git_id', 'git_description', 'package_kind', 'build_type'))
+
+
+def _build_info():
+"""
+Get PyArrow build information.
+
+Returns
+---
+info : pyarrow.BuildInfo
+"""
+cdef:
+const libarrow_python.CBuildInfo* c_info
+const libarrow.CCppBuildInfo* c_cpp_info
+
+c_info = &libarrow_python.GetBuildInfo()
+c_cpp_info = &libarrow.GetCppBuildInfo()
+
+cpp_build_info = CppBuildInfo(version=frombytes(c_cpp_info.version_string),
+
version_info=VersionInfo(c_cpp_info.version_major,
+
c_cpp_info.version_minor,
+
c_cpp_info.version_patch),
+ so_version=frombytes(c_cpp_info.so_version),
+
full_so_version=frombytes(c_cpp_info.full_so_version),
+
compiler_id=frombytes(c_cpp_info.compiler_id),
+ compiler_version=frombytes(
+ c_cpp_info.compiler_version),
+
compiler_flags=frombytes(c_cpp_info.compiler_flags),
+ git_id=frombytes(c_cpp_info.git_id),
+
git_description=frombytes(c_cpp_info.git_description),
+
package_kind=frombytes(c_cpp_info.package_kind),
+
build_type=frombytes(c_cpp_info.build_type).lower(),
+ )
+
+return BuildInfo(build_type=c_info.build_type.decode('utf-8').lower(),
+ cpp_build_info=cpp_build_info)
+
+
+build_info = _build_info()
+cpp_version = build_info.cpp_build_info.version
Review Comment:
How about we keep `cpp_build_info` for compatibility?
```suggestion
cpp_build_info = build_info.cpp_build_info
cpp_version = build_info.cpp_build_info.version
```
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2199073574 ## python/pyarrow/includes/libarrow_python_config.pxd: ## @@ -0,0 +1,29 @@ +# Licensed to the Apache Software Foundation (ASF) under one Review Comment: I am new to Cython and was getting ambiguous overload compilation errors from both GetBuildInfos with a variety of approaches. I did this [dummy namespace pxd](https://cython.readthedocs.io/en/latest/src/userguide/sharing_declarations.html#using-cimport-to-resolve-naming-conflicts) to address naming conflicts; the docs say > "There won’t be any actual [libarrow_python_config] module at run time, but that doesn’t matter; the [libarrow_python_config].pxd file has done its job of providing an additional namespace at compile time." Anyways, I agree it makes sense to keep things in libarrow_python if possible, and I've since found a way to do that. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2194842619 ## python/pyarrow/includes/libarrow_python_config.pxd: ## @@ -0,0 +1,29 @@ +# Licensed to the Apache Software Foundation (ASF) under one Review Comment: This file would be suitable if we actually had a `libarrow_python_config` shared library, but we don't, these APIs are part of `libarrow_python`. Therefore, these declarations should be moved to `libarrow_python.pxd`. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2181200538 ## python/pyarrow/tests/test_misc.py: ## @@ -94,6 +94,10 @@ def test_build_info(): assert pa.cpp_build_info.build_type in ( 'debug', 'release', 'minsizerel', 'relwithdebinfo') +assert isinstance(pa.build_info(), pa.PythonBuildInfo) Review Comment: Ok, did so. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2181199143 ## python/pyarrow/config.pxi: ## @@ -77,6 +78,27 @@ def runtime_info(): detected_simd_level=frombytes(c_info.detected_simd_level)) +PythonBuildInfo = namedtuple( Review Comment: Ok, did that. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2178707611
##
python/pyarrow/tests/test_gdb.py:
##
@@ -195,6 +195,8 @@ def gdb():
def gdb_arrow(gdb):
if 'deb' not in pa.cpp_build_info.build_type:
pytest.skip("Arrow C++ debug symbols not available")
+if pa.build_info().build_type != 'debug':
Review Comment:
With relwithdebinfo, less of the tests fail than release but several still
fail.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2178707611
##
python/pyarrow/tests/test_gdb.py:
##
@@ -195,6 +195,8 @@ def gdb():
def gdb_arrow(gdb):
if 'deb' not in pa.cpp_build_info.build_type:
pytest.skip("Arrow C++ debug symbols not available")
+if pa.build_info().build_type != 'debug':
Review Comment:
With relwithdebug, less of the tests fail than release but several still
fail.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2177638861
##
python/pyarrow/tests/test_gdb.py:
##
@@ -195,6 +195,8 @@ def gdb():
def gdb_arrow(gdb):
if 'deb' not in pa.cpp_build_info.build_type:
pytest.skip("Arrow C++ debug symbols not available")
+if pa.build_info().build_type != 'debug':
Review Comment:
Someone would have to try it out, but I think `relwithdebug` might optimize
local variables away.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
WillAyd commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2177615577
##
python/pyarrow/tests/test_gdb.py:
##
@@ -195,6 +195,8 @@ def gdb():
def gdb_arrow(gdb):
if 'deb' not in pa.cpp_build_info.build_type:
pytest.skip("Arrow C++ debug symbols not available")
+if pa.build_info().build_type != 'debug':
Review Comment:
Should we also allow `relwithdebug` as a value?
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2176491669 ## python/pyarrow/config.pxi: ## @@ -77,6 +78,27 @@ def runtime_info(): detected_simd_level=frombytes(c_info.detected_simd_level)) +PythonBuildInfo = namedtuple( Review Comment: Suggestion: 1. rename the `BuildInfo` class above to `CppBuildInfo` 2. rename this `PythonBuildInfo` class to `BuildInfo` 3. add the `CppBuildInfo` as an attribute of `BuildInfo` ## python/pyarrow/tests/test_misc.py: ## @@ -94,6 +94,10 @@ def test_build_info(): assert pa.cpp_build_info.build_type in ( 'debug', 'release', 'minsizerel', 'relwithdebinfo') +assert isinstance(pa.build_info(), pa.PythonBuildInfo) Review Comment: Ah, I realize that `pa.cpp_build_info` is a simple attribute, not a function, so perhaps `pa.build_info` should be made an attribute as well? -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2144153116 ## python/pyarrow/tests/test_gdb.py: ## @@ -25,6 +25,7 @@ import pytest import pyarrow as pa +from pyarrow.lib import get_pybuild_type Review Comment: Fixed. Exposed pa.build_info(). ## python/pyarrow/tests/test_misc.py: ## @@ -23,6 +23,7 @@ import pyarrow as pa from pyarrow.lib import ArrowInvalid +from pyarrow.lib import get_pybuild_type Review Comment: Fixed. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2144151695
##
python/CMakeLists.txt:
##
@@ -350,6 +350,11 @@ endif()
# PyArrow C++
set(PYARROW_CPP_ROOT_DIR pyarrow/src)
set(PYARROW_CPP_SOURCE_DIR ${PYARROW_CPP_ROOT_DIR}/arrow/python)
+
+# Write out compile-time configuration constants
+string(TOLOWER ${CMAKE_BUILD_TYPE} LOWERCASE_PYBUILD_TYPE)
+configure_file("${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h.cmake"
"${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h" ESCAPE_QUOTES)
Review Comment:
Ok, there is now config.h/.cc instead of doing this in helpers, and
config_internal.h.cmake.
##
python/pyarrow/src/arrow/python/helpers.cc:
##
@@ -499,6 +500,14 @@ bool IsThreadingEnabled() {
#endif
}
+namespace {
+ const std::string kPyBuildType = PYARROW_CYTHON_BUILD_TYPE;
+}
+
+std::string GetPyBuildType() {
+ return kPyBuildType;
+}
Review Comment:
Fixed.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2144151947 ## python/pyarrow/src/arrow/python/pybuild_type.h.cmake: ## @@ -0,0 +1,18 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#define PYARROW_CYTHON_BUILD_TYPE "@LOWERCASE_PYBUILD_TYPE@" Review Comment: Fixed. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2144150093 ## python/pyarrow/lib.pyx: ## @@ -96,6 +96,15 @@ def is_threading_enabled() -> bool: return libarrow_python.IsThreadingEnabled() +def get_pybuild_type() -> str: +""" +Returns the PyArrow build type (debug, minsizerel, release, +relwithdebinfo). The default build type is release, regardless of if C++ +was built in debug mode. Review Comment: Ok. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
dinse commented on code in PR #46755: URL: https://github.com/apache/arrow/pull/46755#discussion_r2144149906 ## python/pyarrow/lib.pyx: ## @@ -96,6 +96,15 @@ def is_threading_enabled() -> bool: return libarrow_python.IsThreadingEnabled() +def get_pybuild_type() -> str: Review Comment: Ok, pa.build_info() now returns a namedtuple. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
pitrou commented on code in PR #46755:
URL: https://github.com/apache/arrow/pull/46755#discussion_r2137128020
##
python/pyarrow/lib.pyx:
##
@@ -96,6 +96,15 @@ def is_threading_enabled() -> bool:
return libarrow_python.IsThreadingEnabled()
+def get_pybuild_type() -> str:
+"""
+Returns the PyArrow build type (debug, minsizerel, release,
+relwithdebinfo). The default build type is release, regardless of if C++
+was built in debug mode.
Review Comment:
Can we keep using the summary line + longer description convention?
##
python/pyarrow/lib.pyx:
##
@@ -96,6 +96,15 @@ def is_threading_enabled() -> bool:
return libarrow_python.IsThreadingEnabled()
+def get_pybuild_type() -> str:
Review Comment:
The function name is awkward and restrictive. Perhaps we want a function
`def build_info() -> dict` that returns various pieces of information about the
build and, more importantly, that we can extend in the future?
##
python/pyarrow/src/arrow/python/helpers.cc:
##
@@ -499,6 +500,14 @@ bool IsThreadingEnabled() {
#endif
}
+namespace {
+ const std::string kPyBuildType = PYARROW_CYTHON_BUILD_TYPE;
+}
+
+std::string GetPyBuildType() {
+ return kPyBuildType;
+}
Review Comment:
The separate constant seems a bit pointless to me? This will return a copy
anyway.
```suggestion
std::string GetPyBuildType() {
return PYARROW_CYTHON_BUILD_TYPE;
}
```
##
python/pyarrow/tests/test_gdb.py:
##
@@ -25,6 +25,7 @@
import pytest
import pyarrow as pa
+from pyarrow.lib import get_pybuild_type
Review Comment:
You don't need to import it explicitly, just call `pa.get_pybuild_type`?
(and make sure this is exposed in the top-level `pyarrow` namespace)
##
python/pyarrow/tests/test_misc.py:
##
@@ -23,6 +23,7 @@
import pyarrow as pa
from pyarrow.lib import ArrowInvalid
+from pyarrow.lib import get_pybuild_type
Review Comment:
Same here.
##
python/pyarrow/src/arrow/python/pybuild_type.h.cmake:
##
@@ -0,0 +1,18 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#define PYARROW_CYTHON_BUILD_TYPE "@LOWERCASE_PYBUILD_TYPE@"
Review Comment:
I would simply call it `PYARROW_BUILD_TYPE`, because this is not related to
Cython.
##
python/CMakeLists.txt:
##
@@ -350,6 +350,11 @@ endif()
# PyArrow C++
set(PYARROW_CPP_ROOT_DIR pyarrow/src)
set(PYARROW_CPP_SOURCE_DIR ${PYARROW_CPP_ROOT_DIR}/arrow/python)
+
+# Write out compile-time configuration constants
+string(TOLOWER ${CMAKE_BUILD_TYPE} LOWERCASE_PYBUILD_TYPE)
+configure_file("${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h.cmake"
"${PYARROW_CPP_SOURCE_DIR}/pybuild_type.h" ESCAPE_QUOTES)
Review Comment:
Why not make this something like `config.h.cmake`? We may want to add other
constants there in the future.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-46728: [Python] Skip test_gdb.py tests if PyArrow wasn't built debug [arrow]
github-actions[bot] commented on PR #46755: URL: https://github.com/apache/arrow/pull/46755#issuecomment-2957306613 :warning: GitHub issue #46728 **has been automatically assigned in GitHub** to PR creator. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
