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

Allow blobs and file systems to pickle #479

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

ghidalgo3
Copy link
Contributor

@ghidalgo3 ghidalgo3 commented Jun 21, 2024

I am also running into a similar issues as #477, trying to fix it by influencing the pickling behavior of blobs and file systems.

@ghidalgo3 ghidalgo3 marked this pull request as draft June 21, 2024 16:46
@ghidalgo3 ghidalgo3 changed the title initial implementation Allow blobs and file systems to pickle Jun 21, 2024
@alex-rakowski
Copy link

alex-rakowski commented Jun 26, 2024

Awesome, thanks for tackling this. I installed from this branch and it seems to have fixed my issue, is there any reason this is still a draft?

@ghidalgo3
Copy link
Contributor Author

Thanks for trying it out! Just forgot to continue working on this, I will publish it for review shortly.

@ghidalgo3 ghidalgo3 marked this pull request as ready for review June 29, 2024 16:10
@fjetter
Copy link

fjetter commented Jul 8, 2024

Could we get a review from a maintainer? (Maybe @TomAugspurger @martindurant ?)

Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

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

Given that the non-pickle code path is unchanged from before, and that pickling now also works, I'm OK with this change.

I have a couple of questions for consideration.

adlfs/spec.py Show resolved Hide resolved
adlfs/spec.py Show resolved Hide resolved
adlfs/tests/conftest.py Show resolved Hide resolved
adlfs/tests/test_pickling.py Show resolved Hide resolved
@TomAugspurger TomAugspurger merged commit 3bd3d09 into fsspec:main Jul 8, 2024
4 checks passed
@TomAugspurger
Copy link
Contributor

Thanks @ghidalgo3! I'll look into making a release with this shortly.

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.

5 participants