pymeasure / pyleco

Python implementation of the Laboratory Experiment COntrol (LECO) protocol
MIT License
9 stars 3 forks source link

Add binary payload handling to PyLECO #82

Closed BenediktBurger closed 2 months ago

BenediktBurger commented 3 months ago

Implements the transport of binary data in additional frames, related to How to transmit binary data (https://github.com/pymeasure/leco-protocol/issues/65).

Idea:

Implementation ideas.

Open questions:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 98.79518% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.52%. Comparing base (cd0bf05) to head (6ba4c19). Report is 1 commits behind head on main.

Files Patch % Lines
pyleco/utils/listener.py 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #82 +/- ## ========================================== + Coverage 89.04% 89.52% +0.48% ========================================== Files 36 36 Lines 2902 2931 +29 Branches 355 361 +6 ========================================== + Hits 2584 2624 +40 + Misses 267 256 -11 Partials 51 51 ``` | [Flag](https://app.codecov.io/gh/pymeasure/pyleco/pull/82/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymeasure) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/pymeasure/pyleco/pull/82/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymeasure) | `89.52% <98.79%> (+0.48%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymeasure#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

BenediktBurger commented 3 months ago

@seb5g, in this PR I experiment with the transport of binary data via PyLECO. As it is a feature you preferred over the base64 encoding, I want to make you aware.

BenediktBurger commented 3 months ago

Idea for:

If not, how to know, whether the binary data is the desired response instead of the json response? A return value of None with additional payload frames is a strong indicator, but not exclusive to this scenario. Is it sufficient?

Just add a parameter, whether to return a binary value.

BenediktBurger commented 3 months ago

@untzag, @ksunden, @VigneshVSV this could be also of interest for your own projects.

BenediktBurger commented 3 months ago

I ask for feedback regarding the current implementation:

This is how you call the method some_funny_method with additional binary payload (for the method):

# on the remote side (on MessageHandler):
def some_funny_method(self):
    additional_payload = self.current_message.payload[1:]
    # do something with additional_payload
    self.additional_response_payload = [b"whatever", b"to return", b"binary"]
    return None

# on the requester side:
ask("receiver", method="some_funny_method", additional_payload=[b"123", b"456], extract_additional_payload=True) 
# returns: [b"whatever", b"to return", b"binary"]

The parameter extract_additional_payload (independent of the additional payload on the way to the remote contact) will return the binary return value of some_funny_method, if the JSON response value is None.

VigneshVSV commented 3 months ago

I think its a tricky question in general as seen by some questions you raised in the issue.

Currently I allow either returning either of non-serialized data or serialized data only. Not both. Technically a user could do


def my_server_side_method_returning_bytes(self):
    return memoryview(self.my_large_numpy_array)

def my_server_side_method_normal(self):
    return 5, 'my name', self.foo_list

i.e. for the user, the usage of methods is the same and there is no need to access an additional request or response object. Although I dont see what is wrong with that either. Some HTTP servers which work based on decorating functions, like flask, allow to access the request object through some global variable. Same can be then done for response.

Nevertheless, since I allow only one type (either of bytes, bytearray or memoryview) or of a other python types which needs to be serialized, its a little easier for me to place them in different indices of the multipart message to know what was the reply. In this way, the client just checks which part of the multipart message containing return values has size greater than 0. The user given pre serialized part or the RPC server serialized part?

Of course, the ideal and best case is to have both so that the user can return any damn return value. This is the right approach. May be its best to reserve a tuple of size 2 as a fixed internal return value scheme. Just before serializing, the type can be checked to be tuple and if its of length 2, then one can proceed to inspect the types each element of the element. If one turns out to be byte type, then dont serialize it again as that will lead to serilization error:


def my_mixed_returning_server_method(self):
    return { 1: self.some_foo_list, 3 : 42 }, memoryview(self.my_large_numpy_array)

Within the RPC server:

    ret = func(*args, **kwargs)
    if isinstance(ret, tuple) and len(ret) == 2: # return value containing one normal part and one byte part
          if isinstance(ret[1], (bytes, bytearray, memoryview)): # check if its really a byte part
                 self.serializer.dumps(ret[0]) # ret[1] is already serialitzed
          else:
                self.serializer.dumps(ret)  # serialize normal data given in tuple of length 2 
          # as the return value was indeed a normal tuple
   elif not isinstance(ret, (bytes, bytearray, memoryview)): # covers the case of prevent serialization of single 
        # return value which is of bytes type
       self.serializer.dumps(ret) # some other data including tuples of lower or higher length

Of course the dumps return value must be assign to a place within the multipart message.

This is the way I would do it.

BenediktBurger commented 3 months ago

Thanks @VigneshVSV for your extensive answer. Part of the challenge is the currently used rpc server. The server sees only the json request and returns a json response. I have to handle the binary things outside (with these external variables).

BenediktBurger commented 3 months ago

I have an idea (inspired by you): If the json frame is empty, the other frames are the return value.

BenediktBurger commented 3 months ago

Newest design:

VigneshVSV commented 3 months ago

Regarding the ask RPC caller, I would still keep in mind that the server side method works transparently irrespective of whether it is called within another function or method, or from client side with the ask. I am not sure if the change you made affects this.

BenediktBurger commented 3 months ago

@VigneshVSV , thanks for your comments, they are helpful. Registering a method makes it available via RPC. Therefore, you have to regsiter it, before you can call it via ask.

BenediktBurger commented 3 months ago

A small change:

Currently the you have to tell the ask method, whether to expect binary data or not. I do not consider this as a major problem:

seb5g commented 3 months ago

I'm not using really pyleco at the moment as you are the one who implemented it in pymodaq... so I still have difficulties to really understand how all this works. But for sure, everything within pymodaq can be binary serialized so direct emission of such data (frame as you call it?) would be better. I'm not sure how i could really help in the design...

BenediktBurger commented 3 months ago

Thanks for your comment @seb5g. I appreciate it.

BenediktBurger commented 2 months ago

Summary: