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

[Data] Support PyArrow 14.0.1 #41036

Merged
merged 12 commits into from
Nov 20, 2023
Merged

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Nov 8, 2023

Why are these changes needed?

Arrow nightly tests are failing because our custom tensor extension uses pyarrow.PyExtensionType, and apache/arrow#38608 deprecated pyarrow.PyExtensionType in favor of pyarrow.ExtensionType. This PR updates our tensor extension to use the now-recommended APIs.

https://buildkite.com/ray-project/postmerge/builds/1593#018baae8-726d-451c-b4e4-db9e95113b81

Related issue number

Fixes #41139

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani bveeramani marked this pull request as draft November 8, 2023 22:15
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
if: build.env("BUILDKITE_PIPELINE_ID") == "0189e759-8c96-4302-b6b5-b4274406bf89"
# TODO(bveeramani): remove soft_fail when nightly tests are stable
soft_fail: true
# if: build.env("BUILDKITE_PIPELINE_ID") == "0189e759-8c96-4302-b6b5-b4274406bf89"
Copy link
Member Author

Choose a reason for hiding this comment

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

Will uncomment this after CI passes

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Copy link
Member Author

Closing for now. Team can pick up when we decide to prioritize #41139

@bveeramani bveeramani closed this Nov 14, 2023
@bveeramani bveeramani reopened this Nov 15, 2023
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani bveeramani changed the title [Data] Fix Arrow nightly tests [Data] Support PyArrow 14.0.1 Nov 20, 2023
@bveeramani bveeramani marked this pull request as ready for review November 20, 2023 06:25
@bveeramani bveeramani merged commit 32ba7c9 into ray-project:master Nov 20, 2023
17 checks passed
bveeramani added a commit that referenced this pull request Nov 20, 2023
To test that Arrow nightly test pass, #41036 temporarily moved the Arrow nightly tests to pre-merge (see this comment). But, we forgot to revert the change before merging the PR.

This PR moves the Arrow nightly tests back to post-merge.

---------

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Comment on lines +78 to +79
elif parse_version(pyarrow_version) >= parse_version("14.0.1"):
pa.PyExtensionType.set_auto_load(True)
Copy link

Choose a reason for hiding this comment

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

In case you are not aware, this is making PyArrow vulnerable to https://www.cve.org/CVERecord?id=CVE-2023-47248

Choose a reason for hiding this comment

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

I understand that you want to keep it working to read files created by older versions of Ray (because you know those extension types are safe), but the problem with this general enabling of PyExtensionType is that just having ray imported opens the security hole for your users also when reading other files.

A possible alternative would be to register a PyExtensionType class that uses a custom unpickler that only allows to unpickle your own type, and raises an error for anything else.
A custom unpickler could look like:

import io
import pickle

class RestrictedUnpickler(pickle.Unpickler):

    def find_class(self, module, name):
        if (module, name) == ("ray.air.util.tensor_extensions.arrow", "ArrowTensorType"):
            from ray.air.util.tensor_extensions.arrow import ArrowTensorType
            return ArrowTensorType
        elif (module, name) == ("pyarrow.lib", "type_for_alias"):
            from pyarrow import type_for_alias
            return type_for_alias
        else:
            raise pickle.UnpicklingError("loading '%s.%s' is forbidden" % (module, name))

RestrictedUnpickler(io.BytesIO(serialized)).load()

That was my quick attempt for ArrowTensorType based on limited knowledge of the code base; it might be too restrictive in general (I don't know which pyarrow data types are allowed). But it should be possible to create something like this that works for all your extension types while still restricting any random pickled data.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorisvandenbossche thanks for the guidance. How would we make PyArrow use the custom unpickler?

Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@pitrou and then we'd use the custom unpickler in __arrow_ext_deserialize__?

Copy link

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha! Thanks!

Choose a reason for hiding this comment

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

You can also take a look at pyarrow's own implementation of PyExtensionType (https://github.com/apache/arrow/blob/43f4c286fef92b709853eefef207a50aefd9f5d3/python/pyarrow/types.pxi#L1700-L1742), and then re-register your own version like the pyarrow-hotfix does.

ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Nov 29, 2023
Arrow nightly tests are failing because our custom tensor extension uses pyarrow.PyExtensionType, and apache/arrow#38608 deprecated pyarrow.PyExtensionType in favor of pyarrow.ExtensionType. This PR updates our tensor extension to use the now-recommended APIs.

---------

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Nov 29, 2023
To test that Arrow nightly test pass, ray-project#41036 temporarily moved the Arrow nightly tests to pre-merge (see this comment). But, we forgot to revert the change before merging the PR.

This PR moves the Arrow nightly tests back to post-merge.

---------

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Data] Ray Data doesn't work with Arrow 14.0.1
6 participants