GH-37254: [Python] Parametrize all pickling tests to use both the pickle and cloudpickle modules #37255

danepitkin · 2023-08-18T19:22:47Z

Rationale for this change

Cloudpickle was not tested in most parts of the pyarrow test suite. Improving this coverage will make the Cython 3.0.0 upgrade cleaner as cloudpickle was failing in a few places where the default pickle module was not. This has been verified using Cython 0.29.36.

What changes are included in this PR?

__reduce__ methods that need to pass kwargs have been changed from classmethod to staticmethod
All pytests that pickle objects are parameterized to use both pickle and cloudpickle

Are these changes tested?

Yes, pytests run successfully with Cython 0.29.36

Are there any user-facing changes?

No.

Closes: [Python] Include cloudpickle in pytests that pickle objects #37254

github-actions · 2023-08-18T19:23:14Z

⚠️ GitHub issue #37254 has been automatically assigned in GitHub to PR creator.

danepitkin · 2023-08-18T19:31:32Z

Cython 3 will set binding=True by default for the compiler. With Cython 0.29.36, we just need to add the binding=True compiler option to make it backwards compatible. Here's a small repro as an example with Cython 0.29:

my_test.pyx

# cython : language_level = 3
import cython

cdef class Foo():
    @staticmethod
    @cython.binding(True)  # This works
    def _reconstruct():
        return Foo()

    def __reduce__(self):
        return Foo._reconstruct, tuple()


cdef class Bar():
    @staticmethod  # This does not work
    def _reconstruct():
        return Bar()

    def __reduce__(self):
        return Bar._reconstruct, tuple()

Usage here:

In [1]: from my_test import Foo, Bar

In [2]: f = Foo()

In [3]: b = Bar()

In [4]: import pickle

In [5]: pickle.dumps(f)
Out[5]: b'\x80\x04\x95#\x00\x00\x00\x00\x00\x00\x00\x8c\x07my_test\x94\x8c\x10Foo._reconstruct\x94\x93\x94)R\x94.'

In [6]: pickle.dumps(b)
---------------------------------------------------------------------------
PicklingError                             Traceback (most recent call last)
Cell In[6], line 1
----> 1 pickle.dumps(b)

PicklingError: Can't pickle <built-in function _reconstruct>: it's not the same object as my_test._reconstruct

danepitkin · 2023-08-18T19:57:02Z

There seems to be an intermittent failure reported:

==================================== ERRORS ====================================
___ ERROR at setup of test_filesystem_pickling[builtin_pickle-S3FileSystem] ____

request = <SubRequest 's3fs' for <Function test_filesystem_pickling[builtin_pickle-S3FileSystem]>>
s3_server = {'connection': ('localhost', 38507, 'arrow', 'apachearrow'), 'process': <Popen: returncode: None args: ['minio', '--compat', 'server', '--quiet', '-...>, 'tempdir': '/tmp/tmpoas4aj3y'}

    @pytest.fixture
    def s3fs(request, s3_server):
        request.config.pyarrow.requires('s3')
        from pyarrow.fs import S3FileSystem
    
        host, port, access_key, secret_key = s3_server['connection']
        bucket = 'pyarrow-filesystem/'
    
        fs = S3FileSystem(
            access_key=access_key,
            secret_key=secret_key,
            endpoint_override='{}:{}'.format(host, port),
            scheme='http',
            allow_bucket_creation=True,
            allow_bucket_deletion=True
        )
>       fs.create_dir(bucket)

opt/conda/envs/arrow/lib/python3.9/site-packages/pyarrow/tests/test_fs.py:250: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pyarrow/_fs.pyx:603: in pyarrow._fs.FileSystem.create_dir
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   OSError: When creating bucket 'pyarrow-filesystem': AWS Error UNKNOWN (HTTP status 503) during HeadBucket operation: No response body.

kou

(Sorry, I can't review this deeply because I'm not familiar with pickle.)

python/pyarrow/_dataset_parquet.pyx

python/pyarrow/tests/parquet/test_dataset.py

danepitkin · 2023-08-21T14:25:00Z

For the intermittent failure, I've also seen it in the CI tests for #37097. The 503 error means "Service unavailable". Seeing as this is a session-scoped pytest fixture and it only is failing on the very first invocation so far, I'm pretty sure that minio is just not fully started yet when the pytest tries to call the server after starting it the first time. We can introduce pytest_xprocess to better manage processes as fixtures. I'll add that to this PR. https://pytest-xprocess.readthedocs.io/en/latest/

danepitkin · 2023-08-21T17:58:20Z

For the intermittent failure, I've also seen it in the CI tests for #37097. The 503 error means "Service unavailable". Seeing as this is a session-scoped pytest fixture and it only is failing on the very first invocation so far, I'm pretty sure that minio is just not fully started yet when the pytest tries to call the server after starting it the first time. We can introduce pytest_xprocess to better manage processes as fixtures. I'll add that to this PR. https://pytest-xprocess.readthedocs.io/en/latest/

xprocess doesn't quite fit cleanly into what we want. I tried adding a connection_timeout parameter to s3fs in our tests instead to see if that helps. We could also add the minio python client as a dependency if we want. This will add retries for 503 errors automatically. Not sure if we want to add another test dependency for that though..

danepitkin · 2023-08-21T18:49:23Z

@github-actions crossbow submit python

github-actions · 2023-08-21T18:51:17Z

Unable to match any tasks for `python`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/5930280856

danepitkin · 2023-08-21T18:55:54Z

@github-actions crossbow submit python

github-actions · 2023-08-21T19:01:21Z

Revision: 0ddbff5

Submitted crossbow builds: ursacomputing/crossbow @ actions-7eabdb808a

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
python-sdist
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3
verify-rc-source-python-linux-almalinux-8-amd64
verify-rc-source-python-linux-conda-latest-amd64
verify-rc-source-python-linux-ubuntu-20.04-amd64
verify-rc-source-python-linux-ubuntu-22.04-amd64
verify-rc-source-python-macos-amd64
verify-rc-source-python-macos-arm64
verify-rc-source-python-macos-conda-amd64

danepitkin · 2023-08-22T00:56:07Z

@github-actions crossbow submit test-conda-python-3.11-hypothesis

github-actions · 2023-08-22T00:58:32Z

Revision: fa83e39

Submitted crossbow builds: ursacomputing/crossbow @ actions-7d987d430e

Task	Status
test-conda-python-3.11-hypothesis

danepitkin · 2023-08-22T00:58:37Z

Still need to fix the intermittent failure with minio server startup.

jorisvandenbossche

Actual code changes (apart from the S3 issue) look good, thanks a lot for this!

danepitkin · 2023-08-22T21:16:38Z

@github-actions crossbow submit python

github-actions · 2023-08-22T21:20:48Z

Revision: 7710a86

Submitted crossbow builds: ursacomputing/crossbow @ actions-c53268ddd6

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
python-sdist
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3
verify-rc-source-python-linux-almalinux-8-amd64
verify-rc-source-python-linux-conda-latest-amd64
verify-rc-source-python-linux-ubuntu-20.04-amd64
verify-rc-source-python-linux-ubuntu-22.04-amd64
verify-rc-source-python-macos-amd64
verify-rc-source-python-macos-arm64
verify-rc-source-python-macos-conda-amd64

danepitkin · 2023-08-22T22:30:24Z

@github-actions crossbow submit test-conda-python-3.10-hdfs*

github-actions · 2023-08-22T22:32:52Z

Revision: 8eb925d

Submitted crossbow builds: ursacomputing/crossbow @ actions-a1c4d5ead7

Task	Status
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1

… modules

danepitkin · 2023-08-23T18:31:46Z

@github-actions crossbow submit python

github-actions · 2023-08-23T18:36:07Z

Revision: 1a5b32b

Submitted crossbow builds: ursacomputing/crossbow @ actions-c0a5b0e9b9

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
python-sdist
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3
verify-rc-source-python-linux-almalinux-8-amd64
verify-rc-source-python-linux-conda-latest-amd64
verify-rc-source-python-linux-ubuntu-20.04-amd64
verify-rc-source-python-linux-ubuntu-22.04-amd64
verify-rc-source-python-macos-amd64
verify-rc-source-python-macos-arm64
verify-rc-source-python-macos-conda-amd64

danepitkin · 2023-08-23T19:58:11Z

CI looks good! No S3 intermittent failures reported after 2 full runs. All current failures are unrelated.

Ready to merge IMO.

jorisvandenbossche · 2023-08-24T06:34:33Z

Nice, thanks a lot Dane!

conbench-apache-arrow · 2023-08-25T13:57:18Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 175b2a2.

There were 4 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-08-24 17:21:34Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-05, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1
and 2 more (see the report linked below)

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…he pickle and cloudpickle modules (apache#37255) ### Rationale for this change Cloudpickle was not tested in most parts of the pyarrow test suite. Improving this coverage will make the Cython 3.0.0 upgrade cleaner as cloudpickle was failing in a few places where the default pickle module was not. This has been verified using Cython 0.29.36. ### What changes are included in this PR? * `__reduce__` methods that need to pass kwargs have been changed from classmethod to staticmethod * All pytests that pickle objects are parameterized to use both `pickle` and `cloudpickle` ### Are these changes tested? Yes, pytests run successfully with Cython 0.29.36 ### Are there any user-facing changes? No. * Closes: apache#37254 Authored-by: Dane Pitkin <dane@voltrondata.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

danepitkin requested review from kou and pitrou August 18, 2023 19:23

github-actions bot added Component: Python awaiting review Awaiting review labels Aug 18, 2023

danepitkin mentioned this pull request Aug 18, 2023

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

Merged

pitrou requested a review from jorisvandenbossche August 18, 2023 19:41

kou reviewed Aug 18, 2023

View reviewed changes

python/pyarrow/_dataset_parquet.pyx Outdated Show resolved Hide resolved

python/pyarrow/tests/parquet/test_dataset.py Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 18, 2023

danepitkin requested a review from AlenkaF August 21, 2023 14:21

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 21, 2023

danepitkin marked this pull request as draft August 22, 2023 00:57

jorisvandenbossche approved these changes Aug 22, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Aug 22, 2023

danepitkin marked this pull request as ready for review August 22, 2023 21:16

danepitkin force-pushed the danepitkin/pytest-cloudpickle branch from 06a21ac to 7710a86 Compare August 22, 2023 21:16

danepitkin added 7 commits August 23, 2023 14:31

Parametrize all pickling tests to use both the pickle and cloudpickle…

4f1bf30

… modules

Add connection timeout in s3fs tests

906b76b

Commit suggestions

3b7e01e

Fix hypothesis tests

e6387a5

Wait for minio server to start up

51db72e

Rebase

eb2314e

Fix hdfs tests

1a5b32b

danepitkin force-pushed the danepitkin/pytest-cloudpickle branch from 8eb925d to 1a5b32b Compare August 23, 2023 18:31

kou merged commit 175b2a2 into apache:main Aug 24, 2023
10 checks passed

kou removed the awaiting merge Awaiting merge label Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37254: [Python] Parametrize all pickling tests to use both the pickle and cloudpickle modules #37255

GH-37254: [Python] Parametrize all pickling tests to use both the pickle and cloudpickle modules #37255

danepitkin commented Aug 18, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Aug 18, 2023

danepitkin commented Aug 18, 2023 •

edited

Loading

danepitkin commented Aug 18, 2023

kou left a comment

danepitkin commented Aug 21, 2023 •

edited

Loading

danepitkin commented Aug 21, 2023 •

edited

Loading

danepitkin commented Aug 21, 2023

github-actions bot commented Aug 21, 2023

danepitkin commented Aug 21, 2023

github-actions bot commented Aug 21, 2023

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 22, 2023

jorisvandenbossche left a comment

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

danepitkin commented Aug 23, 2023

jorisvandenbossche commented Aug 24, 2023

conbench-apache-arrow bot commented Aug 25, 2023

GH-37254: [Python] Parametrize all pickling tests to use both the pickle and cloudpickle modules #37255

GH-37254: [Python] Parametrize all pickling tests to use both the pickle and cloudpickle modules #37255

Conversation

danepitkin commented Aug 18, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Aug 18, 2023

danepitkin commented Aug 18, 2023 • edited Loading

danepitkin commented Aug 18, 2023

kou left a comment

Choose a reason for hiding this comment

danepitkin commented Aug 21, 2023 • edited Loading

danepitkin commented Aug 21, 2023 • edited Loading

danepitkin commented Aug 21, 2023

github-actions bot commented Aug 21, 2023

danepitkin commented Aug 21, 2023

github-actions bot commented Aug 21, 2023

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 22, 2023

jorisvandenbossche left a comment

Choose a reason for hiding this comment

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 22, 2023

github-actions bot commented Aug 22, 2023

danepitkin commented Aug 23, 2023

github-actions bot commented Aug 23, 2023

danepitkin commented Aug 23, 2023

jorisvandenbossche commented Aug 24, 2023

conbench-apache-arrow bot commented Aug 25, 2023

danepitkin commented Aug 18, 2023 •

edited by github-actions bot

Loading

danepitkin commented Aug 18, 2023 •

edited

Loading

danepitkin commented Aug 21, 2023 •

edited

Loading

danepitkin commented Aug 21, 2023 •

edited

Loading