-
Notifications
You must be signed in to change notification settings - Fork 62
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
Cloning and serialization improvements #21
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
====================================== The main purpose of this commit is to increase the correctness when cloning and/or pickling instances of Problem, Solution, SolutionBatch, ObjectArray, and ImmutableContainer. When cloning and/or pickling, two new behaviors are introduced: (i) PyTorch tensors are cut off from their original storages (to avoid small slices from carrying their original storages with themselves); and (ii) self references within these mentioned objects are specially handled, and therefore, many unwanted RecursionError scenarios which could previously happen are now avoided. New function: `deep_clone(...)` =============================== This new function creates a deep copy of a Python container (which can contain other containers or PyTorch tensors or numpy arrays). While traversing through the container, any PyTorch tensors will be copied using the `.clone()` method, which means, in the newly created clone, the PyTorch tensors will be independent copies without extra storage. New base class: `Clonable` ========================== Any subclass of `Clonable` is expected to override the method `_get_cloned_state(...)`. This method is expected to return the state of the object in a dictionary. The `Clonable` base class provides definitions for the methods `__copy__(...)` and `__deepcopy__(...)` where the dictionary made by `_get_cloned_state(...)` is used to create the clone. New base class: `Serializable` ============================== This is a subclass of Clonable, which is meant to be used as a base class. In addition to the method definitions made by `Clonable` ( `__copy__` and `__deepcopy__`), this base class also defines `__getstate__` so that the method `_get_cloned_state(...)` also affects how the inheriting class is pickled. Classes inheriting from `Serializable` ====================================== The following classes now inherit from `Serializable`: - Problem - Solution - SolutionBatch - Distribution Each of these mentioned classes have their own definitions for `_get_cloned_state(...)`, and use `deep_clone(...)` for preparing their state dictionaries. Therefore, at the moment of cloning and pickling, the stored tensors are subject to the `.clone()` method and they are cut off from their original storages. With this feature, when a relatively small slice of a large SolutionBatch is cloned or pickled, the serialized slice will not contain the entire storage of the original batch. New base class: `ReadOnlyClonable` ================================== This is like `Clonable`, but requires the inheriting class to also define a method called `_get_mutable_clone()` so that the object knows how its mutable counterpart can be constructed. ImmutableContainer objects are now ReadOnlyClonable.
engintoklu
requested review from
Higgcz,
flukeskywalker and
NaturalGradient
September 28, 2022 15:20
NaturalGradient
approved these changes
Oct 3, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding the gym
compatibility issues described in #22 , every existing example appears to work as expected. I have also reviewed the code and have found no notable issues except for those already discussed and clarified by @engintoklu .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main purpose of this pull request is to increase the correctness when cloning and/or pickling instances of Problem, Solution, SolutionBatch, ObjectArray, and ImmutableContainer. When cloning and/or pickling, two new behaviors are introduced:
(i) PyTorch tensors are cut off from their original storages (to avoid small slices from carrying their original storages with themselves); and
(ii) self references within these mentioned objects are specially handled, and therefore, many unwanted RecursionError scenarios which could previously happen are now avoided.
New function:
deep_clone(...)
This new function creates a deep copy of a Python container (which can contain other containers or PyTorch tensors or numpy arrays). While traversing through the container, any PyTorch tensors will be copied using the
.clone()
method, which means, in the newly created clone, the PyTorch tensors will be independent copies without extra storage.New base class:
Clonable
Any subclass of
Clonable
is expected to override the method_get_cloned_state(...)
. This method is expected to return the state of the object in a dictionary. TheClonable
base class provides definitions for the methods__copy__(...)
and__deepcopy__(...)
where the dictionary made by_get_cloned_state(...)
is used to create the clone.New base class:
Serializable
This is a subclass of Clonable, which is meant to be used as a base class. In addition to the method definitions made by
Clonable
(__copy__
and__deepcopy__
), this base class also defines__getstate__
so that the method_get_cloned_state(...)
also affects how the inheriting class is pickled.Classes inheriting from
Serializable
The following classes now inherit from
Serializable
:Each of these mentioned classes have their own definitions for
_get_cloned_state(...)
, and usedeep_clone(...)
for preparing their state dictionaries. Therefore, at the moment of cloning and pickling, the stored tensors are subject to the.clone()
method and they are cut off from their original storages. With this feature, when a relatively small slice of a large SolutionBatch is cloned or pickled, the serialized slice will not contain the entire storage of the original batch.New base class:
ReadOnlyClonable
This is like
Clonable
, but requires the inheriting class to also define a method called_get_mutable_clone()
so that the object knows how its mutable counterpart can be constructed. ImmutableContainer objects are now ReadOnlyClonable.