Add retry, error handling for agamemnon and blueapi requests#1690
Add retry, error handling for agamemnon and blueapi requests#1690rtuck99 wants to merge 14 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1690 +/- ##
==========================================
+ Coverage 93.03% 93.07% +0.04%
==========================================
Files 164 164
Lines 8927 8981 +54
==========================================
+ Hits 8305 8359 +54
Misses 622 622
🚀 New features to boost your workflow:
|
|
|
||
| def raise_error_alert(self, content: str, metadata: dict[Metadata, str]): | ||
| """ | ||
| Raise an alert that will be forwarded to beamline staff and EHC controllers for out-of-hours |
There was a problem hiding this comment.
This sentence would be shorter in the form
"Alert beamline staff and EHC controllers ... support."
( We can see from the method name it achieves this by raising something )
There was a problem hiding this comment.
I think I want to retain the raise an alert phrasing - it's something that might have more meaning in future - various alerting paradigms such as AlertManager have the concept of alerts as things that can be raised and continue alerting until a subsequent action is taken.
Just using "alert" as a verb is weaker and could just mean sending a message, as opposed to something more substantial.
In the long run we will probably plug in to such systems via the alerting backend.
| by default. | ||
|
|
||
| """ | ||
| beamline = get_beamline_name("") |
There was a problem hiding this comment.
Reading the dodal getter for beamline name - passing it an empty string
as the default is going to leave us without a beamline name, but suppresses errors
I assume that is intended - it seems odd that the getter doesn't just define an empty string
if everyone in practice is just going to step around the potential warning / error anyway
There was a problem hiding this comment.
given line 64 it's tempting to use "UNSPECIFIED BEAMLINE" or similar ... ?
There was a problem hiding this comment.
I think the function has changed since this was originally authored and I can now get rid of the parameter.
| def _get_parameters_from_url(url: str) -> dict: | ||
| response = requests.get(url, headers={"Accept": "application/json"}) | ||
| response.raise_for_status() | ||
| tries, delay = MAX_TRIES, RETRY_INITIAL_DELAY_S |
There was a problem hiding this comment.
really - could these not have been done on separate lines -
or does ruff ( or similar ) keep putting them back onto one line?
one line - one action is always reader friendlier IMHO
There was a problem hiding this comment.
python gives you tuple assignment, it's the python way. They are related values so it makes sense to me.
| mock_requests.get.assert_has_calls( | ||
| [ | ||
| call( | ||
| "http://agamemnon.diamond.ac.uk/getnextcollect/i03", |
There was a problem hiding this comment.
it's not clear to me from the test
why i03 is going to be the obvious default ...
There was a problem hiding this comment.
external_interaction package has an autouse fixture in conftest.py that sets the beamline to i03.
In any case i03 is currently the only user of hyperion so I think it can be assumed that it is an appropriate beamline to use in tests.
There was a problem hiding this comment.
I don't think tests should ever know anything about a real URL - unless the
test is a system level test to see if it can genuinely see that ( but then You would never want such a test in CI )
Here it is a misleading realistic URL and creates the bad smell that the test might be dependent on the real IO3 beamline for a CI suite to pass - nobody wants to have read all the details of the code learn whether or not that is true
"... i00" ( instead of "... i03") would have made that independence plausible
Something like "http:/not_a_url/not_a_beamline" - even more so
| mock_alert_service: MagicMock, | ||
| ): | ||
| response = Mock(spec=Response) | ||
| response.status_code = 400 |
There was a problem hiding this comment.
if we really mean 40x - could we not have a parameterised test that racks up inputs 400 to 409 inclusive ?
There was a problem hiding this comment.
it could skip a few but at least maybe cover 3 or 4 of 'em
There was a problem hiding this comment.
_get_parameters_from_url() uses requests library's response.raise_for_status() method to separate error- from non-error statuses.
TBH with HTTP client libraries it is pretty pointless to try to test their internal error handling functions, they do all sorts of stuff that probably isn't even fully documented, like following redirects on 30x codes, retrying requests on 401 Unauthorized (because some client libraries don't like to send credentials if they are not needed). All you can do is trust that it mostly works as advertised.
There was a problem hiding this comment.
If You're determined to only ever test 400 - I don't quibble with that
I do quibble with doing only 400 inside a tin misleading labelled 40x
change the label to say 400
A bottle of sulphuric acid isn't usually labelled "some acid or other"
CoePaul
left a comment
CoePaul
left a comment
There was a problem hiding this comment.
A few suggested places for improvement
CoePaul
left a comment
CoePaul
left a comment
There was a problem hiding this comment.
You can read my replies - which I'm mostly letting slide
- and anyway it looks like it's got a merge conflict to repair
You seem to take the view that a test being brittle
can rely on the specifics of the existing production code
(or the specifics of the use cases that the production code is coupled to)
to rescue the test from the consequences of its own brittleness
Whereas another school of thought simply says
test brittleness is unnecessary and should be defensively
( defense through offense ) annihilated unless there's an extremely
good excuse for it being unavoidable / strictly necessary
I'm basically approving once the merge conflict irons out
2 participants
Fixes
Also partially addresses
On encountering a connection error when making a request to
hyperion-blueapi,hyperion-supervisorshould now attempt a further 2 more retries before giving up.Other errors or failure after exhausting retries should cause
hyperion-supervisorto end UDC and release the baton, raise an error alert but not exit.Similarly receiving a timeout error, 50x HTTP error or a connection error when requesting the next instruction from agamemnon, should result in up to 2 more retries, any other error or exhaustion of the retries should result in
hyperion-supervisorreleasing the baton with and raise an error alert but remaining running awaiting the baton.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!)Instructions to reviewer on how to test:
Checks for reviewer