European-XFEL / karabo-bridge-py

Tools to allow data exchange with Karabo, in particular streaming of data
BSD 3-Clause "New" or "Revised" License
9 stars 4 forks source link

Public interface for serializers #56

Closed tmichela closed 4 years ago

tmichela commented 4 years ago

This moves the serializer implementation from extra-data, the aim is to allow 3rd party tools to use it (as requested by @zhujun98 ).

Along the way:

from karabo_bridge import serialize, deserialize

d, m = deserialize(serialize(data, metadata))
zhujun98 commented 4 years ago

In general LGTM from my side. The new serialize implementation is almost a mirror of https://github.com/European-XFEL/EXtra-data/blob/master/extra_data/export.ZMQStreamer._serialize`.

Thanks for implementing this. I will use it in EXtra-foam, particularly for the extension.

tmichela commented 4 years ago

The new serialize implementation is almost a mirror of

Yes, that's the idea. I will then update extra-data to use this version

takluyver commented 4 years ago

LGTM.

One small API question: there are a couple of configuration parameters (protocol_version, dummy_timestamps) which are likely to be the same for many calls. Maybe it would be useful to have a serialiser class which would take those arguments and have a .serialise() method, separating input and configuration. But to be honest, I probably wouldn't bother for just 2 parameters, and we can always add it later if we want more options.

zhujun98 commented 4 years ago

I am also in favor of class.

tmichela commented 4 years ago

Hum, I originally wrote a class, but I found a bit silly seeing a class with a single method and a single argument, so I changed it... And especially because the arguments are mostly there for backward compatibility. I believe that nobody will ever use the version 1.0 and the fake timestamps won't make sens once we use the timestamp from the hdf5 files...

I personally prefer having a simple function, but if you both prefer a class, I can change it back

takluyver commented 4 years ago

With just these two config parameters, I'd probably leave it as a single function, but I don't have a strong preference either way.

tmichela commented 4 years ago

Thanks both for the review. I'll then leave it as functions. This can changed it it becomes more customizable...