-
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
Demark plan vs stub, move orphaned plans into dodal #793
base: main
Are you sure you want to change the base?
Conversation
7ab3c64
to
019b6ed
Compare
afaaddf
to
eb7ca4f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
+ Coverage 95.58% 95.65% +0.06%
==========================================
Files 125 129 +4
Lines 5419 5473 +54
==========================================
+ Hits 5180 5235 +55
+ Misses 239 238 -1 ☔ View full report in Codecov by Sentry. |
fddc20d
to
037fe3e
Compare
src/dodal/plans/scanspec.py
Outdated
], | ||
metadata: dict[str, Any] | None = None, | ||
) -> MsgGenerator: | ||
"""Generic plan for reading `detectors` at every point of a ScanSpec `spec`.""" |
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.
Could: For consistency, could we make this have a docstring like the others, rather than use Annotated
? Ditto for count
b2963ad
to
32dd6e1
Compare
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'm happy with all the changes that affect MX, thanks for doing those. I have added some comments to the other code but given it's just a copy of existing code and we do not make use of it in MX please consider them optional.
Group = Annotated[str, "String identifier used by 'wait' or stubs that await"] | ||
|
||
|
||
# https://github.com/bluesky/bluesky/issues/1821 |
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: From an outside perspective it's not clear from this comment or from the issue itself how this function relates to the issue. Will the function be removed, changed in some other way etc.
""" | ||
|
||
return ( | ||
# https://github.com/bluesky/bluesky/issues/1809 |
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: As above, how does that issue change this code?
|
||
@attach_data_session_metadata_decorator() | ||
@validate_call(config={"arbitrary_types_allowed": True}) | ||
def spec_scan( |
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.
FYI: I added DiamondLightSource/mx-bluesky#596 as I think we have overlap here
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 plan has now moved repositories so many times that the issue has now been lost to time, but it does need improvement. The axes_to_move
parameter was a temporary fix and we should try to get rid of it.
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.
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 no longer need axes_to_move
, I just need to find time to start up blueapi with these plans and ensure we can run a spec just passing the name of the device and no axes_to_move
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.
In theory, all plans that use a ScanSpec should be constructing their specific spec and passing it to this for software scans) or an ophyd plan to-be-produced for hardware.
"""Reads from a number of devices. | ||
Wraps bluesky.plans.count(det, num, delay, md=metadata) exposing only serializable | ||
parameters and metadata.""" |
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 assume we've discussed with bluesky to try and get this into the core codebase and couldn't? May be worth a link to that discussion in case anyone asks the question again
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.
It was a series of mostly offline discussions and should probably be written up as an ADR
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.
Does someone involved in the discussion have time to write an issue with some back of the envelope bullet point reasons and saying to put them in an ADR, just before it's lost to time?
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 believe we need DiamondLightSource/blueapi#474 for the @attach_metadata to be handled in blueapi consistently, then that decorator gets moved back into blueapi and then blueapi can just use bluesky.plans as a plans module (or... the ones we actually want).
|
||
def test_wait(): | ||
# Waits for all groups | ||
# move_on and wait are antonyms: what does move_on do? |
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.
Should: I agree that move_on
is bizarre. It should be documented in the bps.wait
docstring rather than this comment here, which doesn't really add anything for the reader other than more confusion. Can we add an issue into the bluesky repo to document it properly then either link here or remove the comment?
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.
Tracked down the initial issue that added it, raised an upstream ticket to document better.
bluesky/bluesky#1824
from dodal import plan_stubs, plans | ||
from dodal.common.types import PlanGenerator | ||
|
||
|
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: This is sort of a structural/architectural test rather than a unit test. I think it could do with a docstring at the top preparing the reader for that fact and explaining it's general perpose
yield | ||
|
||
|
||
def test_output_of_simple_spec( |
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'm not a big fan of big tests like this that are testing loads of things at once as I think they are poor documentation and make it harder to know what exactly the problem is when they break. I would prefer smaller tests like test_scan_spec_gives_run_start_with_expected_shape
etc.
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.
My $0.02: It's fine as long as you do both, a big integration test is sometimes good too as it catches issues that only arise when you put everything together, however a small unit test is more useful for the reasons you say so if you can only do one, do that.
On the other hand I think we do have to keep a close eye on how long it takes to run the dodal test suite. As we add more and more beamlines I'm expecting it to increase significantly and we might need to start thinking about solutions like pytest-parallel. I'd like to avoid that as long as possible though because we might get lazy and complacent if we know we can always get more resources (see every web browser).
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 both is best if possible, we have some tests in mx-bluesky
that are larger integration ones alongside the ones just testing small bits of logic. I also agree with the idea that they shouldn't take too long and I do regularly add PRs that just speed them up a bit. However, in my experience long running tests are mostly a consequence of poor mocking rather than quantity and it's actually large integration tests that end up taking longer than many small ones as people tend to have done more mocking in the smaller tests. Tests currently take ~30s on dodal
, which seems reasonable. I would suggest if they start hitting ~60s we should think harder about this.
RE(count({det}, num=0)) | ||
|
||
|
||
def test_count_output(det: StandardDetector, path_provider, RE): |
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: As above with long, complex tests
assert len(docs["stream_datum"]) == 1 * 2 # each point per resource | ||
|
||
|
||
def test_multi_count_output(det: StandardDetector, path_provider, RE): |
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: There's a lot of shared code here with the test above. Splitting them up into small, specific tests would help or I would also consider adding some helper functions
Bluesky distinguishes between plans: complete experimental proceedures, which open and close data collection runs, which may be part of a larger plan that collect data multiple times, but that might also be run alone to collect data, and plan_stubs: which do not create & complete data collection runs and are either isolated behaviours or building blocks for plans.
In order to make it clearer when a MsgGenerator can be safely used without considering the enclosing run (as opening a run whilst in a run without explicitly passing a RunID is likely to cause both runs to fail), when it is required to manage a run and when running a procedure will create data documents, we should adopt this standard.
This change also adds the count and spec_scan plan previously from dls_bluesky_core, as remove this as a requirement for blueapi is long overdue and this is the last remnants of that module.
count
is a basic collection of a set of devices, equivalent to Mapping GDA's StaticScan.spec_scan
is analogous to GDA'smscan
command, constructing a generic N-dimensional trajectory for scanning.After these plans have been moved, the default blueapi context can be configured to import as a plans module dodal.plans and dodal.plan_stubs.wrapped: this will only expose those stubs that should be runnable by a user from the client.
Checks for reviewer