Fix: hyperion-supervisor unable to serialize load centre collect#1575
Fix: hyperion-supervisor unable to serialize load centre collect#1575
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1575 +/- ##
==========================================
+ Coverage 92.81% 92.86% +0.04%
==========================================
Files 153 156 +3
Lines 8642 8698 +56
==========================================
+ Hits 8021 8077 +56
Misses 621 621
🚀 New features to boost your workflow:
|
rtuck99
force-pushed
the
1564_unable_to_serialize_load_centre_collect
branch
from
8309bf7 to
bec094d
Compare
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, a couple of comments. I like this, it's generally cleaner but I think we need to pair it with removing some more of the old cruft. For example, I think we could probably remove MxBlueskyParameters as part of this PR. Alternatively we can do it in #1532 but that's starting to get a massive job
There was a problem hiding this comment.
Could: At the moment we don't support a different rotation energy but this implies we do. Given these are the external facing params they probably should be as clean as possible. Could we remove this from here and just set it to the XRC one in the converter?
There was a problem hiding this comment.
I think it should be the other way round surely? They agamemnon isn't specifying the centring parameters, as the XRC is incidental and the rotation is what they are interested in.
There was a problem hiding this comment.
Discussed in person, answer is to add energy into the top level of LoadCentreCollectParams only
| "sample_pin": agamemnon_params["sample"]["position"], | ||
| "select_centres": { | ||
| "name": "TopNByMaxCount", | ||
| "n": pin_type.expected_number_of_crystals, |
There was a problem hiding this comment.
Must: select_centres seems to be missing from LoadCentreCollectParams? I think we need to specify at least the n (at the moment we only ever use the one selection strategy so I don't think we need to specify that). More worrying is why a test didn't catch this given that we should be forbidding extra fields?
There was a problem hiding this comment.
It's there, imported via WithCentreSelection, which is perhaps a bit of a cheat, I didn't want to duplicate that hierarchy here. Perhaps I should move those into the same package.
There was a problem hiding this comment.
Ah, maybe that's ok then. It just wasn't obvious as everything else in that file is a clean slate
| class UDCDefaultState(BaseModel): | ||
| """Represents an instruction to execute the UDC default state plan.""" | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| class UDCCleanup(MxBlueskyParameters): | ||
| class UDCCleanup(BaseModel): | ||
| """Represents an instruction to perform UDC Cleanup, |
There was a problem hiding this comment.
Should: In collect() we're still passing in a parameter_model_version for both of these, which seems unnecessary.
rtuck99
force-pushed
the
1564_unable_to_serialize_load_centre_collect
branch
from
bec094d to
f3f5618
Compare
rtuck99
force-pushed
the
1564_unable_to_serialize_load_centre_collect
branch
4 times, most recently
from
5d36493 to
7ce4cfc
Compare
…odel instead of MxBlueskyParameters
Move demand_energy_ev into top-level
rtuck99
force-pushed
the
1564_unable_to_serialize_load_centre_collect
branch
from
de3afa6 to
0039c45
Compare
rtuck99
changed the title
3 participants
Fixes #1564
Link to dodal PR (if required): #N/A
(remember to update
pyproject.tomlwith the dodal commit tag if you need it for tests to pass!)hyperion-supervisorshould now be able to call the load_centre_collect plan onhyperion-blueapi.The
LoadCentreCollectParamspayload replaces theLoadCentreCollectpayload and no longer includes the paramter model version or duplicates of internal parameters.Instructions to reviewer on how to test:
Checks for reviewer