Overhaul documentation #674

callumforrester · 2024-10-17T12:04:42Z

Fixes #447
Fixes #361
Fixes #671

codecov · 2024-10-17T12:07:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.17%. Comparing base (86c5905) to head (674f5e1).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #674   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files          35       35           
  Lines        1803     1803           
=======================================
  Hits         1662     1662           
  Misses        141      141

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DiamondJoseph

How much is this just re-formatting the docs to pass current linting/build and how much is it intending to update the docs? There's several things that we're actively moving away from that aren't being changed, but also more changes than just updating the formatting.

docs/explanations/architecture.md

docs/explanations/extension-code.md

DiamondJoseph · 2024-10-17T12:14:30Z

docs/explanations/extension-code.md

+
+## Dodal
+
+[Dodal](https://github.com/DiamondLightSource/dodal) is a repository for DLS device configuration, providing factory functions for devices used at DLS. If you work at DLS, new devices should be added there.


new devices = new device classes?

This is mostly true, get devices into dodal, then plan to move to ophyd-async. Let's treat mostly true as true for the purposes of introductory docs.

Should be expanded to explain that beamlines should also be added there, with a link to dodal how to make a new beamline.

This is a problem in general, some of these docs belong in dodal with links from blueapi. I think we can and should do that migration gradually though. I'm going to advocate for removing this sentence and making an issue in dodal for adding a page/pages explaining how to:

Make a new device

Add a device to a beamline

Make a new beamline

The reason I asked is there is already docs in dodal for at least part of that
https://diamondlightsource.github.io/dodal/main/how-to/create-beamline.html
https://diamondlightsource.github.io/dodal/main/reference/device-standards.html (?)

Ah cool! I'll reference those

DiamondJoseph · 2024-10-17T12:14:50Z

docs/explanations/extension-code.md

+* A Github repository
+* A local directory via the [scratch area](../how-to/edit-live.md).
+
+The easiest place to put the code is a repository created with the [`python-copier-template`](https://diamondlightsource.github.io/python-copier-template/main/index.html). Which can then become any of the above.


Could: an example, like i22?

DiamondJoseph · 2024-10-17T12:15:15Z

docs/explanations/lifecycle.md

+
+
+def count(
+    detectors: List[Readable] = [inject("det")],  # default valid for Blueapi only


Is this change going in prior to deprecating inject or after?

Update to >=3.10 typing (List->list, Union[x, y] -> x | y, Optional[x] -> x | None)

I am going to make a separate issue for moving all of the code snippets to Python files and checking that they still work and lint properly at docs build time, similar to ophyd-async.

DiamondJoseph · 2024-10-17T12:17:45Z

docs/explanations/type_validators.md

+```python 
+def my_plan(a: int, b: str = "b") -> Plan
+    ...

-    # Internally becomes something like
+# Internally becomes something like

-    class MyPlanModel(BaseModel):
-        a: int
-        b: str = "b"
+class MyPlanModel(BaseModel):
+    a: int
+    b: str = "b"


Is this repeating what is explained in plans.md?

Probably does need a bit of consolidation, my read is that plans.md explains which parts of blueapi plans are special and why, and type_validators.md explains the very technical implementation of plan parameter parsing.

I think this is just repeating what is in lifecycle.md. It even includes the same json example, I'd really like to see one removed or the difference between the two explained.

So the idea is that lifecycle.md is a general overview of how blueapi stores, retrieves and runs plans whereas this is a deep dive into the special handling for substituting devices for strings. Do you have any suggestions as to what we can shuffle around to make that clearer?

docs/explanations/type_validators.md

docs/how-to/configure-app.md

DiamondJoseph · 2024-10-17T12:36:56Z

docs/how-to/write-devices.md

+Devices are made using the [dodal](https://github.com/DiamondLightSource/dodal) style available through factory functions like this:
+
+```python 
+from my_facility_devices import MyTypeOfDetector
+
+def my_detector(name: str) -> MyTypeOfDetector:
+    return MyTypeOfDetector(name, {"other_config": "foo"})
+```
+
+The return type annotation `-> MyTypeOfDetector` is required as blueapi uses it to determine that this function creates a device. Meaning you can have a Python file where only some functions create devices and they will be automatically picked up. Similarly, these functions can be organized per-preference into files. 
+
+The device is created via a function rather than a global to preserve side-effect-free imports. Each device must have its own factory function.


Should: defer to dodal's docs on writing device classes and beamlines, and ensure they are up to date?

Agreed, see other discussions, I think we should migrate some of this to dodal as a separate PR

dodal already has docs on creating a beamline and making a device class.

https://diamondlightsource.github.io/dodal/main/how-to/create-beamline.html
and
https://diamondlightsource.github.io/dodal/main/how-to/make-new-ophyd-async-device.html

We should defer to these and fix those pages. The former should be updated to use device_factory and the second should be fixed as the Mermaid docs do not render responsively and should explain how to make a Device class.

I agree, but I would rather merge this now and raise separate issues for fixing those pages and consolidating, otherwise this PR will languish for a long time and the published docs (that everyone reads) will continue to contain mistakes that throw people off. Alternatively, I can remove this file entirely from the PR and make issues in dodal, what do you think?

DiamondLightSource/dodal#933

DiamondJoseph · 2024-10-17T12:38:45Z

docs/how-to/write-plans.md

+Blueapi exposes the docstrings of plans to clients, along with the parameter types. It is therefore worthwhile to make these detailed and descriptive. This may include units of arguments (e.g. seconds or microseconds), its purpose in the function, the purpose of the plan etc.
+
+```python
+def temp_pressure_snapshot(
+    detectors: List[Readable],
+    temperature: Movable = inject("sample_temperature"),
+    pressure: Movable = inject("sample_pressure"),
+    target_temperature: float = 273.0,
+    target_pressure: float = 10**5,
+    metadata: Optional[Mapping[str, Any]] = None,
+) -> MsgGenerator:
+    """
+    Moves devices for pressure and temperature (defaults fetched from the context)
+    and captures a single frame from a collection of devices
+    Args:
+        detectors (List[Readable]): A list of devices to read while the sample is at STP
+        temperature (Optional[Movable]): A device controlling temperature of the sample,
+            defaults to fetching a device name "sample_temperature" from the context
+        pressure (Optional[Movable]): A device controlling pressure on the sample,
+            defaults to fetching a device name "sample_pressure" from the context
+        target_pressure (Optional[float]): target temperature in Kelvin. Default 273
+        target_pressure (Optional[float]): target pressure in Pa. Default 10**5
+    Returns:
+        MsgGenerator: Plan
+    Yields:
+        Iterator[MsgGenerator]: Bluesky messages
+    """
+    yield from move({temperature: target_temperature, pressure: target_pressure})
+    yield from count(detectors, 1, metadata or {})


Should: replacing this form of docstring with using Annotated/Field arguments so that the values propagate into json schema?

Oh, I did not realise they didn't propagate, that's quite bad. I was hoping to make the plans as vanilla-bluesky as we could, so vanilla python function docstrings are definitely desirable...

Lemme get a couple of example plans into blueapi and compare the json schemas

@attach_data_session_metadata_decorator() @validate_call(config={"arbitrary_types_allowed": True}) def linkam_plan_1( trajectory: LinkamTrajectory, linkam: Linkam3 = LINKAM, shutter_time: float = 0.04, ) -> MsgGenerator: """ Follow a trajectory in temperature, collecting a number of frames either at equally spaced positions or while continually scanning. e.g. for 2 segments, the first stepped and the 2nd flown:\n trajectory start v v final segment stop\n \\ /\n stepped segment__\\__ /\n \\ / flown segment\n 1st segment stop \\__ /\n exposures: xx xx xx 1/N seconds Args: trajectory: Trajectory for the scan to follow. linkam: Temperature controller shutter_time: Time allowed (s) for opening shutter before triggering detectors. Returns: MsgGenerator: Plan Yields: Iterator[MsgGenerator]: Bluesky messages """ ... @attach_data_session_metadata_decorator() @validate_call(config={"arbitrary_types_allowed": True}) def linkam_plan_2( trajectory: Annotated[ LinkamTrajectory, Field(description="Trajectory for the scan to follow.") ], linkam: Annotated[Linkam3, Field(LINKAM, description="Temperature controller.")], shutter_time: Annotated[ float, Field( 0.04, description="Time allowed for opening shutter before triggering detectors.", json_schema_extra={"units": "s"}, ), ], ) -> MsgGenerator: """ Follow a trajectory in temperature, collecting a number of frames either at equally spaced positions or while continually scanning. e.g. for 2 segments, the first stepped and the 2nd flown:\n trajectory start v v final segment stop\n \\ /\n stepped segment__\\__ /\n \\ / flown segment\n 1st segment stop \\__ /\n exposures: xx xx xx 1/N seconds """ ... @attach_data_session_metadata_decorator() @validate_call(config={"arbitrary_types_allowed": True}) def linkam_plan_3( trajectory: LinkamTrajectory = Field( description="Trajectory for the scan to follow." ), linkam: Linkam3 = Field(LINKAM, description="Temperature controller."), shutter_time: float = Field( 0.04, description="Time allowed for opening shutter before triggering detectors.", json_schema_extra={"units": "s"}, ), ) -> MsgGenerator: """ Follow a trajectory in temperature, collecting a number of frames either at equally spaced positions or while continually scanning. e.g. for 2 segments, the first stepped and the 2nd flown:\n trajectory start v v final segment stop\n \\ /\n stepped segment__\\__ /\n \\ / flown segment\n 1st segment stop \\__ /\n exposures: xx xx xx 1/N seconds """ ...

linkam_plan_1.json
linkam_plan_2.json
linkam_plan_3.json

I've just gone through all that just to prove they're basically identical and the Pydantic model doesn't actually get the information about the Fields 🙃

I think we can continue this discussion in #644

callumforrester · 2024-10-18T10:07:11Z

@DiamondJoseph regarding the fact that some of this is going out of date "soon": I have been bitten too many times by writing code and docs in anticipation of changes that are due "in a week or so" and end up taking 6 months. I'm strongly against putting anything in here that is not true in main right now.

Ideally docs should be updated in the same PR as the change they reference, second-best option is after the PR is merged, worst-best is before the PR is merged.

docs/how-to/write-plans.md

callumforrester · 2024-10-29T11:00:42Z

docs/tutorials/run-bus.md

+```
+podman run -it --rm --net host rmohr/activemq:5.15.9-alpine
+```


Remove per discussion with @DominicOram

Move lots of detail to explanations

Co-authored-by: Joseph Ware <[email protected]>

docs/explanations/lifecycle.md

docs/explanations/plans.md

DiamondJoseph · 2024-11-25T13:18:37Z

docs/explanations/type_validators.md

+```python 
+def my_plan(a: int, b: str = "b") -> Plan
+    ...

-    # Internally becomes something like
+# Internally becomes something like

-    class MyPlanModel(BaseModel):
-        a: int
-        b: str = "b"
+class MyPlanModel(BaseModel):
+    a: int
+    b: str = "b"


I think this is just repeating what is in lifecycle.md. It even includes the same json example, I'd really like to see one removed or the difference between the two explained.

DiamondJoseph · 2024-11-25T13:29:42Z

docs/how-to/write-devices.md

+Devices are made using the [dodal](https://github.com/DiamondLightSource/dodal) style available through factory functions like this:
+
+```python 
+from my_facility_devices import MyTypeOfDetector
+
+def my_detector(name: str) -> MyTypeOfDetector:
+    return MyTypeOfDetector(name, {"other_config": "foo"})
+```
+
+The return type annotation `-> MyTypeOfDetector` is required as blueapi uses it to determine that this function creates a device. Meaning you can have a Python file where only some functions create devices and they will be automatically picked up. Similarly, these functions can be organized per-preference into files. 
+
+The device is created via a function rather than a global to preserve side-effect-free imports. Each device must have its own factory function.


dodal already has docs on creating a beamline and making a device class.

https://diamondlightsource.github.io/dodal/main/how-to/create-beamline.html
and
https://diamondlightsource.github.io/dodal/main/how-to/make-new-ophyd-async-device.html

We should defer to these and fix those pages. The former should be updated to use device_factory and the second should be fixed as the Mermaid docs do not render responsively and should explain how to make a Device class.

docs/how-to/write-devices.md

DiamondJoseph · 2024-11-25T13:32:29Z

docs/how-to/write-plans.md

-   def pass_metadata(x: Movable, metadata: Optional[Mapping[str, Any]] = None) -> MsgGenerator:
-       yield from bp.count{[x], md=metadata or {}}
+```python
+def pass_metadata(x: Movable, metadata: Mapping[str, Any] | None = None) -> MsgGenerator:


I think we need to tighten up Mapping -> dict to behave well with bp and bps usage.

Sorry, not sure I follow, what issues are you seeing?

bluesky.plans have their equivalent md argument (which we should use when possible) is CustomPlanMetadata = dict[str, Any]: we therefore cannot pass a Mapping[str, Any] down to it: Mapping does not allow assignment, where dict does. We also sometimes need to mutate metadata in a "sub"plan before passing it to a "super"plan.

This very plan would fail type checking for the issue. We can quite neatly then have the required parts of the plan be from bluesky.utils:

from bluesky.utils import CustomPlanMetadata, MsgGenerator def foo(metadata: CustomPlanMetadata | None = None) -> MsgGenerator: yield from {}

I'm happy to change it, please feel free to read or ignore the small rant below.

The reason it's dict is because it's mutated upstream:

def my_plan(params: ..., md: dict[str, Any]): ... md["plan_name"] = my_plan.__name__ yield from ...

It could instead be kept immutable:

def my_plan(params: ..., md: Mapping[str, Any]): ... md = {**md, **{"plan_name": my_plan.__name__}} yield from ...

Immutable = good, so I would prefer we change upstream, but the verbosity of that solution may be unpalletable.

Co-authored-by: Joseph Ware <[email protected]>

…planations

DiamondJoseph · 2024-11-28T08:44:27Z

docs/how-to/write-plans.md

+    motor: Movable, 
+    steps: int, 
+    sample_name: str, 
+    extra_metadata: dict[str, Any]) -> MsgGenerator:


nit: the plan linting in dodal enforces that plans have exactly this argument metadata: dict[str, Any] | None = None - would be nice to have this example compliant with that.

See also line 43 of this file which says that plans should have this arg...

callumforrester requested a review from DiamondJoseph October 17, 2024 12:05

DiamondJoseph requested changes Oct 17, 2024

View reviewed changes

callumforrester mentioned this pull request Oct 18, 2024

Lint and execute code snippets in docs #677

Open

DiamondJoseph mentioned this pull request Oct 21, 2024

Replace PlanModel using the __doc__ of a plan with using Annotated pydantic fields #644

Open

callumforrester commented Oct 21, 2024

View reviewed changes

docs/how-to/write-plans.md Show resolved Hide resolved

callumforrester requested a review from DiamondJoseph October 22, 2024 13:23

callumforrester mentioned this pull request Oct 29, 2024

Include convenience bash script for running rabbit mq locally #668

Merged

callumforrester commented Oct 29, 2024

View reviewed changes

callumforrester force-pushed the docs-overhaul branch 2 times, most recently from 3eed614 to 076c875 Compare October 30, 2024 13:25

callumforrester force-pushed the docs-overhaul branch from 076c875 to fc787c3 Compare November 13, 2024 10:54

callumforrester and others added 14 commits November 25, 2024 10:23

Complete basic set of tutorials

e071afe

Split out writing plans and devices section

664e0f4

Move lots of detail to explanations

Add info about writing devices

ded57f2

Make tutorial actually work

b1cf10a

Tidy up how tos

5f6ce25

Tidy up explanations

96bde32

Fix references

3198f50

Consistent title capitalisation

581e3ca

Remove lie

356c119

Remove broken image from readme

6b46a4e

Fix indent

07b8285

Action suggestions for home of plans and devices page

bd6d880

Remove references to inject

0a63937

Apply generalisation of external connections in docs

94a95b8

Co-authored-by: Joseph Ware <[email protected]>

callumforrester force-pushed the docs-overhaul branch from fc787c3 to 94a95b8 Compare November 25, 2024 10:28

Fix markdown check

dd5d750

callumforrester mentioned this pull request Nov 25, 2024

Refactor utility script to allow for future services #707

Open

DiamondJoseph requested changes Nov 25, 2024

View reviewed changes

callumforrester and others added 6 commits November 25, 2024 13:33

Correct MsgGenerator import

e9916b9

Co-authored-by: Joseph Ware <[email protected]>

Fix typo

9e3e3b5

Co-authored-by: Joseph Ware <[email protected]>

Should vs must

529ea2d

Co-authored-by: Joseph Ware <[email protected]>

Correct attach metadata decorator.

1e05386

Co-authored-by: Joseph Ware <[email protected]>

Make metadata examples mutable

d66e431

Remove some duplication between type validators and plan lifecycle ex…

674f5e1

…planations

callumforrester mentioned this pull request Nov 28, 2024

Move blueapi documentation on device best practices into dodal DiamondLightSource/dodal#933

Open

DiamondJoseph approved these changes Nov 28, 2024

View reviewed changes

callumforrester merged commit 33908fd into main Nov 28, 2024
29 checks passed

callumforrester deleted the docs-overhaul branch November 28, 2024 08:56


		## Dodal

		[Dodal](https://github.com/DiamondLightSource/dodal) is a repository for DLS device configuration, providing factory functions for devices used at DLS. If you work at DLS, new devices should be added there.



		def count(
		detectors: List[Readable] = [inject("det")], # default valid for Blueapi only

Overhaul documentation #674

Overhaul documentation #674

Conversation

callumforrester commented Oct 17, 2024

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

DiamondJoseph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2024 •

edited

Loading