Refactor message serialisation and deserialisation #197

milanmlft · 2023-12-19T15:22:20Z

I encountered some friction while adding the project_name and omop_es_timestamp fields to the RabbitMQ messages from the log file for UCLH-Foundry/the-rolling-skeleton#159 because the serialise() function was getting quite bloated and ruff started to complain about too many parameters. I also had to change code in many different places, indicating information leakage.

So I decided to create dedicated classes for Message in core (while also getting rid of the utils name). The Message class is responsible for defining which fields we expect to be present and has a serialisation method.
The serialisation and deserialisation are handled by jsonpickle.

In addition, serialisation is only done at the last instance when publishing to the queue, to avoid repeated serialisation/deserialisation steps, by passing around Message objects instead of bytes.

Hope this will make future changes a bit more streamlined.
Would love to receive some feedback on this! 🙏

Warning

This PR is meant to be merged in #186

Addiing `Message` and `SerialisedMessage` classes in attempt to improve information hiding and decoupling.

…g API

stefpiatek

Looks fun, and agreed one defining a message type for publishing to the queue.

I have some thoughts - let me know what you think (open for discussion, may not necessarily be the best solution)

cli/src/pixl_cli/main.py

pixl_core/src/core/patient_queue/message.py

This also removes the need for the `SerialisedMessage` class

…ssert the `study_datetime`

…test_deserialise()` which deserialises the entire object

…_datetime` instead

This reverts commit 8719153.

…bytes

…handle serialisation

…tead of bytes" This reverts commit 0e4fce4.

stefpiatek

Thanks for refactoring this Milan, looks a lot better and out abstractions are nicer

cli/src/pixl_cli/main.py

cli/tests/test_messages_from_parquet.py

pixl_core/src/core/patient_queue/producer.py

cli/tests/test_messages_from_parquet.py

pixl_core/src/core/patient_queue/message.py

pixl_ehr/src/pixl_ehr/_processing.py

Also add a note about why we ignore ruff rule S301

… parameter and deserialise

milanmlft · 2023-12-20T16:17:32Z

pixl_core/src/core/patient_queue/subscriber.py

-                    await callback(message.body)
+                    await callback(deserialise(message.body))


Warning
This hasn't been tested as it's not coved by any of the current unit tests.

Requires better integration tests, so should probably be done in https://github.com/UCLH-Foundry/the-rolling-skeleton/issues/70

… of bytes

* Add -e to pip install command in cli test * Added flag for csv=True and ran tests for cli * Added test for parquet directory and messages_from_parquet func * Use resources fixture to find test parquest files. Make input options clearer. Tests will still fail due to missing private parquet files. * Add synthetic "private" OMOP data taken from the issue * Add dependency for reading parquet files * Add procedure occurrence id column to message. Use pandas merge instead of join. * Remove option for CSV cohort file * formatting changes * Linting fixes * Fix other uses of modified method * ruff fix * Downstream class needs new attribute as well * Add type annotations to `test_messages_from_parquet()` * Add project name and OMOP ES timestamp to rabbitmq messages * Update `serialise()` callers with new arguments * Add `project_name` and `omop_es_timestamp` fields to `ImagingStudy` class * Refactor message serialisation and deserialisation (#197) * Refactor message serialization and deserialization Addiing `Message` and `SerialisedMessage` classes in attempt to improve information hiding and decoupling. * Rename `utils.py` -> `message.py` * Add `decode()` method for `SerialisedMessage` * Update docstring * Use new classes in message testing * Refactor message processing in the CLI * Refactor `process_message` to use `SerialisedMessage` class in EHR API * Refactor `process_message` to use `SerialisedMessage` class in imaging API * Fix `ImagingStudy` initalisation in `ImagingStudy.from_message()` * Fix imports * Fix test: access serialised message bodies * Turn `Message` into a `dataclass` * Fix failing tests * Use `jsonpickle` for (de)serializing messages This also removes the need for the `SerialisedMessage` class * Fix `test_deserialise_datetime()` so it uses the `Message` class to assert the `study_datetime` * Add `study_datetime` property for `Message` * No need to test deserialising individual fields, already covered by `test_deserialise()` which deserialises the entire object * Remove `study_date_from_serialised()`, use the class attribute `study_datetime` instead * Revert "Add `study_datetime` property for `Message`" This reverts commit 8719153. * Remove `Messages` class, use `list[Message]` instead * Add type checking for messages parsed from parquet input * Update `test_messages_from_parquet()` to use JSON strings instead of bytes * Update `PixlProducer.publish()` to use a list of Message objects and handle serialisation * Convert JSON string to bytes when serialising * Revert "Update `test_messages_from_parquet()` to use JSON strings instead of bytes" This reverts commit 0e4fce4. * `PixlProducer.publish()` should take a `list[Message]` as input in tests * Update EHR API to use new `Message` design * Update imaging API to use new `Message` design * Update deserialise function to accept bytes-encoded JSON string * Assert messages against list of `Message`s * Print dataclass in logs * `jsonpickle.decode()` can handle bytes so no need to decode first Also add a note about why we ignore ruff rule S301 * Make `deserialisable` a keyword only argument * Copilot forgot to convert dates to datetimes 🥲 * Refactor PixlConsumer run method to accept Message object as callback parameter and deserialise * Update consumer in `test_subscriber` to accept Message object instead of bytes * Format README * Update documentation with new parquet inputs * Update system test to use parquet files * Move `extract_summary.json` one level up and update docs * Change `parquet-dir` to a click argument instead of option * `click.argument` doesn't have a `help` parameter Document `parquet-dir` in the docstrings instead * Slim down parquet test files to only those needed * Raise more appropriate exceptions for missing file or directory * Print log message about messages created during regular runs * Update error message for parquet files --------- Co-authored-by: ruaridhg <ruaridhg@users.noreply.github.com> Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk> Co-authored-by: Milan Malfait <m.malfait@ucl.ac.uk>

milanmlft added 9 commits December 19, 2023 12:10

Refactor message serialization and deserialization

5cf1111

Addiing `Message` and `SerialisedMessage` classes in attempt to improve information hiding and decoupling.

Rename utils.py -> message.py

2299519

Add decode() method for SerialisedMessage

0bfcab0

Update docstring

b9e4a59

Use new classes in message testing

558f694

Refactor message processing in the CLI

fef8779

Refactor process_message to use SerialisedMessage class in EHR API

e239299

Refactor process_message to use SerialisedMessage class in imagin…

596e85c

…g API

Fix ImagingStudy initalisation in ImagingStudy.from_message()

7f160a2

milanmlft requested review from jeremyestein and stefpiatek December 19, 2023 15:22

Fix imports

31a6f29

stefpiatek reviewed Dec 19, 2023

View reviewed changes

cli/src/pixl_cli/main.py Outdated Show resolved Hide resolved

pixl_core/src/core/patient_queue/message.py Show resolved Hide resolved

pixl_core/src/core/patient_queue/message.py Outdated Show resolved Hide resolved

pixl_core/src/core/patient_queue/message.py Outdated Show resolved Hide resolved

stefpiatek reviewed Dec 19, 2023

View reviewed changes

pixl_core/src/core/patient_queue/message.py Outdated Show resolved Hide resolved

milanmlft added 16 commits December 19, 2023 16:53

Fix test: access serialised message bodies

9a8f5a9

Turn Message into a dataclass

aa78318

Fix failing tests

cc5b12c

Use jsonpickle for (de)serializing messages

b8dfad0

This also removes the need for the `SerialisedMessage` class

Fix test_deserialise_datetime() so it uses the Message class to a…

f1b848d

…ssert the `study_datetime`

Add study_datetime property for Message

8719153

No need to test deserialising individual fields, already covered by `…

b08a9c4

…test_deserialise()` which deserialises the entire object

Remove study_date_from_serialised(), use the class attribute `study…

3e7c553

…_datetime` instead

Revert "Add study_datetime property for Message"

e8c0cc9

This reverts commit 8719153.

Remove Messages class, use list[Message] instead

fb44bf6

Add type checking for messages parsed from parquet input

6af792e

Update test_messages_from_parquet() to use JSON strings instead of …

0e4fce4

…bytes

Update PixlProducer.publish() to use a list of Message objects and …

99ffd00

…handle serialisation

Convert JSON string to bytes when serialising

986633d

Revert "Update test_messages_from_parquet() to use JSON strings ins…

d2c05ca

…tead of bytes" This reverts commit 0e4fce4.

PixlProducer.publish() should take a list[Message] as input in tests

cd3e101

milanmlft added 3 commits December 20, 2023 14:42

Update EHR API to use new Message design

0b855e1

Update imaging API to use new Message design

9a4d6a8

Update deserialise function to accept bytes-encoded JSON string

baad796

stefpiatek approved these changes Dec 20, 2023

View reviewed changes

stefpiatek reviewed Dec 20, 2023

View reviewed changes

pixl_core/src/core/patient_queue/message.py Outdated Show resolved Hide resolved

stefpiatek reviewed Dec 20, 2023

View reviewed changes

pixl_ehr/src/pixl_ehr/_processing.py Outdated Show resolved Hide resolved

milanmlft added 6 commits December 20, 2023 15:26

Assert messages against list of Messages

a81e285

Print dataclass in logs

8583ca0

jsonpickle.decode() can handle bytes so no need to decode first

7c32e8d

Also add a note about why we ignore ruff rule S301

Make deserialisable a keyword only argument

ae90838

Copilot forgot to convert dates to datetimes 🥲

2c49dc3

Refactor PixlConsumer run method to accept Message object as callback…

db1e34d

… parameter and deserialise

milanmlft commented Dec 20, 2023

View reviewed changes

Update consumer in test_subscriber to accept Message object instead…

7343fcd

… of bytes

milanmlft merged commit 90b6672 into rmg/parquet_inp Dec 20, 2023
7 checks passed

milanmlft deleted the mm/serialised-message-class branch December 20, 2023 17:36

skeating mentioned this pull request Jan 4, 2024

PIXL pipeline: make integration tests more realistic #205

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor message serialisation and deserialisation #197

Refactor message serialisation and deserialisation #197

milanmlft commented Dec 19, 2023 •

edited

Loading

stefpiatek left a comment •

edited

Loading

stefpiatek left a comment

milanmlft Dec 20, 2023

		await callback(message.body)
		await callback(deserialise(message.body))

Refactor message serialisation and deserialisation #197

Refactor message serialisation and deserialisation #197

Conversation

milanmlft commented Dec 19, 2023 • edited Loading

stefpiatek left a comment • edited Loading

Choose a reason for hiding this comment

stefpiatek left a comment

Choose a reason for hiding this comment

milanmlft Dec 20, 2023

Choose a reason for hiding this comment

milanmlft commented Dec 19, 2023 •

edited

Loading

stefpiatek left a comment •

edited

Loading