-
Notifications
You must be signed in to change notification settings - Fork 8
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
850 fix for exceptions being ignored in beamline unit tests #868
850 fix for exceptions being ignored in beamline unit tests #868
Conversation
819f11b
to
6cd0727
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
=======================================
Coverage 95.54% 95.54%
=======================================
Files 125 125
Lines 5343 5344 +1
=======================================
+ Hits 5105 5106 +1
Misses 238 238 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit
src/dodal/beamlines/p38.py
Outdated
@@ -253,6 +252,7 @@ def undulator( | |||
f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:", | |||
wait_for_connection, | |||
fake_with_ophyd_sim, | |||
id_gap_lookup_table_path="SPECIFY_ME", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd prefer that Undulator took as a default argument id_gap_lookup_table_path=os.devnull
: on i22, p38, the undulator isn't used as a Movable, and i22.py is pointing to an empty file on the dls filesystem. This would give us an empty file that is available everywhere, and when a beamline wants to specify it they can.
(splitting the Undulator into a Movable impl and a non-movable impl again is another issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all this device instantiation stuff has taught us we need a general look at how we provide these lookup tables to devices anyway. The undulator is maybe an exceptional case to this where actually in some places it's not changed, maybe we should have a writable and a read-only undulator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it to os.devnull
for now - I was basically looking here for someone to tell me what the correct path was
src/dodal/devices/i22/dcm.py
Outdated
prefix: str, | ||
temperature_prefix: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I disagree with this- I assume it's done because device_instantiation wants a prefix
arg: there's 2 prefixes here, motion gives context to what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the downside of python having both named and positional parameters - you don't get to choose the parameter names of your functions if you want them to be called by a framework :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should allow not forcing prefix
but whilst the old instantiation method insists on it I think we have to have it
src/dodal/beamlines/i04_1.py
Outdated
@@ -123,6 +123,7 @@ def undulator( | |||
f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:", | |||
wait_for_connection, | |||
fake_with_ophyd_sim, | |||
id_gap_lookup_table_path="SPECIFY_ME", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just do #871. I'll do that separate to this PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, thanks. I agree that the SPECIFY_ME
isn't great but I will make a new issue to holistically look at undulators
Sorry to ask a silly question here - does P38 has undulator or insertion device?
________________________________
From: Dominic Oram ***@***.***>
Sent: Friday, October 25, 2024 16:27
To: DiamondLightSource/dodal ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [DiamondLightSource/dodal] 850 fix for exceptions being ignored in beamline unit tests (PR #868)
@DominicOram commented on this pull request.
________________________________
In src/dodal/beamlines/p38.py<#868 (comment)>:
@@ -253,6 +252,7 @@ def undulator(
f"{BeamlinePrefix(BL).insertion_prefix}-MO-SERVC-01:",
wait_for_connection,
fake_with_ophyd_sim,
+ id_gap_lookup_table_path="SPECIFY_ME",
I think all this device instantiation stuff has taught us we need a general look at how we provide these lookup tables to devices anyway. The undulator is maybe an exceptional case to this where actually in some places it's not changed, maybe we should have a writable and a read-only undulator
—
Reply to this email directly, view it on GitHub<#868 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAAQXXIOJBYA7XX7ICUJ2KDZ5JPORAVCNFSM6AAAAABQS4Y7PGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOJVGY2TMNBXHA>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail. Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd.
Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message.
Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom.
|
74a344a
to
54c9f54
Compare
923a946
to
6bf0ab6
Compare
6bf0ab6
to
a912bfb
Compare
Fixes #850
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}