Add background scans to i15-1 converter - #73
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 98.74% 98.76% +0.01%
==========================================
Files 17 18 +1
Lines 876 889 +13
==========================================
+ Hits 865 878 +13
Misses 11 11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, looks like something we can definitely start testing. Some comments in code.
Additionally, could we put some more info logging in so that we can work out what the queue is doing in terms of finding existing backgrounds, adding new ones and removing duplicates?
|
|
||
| from pydantic import BaseModel | ||
|
|
||
| BACKGROUND = Literal["air", "capillary_1", "capillary_2"] |
There was a problem hiding this comment.
Should: I think the capillaries are the same as those at https://github.com/DiamondLightSource/ulims-json-schemas/blob/9c156ced3504787f98b102747b28e4afba152292/schemas/samples/i15-1/powder/0.0.1.json#L38, which we can pull out of the graph. Happy to put this in another issue. We should also check with the beamline as this is a weirdly long list. It may be that we just need bs, fq, pi.
| return new_tasks | ||
|
|
||
| def _get_required_backgrounds(self, experiment: Experiment) -> list[BackgroundInfo]: | ||
| return [BackgroundInfo(bg_type="air", cobra=False, blower=False)] |
There was a problem hiding this comment.
Should: This is fine as a start but can you add tickets for adding the rest of the logic? i.e. add a background with that capillary type and ones at the correct temperature.
| name="Background", | ||
| instrument_session=instrument_session, | ||
| # Need to get sample info for test samples (air, empty capillary etc) | ||
| sample=Sample(name="air_1_1", id="", data={}), |
There was a problem hiding this comment.
So the current expected behaviour is that we will run a collection on puck 1, sample 1? I think this is ok but:
- We need to make sure we put something in this position on the beamline
- For the air collection we will actually need to run a data collection with no sample loaded - this is proabbly a new issue
| self, background: BackgroundInfo, instrument_session: str | ||
| ) -> Experiment: | ||
| return Experiment( | ||
| name="Background", |
There was a problem hiding this comment.
Should: Can we pull this into a constant?
|
|
||
| @app.exception_handler(ConverterError) | ||
| async def converter_error_handler(request: Request, exception: ConverterError): | ||
| LOGGER.exception("Queue error occurred") |
There was a problem hiding this comment.
Should: Was this accidentally added? If not, we should add the exception details.
There was a problem hiding this comment.
By some magic, this gives you the whole stack trace. However, I should add the same for all the exception handlers as it makes the queue logs much more useful.
There was a problem hiding this comment.
Oh, I remember the exception magic now...
| new_tasks.append(task) | ||
| return self._remove_repeated_backgrounds(new_tasks) | ||
|
|
||
| def _remove_repeated_backgrounds(self, tasks: list[Task]) -> list[Task]: |
There was a problem hiding this comment.
Adding all the backgrounds then removing the repeats is probably not going to work indefinitely. There is a usecase for the different temperature collections to do something more clever e.g. if there is an experiment that takes data between 0-10 deg and another between 5-10 degrees then we can cover it with just one background between 0-10 degrees. There's also quite a bit of inefficiency here where we're looping through the list multiple times. I think it's ok for now to just think about air and empty capillaries at room temp though, which this does cover, so we can think about that another day.
There was a problem hiding this comment.
Yeah I think the logic here needs to be more complex, especially since background tasks persist between _syncs. I think this diagram sums it up
Not covered there though is the need to remove background experiments from the queue if they are no longer required, for example if the experiment that required it has been removed.
There was a problem hiding this comment.
Yh, I think spin it into a new issue though. For now just getting something where we can show it adds some backgrounds would be good
| ), | ||
| ] | ||
|
|
||
| def _add_required_background_scans(self, tasks: list[Task]) -> list[Task]: |
There was a problem hiding this comment.
Could: Can you add a docstring here?
| TaskWithPosition, | ||
| ) | ||
|
|
||
| SCAN_PLANS = Literal["centre_sample", "static_collection"] |
There was a problem hiding this comment.
Should: What is this literal for?
There was a problem hiding this comment.
Have deleted it
| def get_background_tiled_id( | ||
| required_background: BackgroundInfo, instrument_session: str | ||
| ) -> str | None: | ||
| client = from_uri("https://tiled.diamond.ac.uk/api/v1") |
There was a problem hiding this comment.
Should: I think this only works if you're logged in already right? I'm not sure how we make sure the queue is logged in?
There was a problem hiding this comment.
This seems to work when running the queue locally without having logged in.. which does seem weird
There was a problem hiding this comment.
Have we tested in prod? Let's merge it and see
| from tiled.queries import Eq | ||
|
|
||
| from daq_queuing_service.plugins.i15_1.backgrounds import BackgroundInfo | ||
|
|
There was a problem hiding this comment.
Should: I think just add an ignore for the linter on this whole file?
7f3945d to
733f0e0
Compare
2 participants
Fixes DiamondLightSource/crystallography-bluesky#95