search

DiamondLightSource / python-zocalo

Infrastructure components for automated data processing at Diamond Light Source
https://pypi.python.org/pypi/zocalo
BSD 3-Clause "New" or "Revised" License
6 stars 6 forks source link

Update pydantic to v2 #247

Closed dperl-dls closed 2 months ago

dperl-dls commented 7 months ago

We have been trying to move Hyperion and Dodal to Pydantic v2 for some time. We think that this is the only remaining dependency we have which is stuck on v1.

I'm happy do do any work needed if necessary.

dperl-dls commented 7 months ago

I note that running the tests in a fresh environment with Pydantic upgraded to 2.6 doesn't result in any failures. Is there (still) a reason for it to be pinned? @ndevenish @graeme-winter

ndevenish commented 6 months ago

There was a reason, which was that v2 broke things and we needed to update the code. Noemi pointed out to me on Friday that they eventually added some backwards-compatibility to newer versions, so if the code doesn't need changing then there is no reason to pin.

I'm not sure how much work this would be to verify, but I'd probably like to get things locked in for this run before changing everything round (next week is probably going to be busy!)

dperl-dls commented 6 months ago

@ndevenish Yes, I also believe that it should be okay because of the retrospective addition of backwards compatibility (they caused a lot of annoyance breaking everything...). Looking at the pydantic classes here they appear very simple so I think the chances are pretty good that few if any changes should be needed.

I was wrong about the tests though, there are a few which fail: test_api_exchange_declare, test_api_binding_declare and test_api_add_vhost. Each of these assert that puted requests contain some empty items ('arguments': {} and "tags": [] respectively) which seem to be explicitly excluded with exclude_defaults=True options passed to various model .dict() calls in rabbitmq.py - it kind of feels like this was a bug before already? (Since the empty list/dicts are created by the Field's default_factory)

Here is a branch which is "working" and with some new tests which show the above change: https://github.com/dperl-dls/python-zocalo/tree/247_upgrade_pydantic but I'm not sure if this is correct/how to tell if those empty arguments are required by rabbitMQ. I would really appreciate if you could find time to take a look in the not too distant future but I understand it's Easter and busy startup etc.

ndevenish commented 2 months ago

Should be resolved with https://github.com/DiamondLightSource/python-zocalo/pull/255