Perform custom deserializations #1
Conversation
kombu/asynchronous/timer.py
Outdated
| @@ -11,25 +11,26 @@ | |||
|
|
|||
| from vine.utils import wraps | |||
|
|
|||
| from kombu.log import get_logger | |||
| from kombu.log import get_logg | |||
There was a problem hiding this comment.
surprised that our build is working despite this
jbkkd
force-pushed
the
v5.2.4-patched
branch
2 times, most recently
from
7c604f1 to
2c73452
Compare
| @@ -13,23 +13,24 @@ | |||
|
|
|||
| from kombu.log import get_logger | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
No change necessary, but to save us the conditional handling of ZoneInfo, as we only want UTC we could use Python's datetime.timezone.utc.
There was a problem hiding this comment.
I am +1 on this, seems like a less risky change than introducing zoneinfo
There was a problem hiding this comment.
You're right that we could do that, however this change is taken directly from kombu:
celery#1680
Seeing that ZoneInfo will be added once we upgrade regardless, we might as well do this now. I prefer sticking to whatever kombu is doing even if it isn't the best solution.
kombu/utils/json.py
Outdated
| @@ -69,6 +69,19 @@ def dumps(s, _dumps=json.dumps, cls=None, default_kwargs=None, **kwargs): | |||
| **dict(default_kwargs, **kwargs)) | |||
|
|
|||
|
|
|||
| def object_hook(dct): | |||
There was a problem hiding this comment.
this looks wrong to me. Shouldn't we be checking for the "__type__" and "__value__" keys in the dict?
like here https://github.com/celery/kombu/blob/v5.3.2/kombu/utils/json.py#L60-L69
Also, shouldn't this function be returning strings?
There was a problem hiding this comment.
Ah yes, I based my changes on this pr:
celery#1516
But I see now that main has changed that behaviour
There was a problem hiding this comment.
As for returning strings - we're going to do that in kraken directly rather than through a patch to kombu, so we won't have to re-patch on every new kombu upgrade
There was a problem hiding this comment.
I've changed the PR to make it more similar to what's currently on the main branch
There was a problem hiding this comment.
I've changed the PR to make it more similar to what's currently on the main branch
I don't think we should do this. This PR should keep kombu as close as possible to 5.2.4 (not main), with the minimal changes needed to handle {"__type__": ..., "__value__": ...} objects.
There was a problem hiding this comment.
As for returning strings - we're going to do that in kraken directly rather than through a patch to kombu, so we won't have to re-patch on every new kombu upgrade
We're doing that in kraken only once kombu is upgraded to 5.3.0+ and this patch is thrown away.
This patch is for kraken still running kombu 5.2.4, it needs to be able to receive the special objects and return strings for the workers e.g. receive
{"__type__": "datetime", "__value__": "2023-09-29T00:45:25.412564Z"}and return the string "2023-09-29T00:45:25.412564Z" since that is what the workers expect
There was a problem hiding this comment.
I have a slightly different approach in mind, with the following steps:
- Update kraken-core to use this branch, and also add the register_type on the same PR in kraken-core. This means that rather than needing this (kombu) branch to do the the
__datetime__deserialization, this will be done in kraken-core using the same method we'll eventually use once >5.3.0 is live. - Deploy the kombu upgrade PR, which will only include the kombu upgrade and nothing more.
It's a slightly different approach to the on you're suggesting (which is doing the datetime deserialization in this branch), but achieves the same results - we'll be able to safely rollback at any point whilst always returning str for datetimes and UUIDs.
I've tested these changes locally. First, I installed this patched version, scheduled a task with this version that sends a datetime argument, then installed 5.2.4 again and ran the celery worker. The argument was deserialized as an str.
Then I did the same thing with 5.3.2 - I scheduled as task on that version, and ran the worker with the patched version, and again the task was deserialized as str.
I've updated the kraken-core PR related to this to reflect that.
jamesbeith
dismissed
their stale review
Only meant to leave a commenting review.
Items with these values will start appearing in a later version of kombu (5.3.0 onwards). To migrate our kombu installation safely, we first add support for deserializing these objects, deploy that, and then we can migrate kombu to version 5.3.0 safely.
It's been removed in this PR: celery#1680 Moreover, the test.txt syntax is invalid with our version of setuptools
There was a problem hiding this comment.
Omer has talked me through the overall approach, and I think while there is more than one way to tackle this, it seems to me a decent one - on the basis that this fork won't stick around for very long.
This doesn't need an approval, but I'm approving https://github.com/octoenergy/kraken-core/pull/105425 instead. I think that one could do with a second thumbs up from @LincolnPuzeyCC, who is closer to the details.
| # NOTE: datetime should be registered before date, | ||
| # because datetime is also instance of date. | ||
| register_type(datetime, "datetime", datetime.datetime.isoformat, datetime.datetime.fromisoformat) | ||
| register_type( | ||
| datetime.date, | ||
| "date", | ||
| lambda o: o.isoformat(), | ||
| lambda o: datetime.datetime.fromisoformat(o).date(), | ||
| ) | ||
| register_type(datetime.time, "time", lambda o: o.isoformat(), datetime.time.fromisoformat) | ||
| register_type(Decimal, "decimal", str, Decimal) | ||
| register_type( | ||
| uuid.UUID, | ||
| "uuid", | ||
| lambda o: {"hex": o.hex}, | ||
| lambda o: uuid.UUID(**o), | ||
| ) |
There was a problem hiding this comment.
given this is our fork of kombu you could remove this part since these registrations are overwritten by kraken anyway - and these contain the behavior we don't want - kombu de-coding into typed objects rather than strings.
|
Left one suggestion - but otherwise looks fine - this will let kraken handle |
Items with these values will start appearing in a later version of kombu (5.3.0 onwards).
To migrate our kombu installation safely, we first add support for deserializing these objects, deploy that, and then we can migrate kombu to version 5.3.0 safely.