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

Update pydantic to v2 #247

Closed
dperl-dls opened this issue Mar 7, 2024 · 4 comments
Closed

Update pydantic to v2 #247

dperl-dls opened this issue Mar 7, 2024 · 4 comments

Comments

@dperl-dls
Copy link

dperl-dls commented Mar 7, 2024

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
Copy link
Author

dperl-dls commented Mar 7, 2024

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
Copy link
Contributor

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 added a commit to dperl-dls/python-zocalo that referenced this issue Mar 28, 2024
dperl-dls added a commit to dperl-dls/python-zocalo that referenced this issue Mar 28, 2024
@dperl-dls
Copy link
Author

@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
Copy link
Contributor

Should be resolved with #255

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

No branches or pull requests

2 participants