Update to Pydantic2 and ophyd_async 0.5.x #764
Conversation
DiamondJoseph
force-pushed
the
pydantic2
branch
from
ac026f8
to
5820636
Compare
DiamondJoseph
force-pushed
the
pydantic2
branch
2 times, most recently
from
30e1c66
to
7775702
Compare
DiamondJoseph
force-pushed
the
pydantic2
branch
from
7775702
to
45ffb5f
Compare
DiamondJoseph
requested review from
DominicOram,
callumforrester,
abbiemery,
coretl and
dperl-dls
|
Tagged 5 reviewers on this because there's a lot of lines changed so would really appreciate a few pairs of eyes over this. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #764 +/- ##
==========================================
+ Coverage 94.55% 94.89% +0.33%
==========================================
Files 115 115
Lines 4631 4640 +9
==========================================
+ Hits 4379 4403 +24
+ Misses 252 237 -15 ☔ View full report in Codecov by Sentry. |
DiamondJoseph
force-pushed
the
pydantic2
branch
2 times, most recently
from
8d2a5ba
to
56f426e
Compare
DiamondJoseph
force-pushed
the
pydantic2
branch
from
56f426e
to
0047e4c
Compare
There was a problem hiding this comment.
Great, thank you. Some comments in code. Can we wait for DiamondLightSource/mx-bluesky#454 to be done and merge both at the same time? I'll pick that up now
src/dodal/common/visit.py
Outdated
| @@ -61,7 +73,7 @@ async def get_current_collection(self) -> DataCollectionIdentifier: | |||
| async with session.get(f"{self._url}/numtracker") as response: | |||
| response.raise_for_status() | |||
| json = await response.json() | |||
| current_collection = DataCollectionIdentifier.parse_obj(json) | |||
| current_collection = DataCollectionIdentifier.model_validate_json(json) | |||
There was a problem hiding this comment.
Should: Can we add some unit tests against DirectoryServiceClient whilst we're here to confirm this? Bonus points for refactoring the almost identical create_new_collection and get_current_collection to share common code
…for override_run_number
DiamondJoseph
force-pushed
the
pydantic2
branch
from
8241989
to
70362f0
Compare
DiamondJoseph
force-pushed
the
pydantic2
branch
from
70362f0
to
6156312
Compare
| StaticVisitPathProvider( | ||
| BL, | ||
| Path("/exports/mybeamline/data"), | ||
| client=LocalDirectoryServiceClient(), |
There was a problem hiding this comment.
n.b. this means the file number will reset each time blueapi is restarted @callumforrester
src/dodal/common/visit.py
Outdated
| response.raise_for_status() | ||
| json = await response.json() | ||
| new_collection = DataCollectionIdentifier.model_validate_json(json) | ||
| LOGGER.debug("New DataCollection: %s", new_collection) |
There was a problem hiding this comment.
Should: Sorry, I know I asked you to combine these but now both of them print New DataCollection, which I suspect will be confusing. I will fix this and add tests
There was a problem hiding this comment.
Good catch, I thought about making it just "DataCollection %s". I think the new visit service will remove one of the methods, we just mutate a new DCI and cache the return value, which is probably also how this should behave now.
There was a problem hiding this comment.
Thanks @DiamondJoseph. I'm happy with all the changes here to code that MX uses. I've only done a basic review on:
- Directory provider stuff
- Tetramm
As I'm not so familiar with it, if someone else from core wants to review those bits that would be good. Otherwise the only reason I'm blocking merging is I would like DiamondLightSource/mx-bluesky#454 to be at the same time.
There was a problem hiding this comment.
I've been convinced that we should merge this now to get out of dependency hell elsewhere. I have also been assure that @callumforrester is happy with the bits of the PR I've not looked at too closely
Fixes #679, paves route to #644 after p4p updated.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}