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

Fix cloudpickle sentinel cloning #3825

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Conversation

cgarciae
Copy link
Collaborator

@cgarciae cgarciae commented Apr 4, 2024

What does this PR do?

Uses isinstance instead of is to check for _Sentinel instances to avoid non uniqueness issues when using cloudpickle (creates a new _Sentinel instance for _unspecified_parent).

@cgarciae cgarciae requested a review from superbobry April 4, 2024 10:19
a = MyModule()

# Fails with ValueError: parent must be None, Module or Scope:
UnpickledMyModule = cloudpickle.loads(cloudpickle.dumps(MyModule))
Copy link
Member

Choose a reason for hiding this comment

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

Why does cloudpickle pickle recreate the sentinel? Maybe you just need to define __reduce__ for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implemented it using __reduce__, thanks for the tip!

@cgarciae cgarciae force-pushed the linen-cloudpickle-issue branch from 15c8594 to d7c7c05 Compare April 5, 2024 07:58
@cgarciae cgarciae requested a review from superbobry April 5, 2024 07:59
@cgarciae cgarciae force-pushed the linen-cloudpickle-issue branch from d7c7c05 to d00dfc7 Compare April 10, 2024 07:06
@copybara-service copybara-service bot merged commit 1f39ba2 into main Apr 10, 2024
21 checks passed
@copybara-service copybara-service bot deleted the linen-cloudpickle-issue branch April 10, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants