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

Allowing checkpointers' serializers to be asynchronous #543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wpoynter
Copy link

@wpoynter wpoynter commented Oct 2, 2024

Updated the SerializationProtocol interface to allow dumpsTyped and loadsTyped to be run synchronously or asynchronously.
Applied await to calls of each function throughout the existing checkpoint savers and tests.

This should allow future users to provide asynchronous serialization and deserialization for their savers, without breaking any current implementations.

@jacoblee93
Copy link
Collaborator

jacoblee93 commented Oct 2, 2024

This will break folks using custom checkpointers - it's early days but I'd prefer not to. Will think through a backwards compatible way to do this.

@wpoynter
Copy link
Author

wpoynter commented Oct 4, 2024

This will break folks using custom checkpointers - it's early days but I'd prefer not to. Will think through a backwards compatible way to do this.

@jacoblee93 Not a problem, but can you explain how this will break existing users with custom checkpointers? I think I am missing something that it would be worth me learning.

@jacoblee93
Copy link
Collaborator

I mean this will be breaking if you've implemented your own custom checkpointer (which I know a few have) since they'll need to change their code to await serde methods.

@hinthornw hinthornw changed the title Allowing checkpoint savers to be asynchronous Allowing checkpointers' serializers to be asynchronous Oct 7, 2024
@benjamincburns
Copy link
Contributor

benjamincburns commented Oct 11, 2024

This will break folks using custom checkpointers - it's early days but I'd prefer not to. Will think through a backwards compatible way to do this.

I agree with and support your preference to avoid a breaking change here. That said, if a breaking change is necessary, I think it would be a bit cleaner to remove the serialization stuff from BaseCheckpointSaver altogether, as this seems like it's more fitting as an internal detail of the concrete implementation, rather than a requirement of all classes that extend BaseCheckpointSaver.

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.

3 participants