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

Binary Serialization Integration #16

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

elenialex
Copy link

This pull request included two primary changes. The addition of classes to support new parsing methods, and the integration of necessary code for user selection of encoding method.

Message classes for supporting variation in data format:

  • Parent Class: MessageCodec: supports child classes for various processes
  • Child Classes: JsonMessageCodec & BinaryMessageCodec : Supports various types of message formats.

Integration of code for User selection of encoding method:

  • Integration of encoder selection type into Transport class.
  • Integration of serialization based on Transport selected encoder

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

_Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@elenialex
Copy link
Author

hi @gonzalocasas ! I think I added the basics for the binary serialization we talked about. It will be great if you can have a look !

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

It's looking good! I added some request for changes and I would add also that it'd be nice to add some unit tests to the PR since at this point, it would be relatively trivial to unit test this new code. If you use Copilot for your coding, it can actually generate a good starting point for them.

@@ -43,6 +45,7 @@ def id_counter(self):
return self._id_counter

def publish(self, topic, message):
self.codec.encode(message)
pass
Copy link
Member

Choose a reason for hiding this comment

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

This pass has no effect now, should be removed. But perhaps more importantly, this is probably not where we should be calling the encoding. This method is empty because the implementation is on the sub-classes of transport. So, I would leave it empty.

@@ -58,6 +61,150 @@ def unadvertise(self, topic):
pass


class MessageCodec:
Copy link
Member

Choose a reason for hiding this comment

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

In order to maintain IronPython compat, the class def should explicitly inherit from object:

Suggested change
class MessageCodec:
class MessageCodec(object):

Once we move fully to cpython, we can simplify this again.

@@ -1,5 +1,6 @@
from compas.data import json_dumps
from compas.data import json_loads
import msgpack
Copy link
Member

Choose a reason for hiding this comment

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

Since msgpack is not available for ironpython, we should either try..except this import or check if compas.IPY (and subsequently disable binary serialization if that's the case).

Comment on lines +138 to +141
if isinstance(message, Message):
data = message.data
else:
data = message
Copy link
Member

Choose a reason for hiding this comment

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

This logic of detecting the message type is a tad more complex (there's one more case to handle) and it should be handled centrally, so I would suggest we change Topic._message_to_json to Topic._message_to_data with the following implementation:

    def _message_to_data(self, message):
        """Convert a message to a data representation ready to be encoded.

        Normally, this method expects sub-classes of ``Message`` as input.
        However, it can deal with regular dictionaries as well as classes
        implementing the COMPAS data framework.
        """
        try:
            data = message.data
        except (KeyError, AttributeError):
            try:
                data = message.__data__
            except (KeyError, AttributeError):
                data = dict(message)
        return data

and this method can be much simpler:

    def encode(self, data):
        return json_dumps(data)

... (cont'd)

Comment on lines +184 to +188
if isinstance(message, Message):
data = message.data
else:
data = message
return msgpack.packb(data)
Copy link
Member

Choose a reason for hiding this comment

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

...and as mentioned in the other encode, this would also change to just do return msgpack.packb(data)

Comment on lines +63 to +64
encoded_message = self.codec.encode(message)
self.client.publish(topic.name, encoded_message)
Copy link
Member

Choose a reason for hiding this comment

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

Based on the comments above, this would change to call the conversion to data from the topic, and then use the codec for encoding:

Suggested change
encoded_message = self.codec.encode(message)
self.client.publish(topic.name, encoded_message)
data = topic._message_to_data(message)
encoded_message = self.codec.encode(data)
self.client.publish(topic.name, encoded_message)

Copy link
Member

Choose a reason for hiding this comment

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

the same would need to be done in mqtt_cli.py for this to work on IronPython

Comment on lines +90 to +94
decoded_message = self.codec.decode(msg.payload)
callback(decoded_message)

# msg = topic._message_from_json(msg.payload.decode())
# callback(msg)
Copy link
Member

Choose a reason for hiding this comment

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

Handling decoding needs to follow the same approach as the one commented on the encoding, because there's this custom type parsing supported in there, so, we need to change Topic._message_from_json to this:

    def _message_from_data(self, data):
        """Converts a decoded data object back into a message instance."""
        return self.message_type.parse(data)

and this needs to change a bit as well:

Suggested change
decoded_message = self.codec.decode(msg.payload)
callback(decoded_message)
# msg = topic._message_from_json(msg.payload.decode())
# callback(msg)
decoded_data = self.codec.decode(msg.payload.decode())
decoded_message = topic._message_from_data(decoded_data)
callback(decoded_message)

Copy link
Member

Choose a reason for hiding this comment

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

and the mqtt_cli.py would change as well to this sequence of calls

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

:)

@gonzalocasas
Copy link
Member

Now that the integration tests ran, you can see the cases in which the extra case fails to parse the message:
https://github.com/compas-dev/compas_eve/actions/runs/11976510272/job/33395355496?pr=16#step:4:109

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.

2 participants