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

ReadOnlyDict/List pickle compatibility and serialization tests #737

Merged
merged 13 commits into from
Oct 16, 2020

Conversation

shwang
Copy link
Contributor

@shwang shwang commented May 30, 2020

Makes ReadOnly{Dict,List} picklable. Also adds tests for pickle, json serialization.

The main changes to the read-only custom types were adding a __reduce__ method and simplifying the various __init__ calls a bit by using using explicit class.__init__ calls rather than super().__init__ (h/t @Qwlouse #508 (comment) ).

Closes #499. Closes #738.

@shwang
Copy link
Contributor Author

shwang commented May 31, 2020

CI is outputting ##[warning]An image label with the label macos-10.13 does not exist. on cancelled jobs

@stale
Copy link

stale bot commented Aug 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 29, 2020
@stale stale bot closed this Sep 5, 2020
sacred/config/custom_containers.py Outdated Show resolved Hide resolved
sacred/config/custom_containers.py Outdated Show resolved Hide resolved
sacred/config/custom_containers.py Outdated Show resolved Hide resolved
@thequilo thequilo reopened this Oct 15, 2020
@stale stale bot removed the stale label Oct 15, 2020
@thequilo
Copy link
Collaborator

And an item open for discussion that I already mentioned in #770: Should we make the pickled type a read-only container or list and dict?

@thequilo
Copy link
Collaborator

The configuration for checks on OSX was updated on the master branch. They should pass after you merge the master into your PR.

@boeddeker
Copy link
Contributor

And an item open for discussion that I already mentioned in #770: Should we make the pickled type a read-only container or list and dict?

As far as I know, are the read only types introduced to prevent accidental modifications of the config
and they should act like the build in types.
So

config = deepcopy(config)
config[...] = ...

should work, because it is obvious that the user took care to not change the global config.
If can only work, then the pickled type is list and dict

@thequilo
Copy link
Collaborator

As far as I know, are the read only types introduced to prevent accidental modifications of the config
and they should act like the build in types.

Yes, that's right. The question is if we want to allow

config = pickle.loads(pickle.dumps(config))
config[...] = ...

In most cases that I can imagine, we want to lose the read-only properties when pickling (e.g., when saving to disk), but in some scenarios (e.g., multiprocessing, ray-tune) it might be safer to keep the read-only status.
In #663 it was suggested to make the experiment pickleable, which would enable passing it between processes. In that case, we definitely want to keep things read-only.

@shwang
Copy link
Contributor Author

shwang commented Oct 15, 2020

Having ReadOnlyDict pickled as dict seems unexpectedly asymmetrical to me.

As @thequilo alluded to in this comment, I think it's better to have the user make a copy first to dict if they want to explicitly save as dict. We already have to do the same thing if they want to make modifications before pickling.

@thequilo
Copy link
Collaborator

@shwang Thank you!

@thequilo thequilo merged commit fc8e8d7 into IDSIA:master Oct 16, 2020
@shwang shwang deleted the shwang/readonly-picklable branch October 16, 2020 06:34
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.

Pickled Object created by Python 3.6 cannot be loaded in Python 3.7 ReadOnlyDict not pickleable
4 participants