feat(bindings/python): support pickle [de]serialization for Operator by TennyZhuang · Pull Request #5324 · apache/opendal

TennyZhuang · 2024-11-15T02:00:38Z

Which issue does this PR close?

Partially finished #5316

Rationale for this change

Now an Operator can be serialized and deserialized via pickle, as a result, the Operator class can be used in multiprocessing context.

What changes are included in this PR?

In the PR, we store the arguments for Operator constructor, so that we can recover the Operator from pickle deserialization.

Are there any user-facing changes?

support pickle [de]serialization for Operator in python binding
support Operator to be used in multiprocessing context

@Zheaoli I'm not very familiar with Python binding, PTAL for the PR, thanks!

TennyZhuang · 2024-11-15T02:21:14Z

It seems like we should add a new capability MultiProcessingAccess to guard the behavior.

Zheaoli · 2024-11-15T17:50:26Z

bindings/python/tests/test_pickle_types.py

+    """
+    serialized = pickle.dumps(operator)
+    deserialized = pickle.loads(serialized)
+    assert repr(operator) == repr(deserialized)


For me, I think the ideal behavior for pickle [de]serialization is that the object has the same behavior/operation result.

So I suggest the test can be

data="abc" operator.write("/abc","abc") serialized = pickle.dumps(operator) del operator gc.collect() deserialized = pickle.loads(serialized) assert "abc"==deserialized.read("/abc")

Sounds good!

Zheaoli · 2024-11-15T17:51:06Z

bindings/python/src/operator.rs

+        })
    }

    fn __repr__(&self) -> String {


I think we need add extra __hash__ method to represent two object are equal.

It's hard to implement, we can't get internal information of Operator.

how about hash the scheme and the map?

repr is used for show what the object is looking at. Maybe not a good method for identify

I prefer don’t implement hash and eq for Operator, and remove the equal assertion based on repr.

(schema, map) can’t identify an Operator, leave eq and empty empty is better. Only the ID of PyObject can identify the Operator.

I prefer don’t implement hash and eq for Operator, and remove the equal assertion based on repr.

Agreed. ocore::Operator doesn't implement Eq ahd Hash too.

Zheaoli · 2024-11-15T17:51:47Z

It seems like we should add a new capability MultiProcessingAccess to guard the behavior.

SGTM, maybe Pickable would be better?

TennyZhuang · 2024-11-16T06:34:37Z

Pickable

IMO all operators can be serialized as pickle.

In most Python libraries, all objects can be serialized as pickle, but it may not be identical when deserialized. The behavior is acceptable.

We need the capability only for tests. We can ignore backends which doesn't support the capability in the test.

Xuanwo · 2024-11-16T07:21:05Z

I'm considering adding a shared capability to indicate whether this storage service can be shared across different processes or even different instances.

Based on this definition:

memory is not shared: it cannot be accessed by other processes.
sled is not shared: it has a unique lock that only one process can use.
fs is shared: different processes can access the same fs as long as they have it mounted.
s3 is shared: obviously.

I believe this capability is better than the following:

multi_processing_access: too long and doesn't align with exstsing capability naming style.
pickable: python only and doesn't reflect the root cause.

I don't like the idea that:

We need the capability only for tests

I view it as a design failure for OpenDAL to include something in the public API solely for testing purposes. shared is somewhat useful for users who want to understand what will happen if they try to share the same storage services across processes or instances.

Zheaoli · 2024-11-16T09:59:58Z

I'm considering adding a shared capability to indicate whether this storage service can be shared across different processes or even different instances.

SGTM

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

bindings/python/src/operator.rs:230

The method __getnewargs_ex__ might not correctly handle the conversion of kwargs to a Python dictionary. Ensure that kwargs are correctly converted to a Python dictionary.

fn __getnewargs_ex__(&self, py: Python) -> PyResult<PyObject> {

bindings/python/src/operator.rs:51

[nitpick] The naming of the private attributes __scheme and __map should be consistent with the convention used in the rest of the codebase.

__scheme: ocore::Scheme,

TennyZhuang · 2024-11-16T13:24:35Z

I don't like the idea that:

Please note that pickle doesn’t mean shared. The user can pickle the operator, close the operator, pass the bytes to another process and unpickle the operator. So we should implement pickle for every operator, both shared and unique.

What we only can do is disable the tests for unique operator.

Xuanwo · 2024-11-16T13:36:06Z

Please note that pickle doesn’t mean shared. The user can pickle the operator, close the operator, pass the bytes to another process and unpickle the operator. So we should implement pickle for every operator, both shared and unique.

That's correct. As you mentioned, we just need to avoid testing it in a shared manner (which implies that we can access the same storage service after unpickling) if this storage service doesn't shared.

I prefer to add the shared capability, which I believe is a comprehensive solution. However, I'm also open to the python binding just testing against specified services like s3, gcs, and fs if you feel that's sufficient.

TennyZhuang · 2024-11-16T13:46:33Z

In fact, there are two types of capabilities in all backends.

shared
Access rights can be shared, all services backends implement this, while embedded engines don't.
send
Access rights can be transferred
Almost all backends implement this, except for memory backend such as dashmap.

Of course, we don't need to be so strict and complicated, I support temporarily adding only shared capability.

Xuanwo · 2024-11-16T14:00:22Z

I support temporarily adding only shared capability.

Would you like to add this capability? I believe we can include them in the next 0.51 release.

TennyZhuang · 2024-11-16T14:12:05Z

I support temporarily adding only shared capability.

Would you like to add this capability? I believe we can include them in the next 0.51 release.

I can add it.

Please note that shared only indicates shared between processes but not shared between threads, it may be a little confusing if we will add backends which only support single-thread later.

However, I guess we will not support that in a while.

Xuanwo · 2024-11-16T14:21:17Z

Please note that shared only indicates shared between processes but not shared between threads, it may be a little confusing if we will add backends which only support single-thread later.

However, I guess we will not support that in a while.

Currently, we don't offer any services that aren't Send because Access requires services to be both Send and Sync. There are no plans to add such services, but we can revisit this decision if necessary.

TennyZhuang · 2024-11-16T14:28:23Z

Will test another PR use the CI flow, just ignore it.

Zheaoli

LGTM about the Python binding part

I will merge this after all ci green

Xuanwo

LGTM. Thank you @TennyZhuang for working on this and thanks @Zheaoli's review.

Please merge when you feel this PR is ok, @Zheaoli

feat(bindings/python): support pickle [de]serialization for Operator

5bd8d66

TennyZhuang requested review from Zheaoli and messense as code owners November 15, 2024 02:00

github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Nov 15, 2024

TennyZhuang mentioned this pull request Nov 15, 2024

new feature: new capability shared #5327

Closed

1 task

Zheaoli reviewed Nov 15, 2024

View reviewed changes

Xuanwo requested a review from Copilot November 16, 2024 12:53

Copilot AI reviewed Nov 16, 2024

View reviewed changes

TennyZhuang added 2 commits November 16, 2024 22:26

feat(core): add new cap shared

77906da

Merge branch 'feat-cap-shared' into pickle-support-for-python-operator

c1b186e

TennyZhuang added 3 commits November 16, 2024 22:35

add a missing file

0413fc3

Merge branch 'feat-cap-shared' into pickle-support-for-python-operator

83a6b70

refine tests

5021d23

TennyZhuang requested review from PsiACE, suyanhanx and tisonkun as code owners November 16, 2024 14:36

fix C binding

24461f8

fix java binding

5e67769

TennyZhuang mentioned this pull request Nov 16, 2024

feat(core): add new cap shared #5328

Merged

TennyZhuang added 5 commits November 16, 2024 22:49

Merge branch 'feat-cap-shared' into pickle-support-for-python-operator

13ef152

refine tests

6a19304

fix nodejs binding

85e3881

Merge branch 'feat-cap-shared' into pickle-support-for-python-operator

e5119a1

Merge branch 'main' into pickle-support-for-python-operator

8a3a69e

Zheaoli approved these changes Nov 16, 2024

View reviewed changes

Xuanwo approved these changes Nov 16, 2024

View reviewed changes

Xuanwo mentioned this pull request Nov 16, 2024

EPIC: Make OpenDAL Python The Greatest #5273

Closed

3 tasks

Zheaoli merged commit 73bbb85 into apache:main Nov 16, 2024

Comments

Conversation

TennyZhuang commented Nov 15, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

TennyZhuang commented Nov 15, 2024

Uh oh!

Zheaoli Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

TennyZhuang Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Zheaoli Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TennyZhuang Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Zheaoli Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TennyZhuang Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xuanwo Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Zheaoli commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TennyZhuang commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xuanwo commented Nov 16, 2024

Uh oh!

Zheaoli commented Nov 16, 2024

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

TennyZhuang commented Nov 16, 2024

Uh oh!

Xuanwo commented Nov 16, 2024

Uh oh!

TennyZhuang commented Nov 16, 2024

Uh oh!

Xuanwo commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TennyZhuang commented Nov 16, 2024

Uh oh!

Xuanwo commented Nov 16, 2024

Uh oh!

TennyZhuang commented Nov 16, 2024

Uh oh!

Zheaoli left a comment

Choose a reason for hiding this comment

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Zheaoli Nov 15, 2024 •

edited

Loading

Zheaoli Nov 16, 2024 •

edited

Loading

TennyZhuang Nov 16, 2024 •

edited

Loading

Zheaoli commented Nov 15, 2024 •

edited

Loading

TennyZhuang commented Nov 16, 2024 •

edited

Loading

Xuanwo commented Nov 16, 2024 •

edited

Loading