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

GH-36730: [Python] Add support for Cython 3.0.0 #36745

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ jobs:
shell: bash
run: |
gem install test-unit
pip install "cython<3" setuptools six pytest jira
pip install cython setuptools six pytest jira
- name: Run Release Test
env:
ARROW_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Expand Down
2 changes: 1 addition & 1 deletion ci/conda_env_python.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
# don't add pandas here, because it is not a mandatory test dependency
boto3 # not a direct dependency of s3fs, but needed for our s3fs fixture
cffi
cython<3
cython
cloudpickle
fsspec
hypothesis
Expand Down
2 changes: 1 addition & 1 deletion dev/release/verify-release-candidate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ test_python() {
show_header "Build and test Python libraries"

# Build and test Python
maybe_setup_virtualenv "cython<3" numpy setuptools_scm setuptools || exit 1
maybe_setup_virtualenv cython numpy setuptools_scm setuptools || exit 1
maybe_setup_conda --file ci/conda_env_python.txt || exit 1

if [ "${USE_CONDA}" -gt 0 ]; then
Expand Down
29 changes: 18 additions & 11 deletions python/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -168,37 +168,44 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${PYARROW_CXXFLAGS}")

if(MSVC)
# MSVC version of -Wno-return-type-c-linkage
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4190")
string(APPEND 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")
string(APPEND 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")
string(APPEND 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")
string(APPEND CMAKE_CXX_FLAGS " /wd4804")

# See https://github.com/cython/cython/issues/4445.
#
# Cython 3 emits "(void)__Pyx_PyObject_CallMethod0;" to suppress a
# "unused function" warning but the code emits another "function
# call missing argument list" warning.
string(APPEND CMAKE_CXX_FLAGS " /wd4551")
else()
# Enable perf and other tools to work properly
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer")
string(APPEND CMAKE_CXX_FLAGS " -fno-omit-frame-pointer")

# Suppress Cython warnings
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-variable -Wno-maybe-uninitialized")
string(APPEND 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")
string(APPEND CMAKE_CXX_FLAGS " -Wno-parentheses-equality")
string(APPEND CMAKE_CXX_FLAGS " -Wno-constant-logical-operand")
string(APPEND CMAKE_CXX_FLAGS " -Wno-missing-declarations")
string(APPEND 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")
string(APPEND CMAKE_CXX_FLAGS " -Wno-return-type-c-linkage")
endif()
endif()

Expand Down
10 changes: 7 additions & 3 deletions python/pyarrow/_flight.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -982,8 +982,10 @@ cdef class _MetadataRecordBatchReader(_Weakrefable, _ReadPandasMixin):
cdef shared_ptr[CMetadataRecordBatchReader] reader

def __iter__(self):
while True:
yield self.read_chunk()
return self

def __next__(self):
return self.read_chunk()

@property
def schema(self):
Expand Down Expand Up @@ -1625,7 +1627,9 @@ cdef class FlightClient(_Weakrefable):

def close(self):
"""Close the client and disconnect."""
check_flight_status(self.client.get().Close())
client = self.client.get()
if client != NULL:
check_flight_status(client.Close())

def __del__(self):
# Not ideal, but close() wasn't originally present so
Expand Down
15 changes: 10 additions & 5 deletions python/pyarrow/includes/libarrow_flight.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,16 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
c_bool Equals(const CLocation& other)

@staticmethod
CResult[CLocation] Parse(c_string& uri_string)
CResult[CLocation] Parse(const c_string& uri_string)

@staticmethod
CResult[CLocation] ForGrpcTcp(c_string& host, int port)
CResult[CLocation] ForGrpcTcp(const c_string& host, int port)

@staticmethod
CResult[CLocation] ForGrpcTls(c_string& host, int port)
CResult[CLocation] ForGrpcTls(const c_string& host, int port)

@staticmethod
CResult[CLocation] ForGrpcUnix(c_string& path)
CResult[CLocation] ForGrpcUnix(const c_string& path)

cdef cppclass CFlightEndpoint" arrow::flight::FlightEndpoint":
CFlightEndpoint()
Expand Down Expand Up @@ -172,7 +172,9 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CResult[unique_ptr[CFlightInfo]] Next()

cdef cppclass CSimpleFlightListing" arrow::flight::SimpleFlightListing":
CSimpleFlightListing(vector[CFlightInfo]&& info)
# This doesn't work with Cython >= 3
# CSimpleFlightListing(vector[CFlightInfo]&& info)
CSimpleFlightListing(const vector[CFlightInfo]& info)

cdef cppclass CFlightPayload" arrow::flight::FlightPayload":
shared_ptr[CBuffer] descriptor
Expand Down Expand Up @@ -308,7 +310,10 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
cdef cppclass CCallHeaders" arrow::flight::CallHeaders":
cppclass const_iterator:
pair[c_string, c_string] operator*()
# For Cython < 3
const_iterator operator++()
# For Cython >= 3
const_iterator operator++(int)
bint operator==(const_iterator)
bint operator!=(const_iterator)
const_iterator cbegin()
Expand Down
15 changes: 8 additions & 7 deletions python/pyarrow/ipc.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -436,8 +436,10 @@ cdef class MessageReader(_Weakrefable):
return result

def __iter__(self):
while True:
yield self.read_next_message()
return self

def __next__(self):
return self.read_next_message()

def read_next_message(self):
"""
Expand Down Expand Up @@ -656,11 +658,10 @@ cdef class RecordBatchReader(_Weakrefable):
# cdef block is in lib.pxd

def __iter__(self):
while True:
try:
yield self.read_next_batch()
except StopIteration:
return
return self

def __next__(self):
return self.read_next_batch()

@property
def schema(self):
Expand Down
5 changes: 4 additions & 1 deletion python/pyarrow/scalar.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -791,8 +791,11 @@ cdef class MapScalar(ListScalar):
"""
Iterate over this element's values.
"""
return self

def __next__(self):
arr = self.values
if array is None:
if arr is None:
raise StopIteration
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

Copy link
Member

Choose a reason for hiding this comment

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

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

Expand Down
2 changes: 1 addition & 1 deletion python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

[build-system]
requires = [
"cython >= 0.29.31,<3",
"cython >= 0.29.31",
"oldest-supported-numpy>=0.14",
"setuptools_scm",
"setuptools >= 40.1.0",
Expand Down
2 changes: 1 addition & 1 deletion python/requirements-build.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cython>=0.29.31,<3
cython>=0.29.31
oldest-supported-numpy>=0.14
setuptools_scm
setuptools>=38.6.0
2 changes: 1 addition & 1 deletion python/requirements-wheel-build.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cython>=0.29.31,<3
cython>=0.29.31
oldest-supported-numpy>=0.14
setuptools_scm
setuptools>=58
Expand Down
6 changes: 3 additions & 3 deletions python/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
# Check if we're running 64-bit Python
is_64_bit = sys.maxsize > 2**32

if Cython.__version__ < '0.29.31' or Cython.__version__ >= '3.0':
if Cython.__version__ < '0.29.31':
raise Exception(
'Please update your Cython version. Supported Cython >= 0.29.31, < 3.0')
'Please update your Cython version. Supported Cython >= 0.29.31')

setup_dir = os.path.abspath(os.path.dirname(__file__))

Expand Down Expand Up @@ -492,7 +492,7 @@ def has_ext_modules(foo):
'pyarrow/_generated_version.py'),
'version_scheme': guess_next_dev_version
},
setup_requires=['setuptools_scm', 'cython >= 0.29.31,<3'] + setup_requires,
setup_requires=['setuptools_scm', 'cython >= 0.29.31'] + setup_requires,
install_requires=install_requires,
tests_require=['pytest', 'pandas', 'hypothesis'],
python_requires='>=3.8',
Expand Down