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

Complete serialization overhaul #263

Merged
merged 99 commits into from
Jul 17, 2018
Merged

Complete serialization overhaul #263

merged 99 commits into from
Jul 17, 2018

Conversation

terrorfisch
Copy link
Member

The aim of this overhaul is to make Serialization more future proof and lay the groundwork for easier pulse management. Also solves #250 completely.

  • dict interface where ever it makes sense (StorageBackends i.e.)
  • Serialization based on the extenability of json.JSONEncoder and json.JSONDecoder. This leads to much less code in each PulseTemplate but requries that references are marked explicitly.
  • A PulseStorage class that also helps organizing pulses (draft)
  • A central registration for all Serializables to avoid identifier duplication if not explicitly disabled

Some of these are backward incompatible and need to be discussed in detail.

@terrorfisch terrorfisch requested a review from lumip May 23, 2018 11:14
@terrorfisch
Copy link
Member Author

Maybe introduce a new method to_dict for the new serialization interface and just dereprecate the old one.

lumip added 2 commits May 30, 2018 12:23
…oth transition.

New classes are now called JSONSerializableDecoder/-Encoder .
lumip
lumip previously requested changes May 30, 2018
Copy link
Contributor

@lumip lumip left a comment

Choose a reason for hiding this comment

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

In general most of the changes make a lot of sense, especially moving code from the old Serializer to JSONDecoder/-Encoder extensions. I would, however, rename the ExtendedJSONDecoder/-Encoder classes to a more specific name (e.g. JSONSerializableDecoder/-Encoder) but that's a minor thing (but would allow us to have old and new routines in co-existence for a transition period).

The only thing I really don't like here is the central pulse register. It makes object construction more inconvenient and isn't even guaranteed to detect duplicate ids: if a pulse with some id exists in the storage but is never loaded in an application, it will not appear in the register... we need to check for that in the PulseStorage or StorageBackend anyways, so there is no benefit to the register.

@lumip
Copy link
Contributor

lumip commented May 30, 2018

I also tried to think about a transition period where the old routines are still available but deprecated but I don't really want to rename the relevant methods in the new implementations (and can't in the old one) (to_dict is much less precise a name then get_serialization_data() )... it would be nice if Python would support method overloading but meh

@lumip
Copy link
Contributor

lumip commented May 30, 2018

Branch issues/250_serialization_backward_compatability contains my current state of implementing the corresponding methods in PTs for the new ecosystem but for now backward compatible (and deprecated).

lumip added 11 commits June 7, 2018 16:17
All now implement get_serialization_data and deserialize for old and new
serialization routines for a transition period.
Methods did not call Serializable::get_serialization_data() and thus did not inculde #type and #identifier keys in the returned dictionaries. Tests did not cover this.
Moved identifier_name and type_identifier_name from JSONSerializableEncoder/-Decoder to Serializable.

Signed-off-by: Lukas Prediger <lukas.prediger@rwth-aachen.de>
Implementation was inconsistent with _temporary_storage usage in other places.
DummySerializable now behaves as follows: Takes any number of keyword-arguments and stores them as class attributes. get_serialization_data() returns a dictionary with (key, value) pairs for all attributes given this way. This is compatible with previous usage (where only a "data" attribute of type Any was given and get_serialization_data() returned a dictionary with only key "data" and whatever given value).
Removed duplicated definition of DummySerializable further down the same file.
NestedDummySerializable now is also compatible with new serialization
routines and implements deserialize().
..to adapt syntax/type to behavior.

Slight change in DummySerializableTests.make_kwargs to extend testing of DummySerializable funcitonality a bit.
…format.

Pulse is read using old serialization routines from a source storage backend and destination storage backend to a new using the new routines.
Method should list all available serializables. No tests currently.
@lumip lumip added this to the New Serialization Routines milestone Jun 16, 2018
convert_pulses_in_storage converts all pulses in a given source StorageBackend
from the old serialization format into the new and stores them in a given
destination StorageBackend.
Both conversion methods now raise an exception if they would need to
overwrite identifiers already existing in the destination backend.
Commit also includes more extensive tests for both conversion methods as
well as some style corretions for already existing tests.
lumip and others added 3 commits July 12, 2018 14:32
…global registry after they are completely initialized.

Serializable now does not perform registration in __init__ but provides a method _register() which MUST be called by leaf-level subclasses. (This is probably also not a good solution in the long term as it effectively prevents further subclassing from those classes but it works for now).

Signed-off-by: Lukas Prediger <lukas.prediger@rwth-aachen.de>
@lumip lumip dismissed their stale review July 12, 2018 12:52

outdated

@lumip
Copy link
Contributor

lumip commented Jul 12, 2018

We should be about to wrap this up. Can merge into master after incorporating pull requests
#307 #306 #304 and thus closing issues #272, #296, #298, #301
and possibly merging #300 to address #299 .
The remaining issues #274 and #294 are not required for merge into master and can/will be addressed in the future.

terrorfisch and others added 18 commits July 12, 2018 15:08
Implementations of __len__ and __iter__ curently raise NotImplementedError s.

Signed-off-by: Lukas Prediger <lukas.prediger@rwth-aachen.de>
…on_script

Serialization conversion script
…istencies

Issues/272 pulse storage id inconsistencies
…tions_for_registry

Issues/308 consistent type annotations for registry
Increased test coverage for serialization
@terrorfisch terrorfisch changed the title [DO NOT MERGE] Complete serialization overhaul Complete serialization overhaul Jul 17, 2018
@terrorfisch
Copy link
Member Author

terrorfisch commented Jul 17, 2018

I think we can merge now.
@lumip any objections?

This will:
close #250 and
close #263

@lumip
Copy link
Contributor

lumip commented Jul 17, 2018

let's go

@lumip lumip merged commit b69341f into master Jul 17, 2018
@lumip lumip deleted the issues/250_serialization branch July 19, 2018 06:37
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