From 34d2b0d0435f89d6628fd99280032b02bd673c7c Mon Sep 17 00:00:00 2001 From: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com> Date: Tue, 26 Nov 2024 14:06:52 +0000 Subject: [PATCH 1/4] Update training-rigs to use device_factory (#924) --- src/dodal/beamlines/training_rig.py | 42 +++++++++++------------------ 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/src/dodal/beamlines/training_rig.py b/src/dodal/beamlines/training_rig.py index 3c026c6576..8c6782f8c5 100644 --- a/src/dodal/beamlines/training_rig.py +++ b/src/dodal/beamlines/training_rig.py @@ -3,15 +3,16 @@ from ophyd_async.epics.adaravis import AravisDetector from dodal.common.beamlines.beamline_utils import ( - device_instantiation, + device_factory, get_path_provider, set_path_provider, ) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline +from dodal.common.beamlines.device_helpers import HDF5_PREFIX from dodal.common.visit import LocalDirectoryServiceClient, StaticVisitPathProvider from dodal.devices.training_rig.sample_stage import TrainingRigSampleStage from dodal.log import set_beamline as set_log_beamline -from dodal.utils import get_beamline_name +from dodal.utils import BeamlinePrefix, get_beamline_name # # HTSS Training Rig @@ -20,11 +21,12 @@ # simple motors, a GigE camera and a PandA. # Since there are multiple rigs whose PVs are identical aside from the prefix, # this module can be used for any rig. It should fill in the prefix automatically -# if the ${BEAMLINE} environment variable is correctly set. It currently defaults -# to p47. +# if the ${BEAMLINE} environment variable is correctly set, else defaulting +# to p46, which is known to be in good working order. # -BL = get_beamline_name("p47") +BL = get_beamline_name("p46") +PREFIX = BeamlinePrefix(BL) set_log_beamline(BL) set_utils_beamline(BL) @@ -37,28 +39,16 @@ ) -def sample_stage( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> TrainingRigSampleStage: - return device_instantiation( - TrainingRigSampleStage, - "sample_stage", - "-MO-MAP-01:STAGE:", - wait_for_connection, - fake_with_ophyd_sim, - ) +@device_factory() +def sample_stage() -> TrainingRigSampleStage: + return TrainingRigSampleStage(f"{PREFIX.beamline_prefix}-MO-MAP-01:STAGE:") -def det( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> AravisDetector: - return device_instantiation( - AravisDetector, - "det", - "-EA-DET-01:", - wait_for_connection, - fake_with_ophyd_sim, - drv_suffix="DET:", - hdf_suffix="HDF5:", +@device_factory() +def det() -> AravisDetector: + return AravisDetector( + f"{PREFIX.beamline_prefix}-EA-DET-01:", path_provider=get_path_provider(), + drv_suffix="DET:", + hdf_suffix=HDF5_PREFIX, ) From 86d798ad1eb1e3c1c3969f92636e13c53eae9e9b Mon Sep 17 00:00:00 2001 From: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:02:39 +0000 Subject: [PATCH 2/4] Update b01-1 to use device_factory (#923) --- src/dodal/beamlines/b01_1.py | 47 ++++++++++++------------------------ 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/src/dodal/beamlines/b01_1.py b/src/dodal/beamlines/b01_1.py index abc4e91b27..27aab36ba1 100644 --- a/src/dodal/beamlines/b01_1.py +++ b/src/dodal/beamlines/b01_1.py @@ -4,16 +4,19 @@ from ophyd_async.fastcs.panda import HDFPanda from dodal.common.beamlines.beamline_utils import ( - device_instantiation, + device_factory, get_path_provider, set_path_provider, ) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline +from dodal.common.beamlines.device_helpers import HDF5_PREFIX from dodal.common.visit import LocalDirectoryServiceClient, StaticVisitPathProvider from dodal.devices.synchrotron import Synchrotron from dodal.log import set_beamline as set_log_beamline +from dodal.utils import BeamlinePrefix BL = "c01" +PREFIX = BeamlinePrefix(BL) set_log_beamline(BL) set_utils_beamline(BL) @@ -36,42 +39,24 @@ """ -def panda( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> HDFPanda: - return device_instantiation( - device_factory=HDFPanda, - name="panda", - prefix="-EA-PANDA-01:", - wait=wait_for_connection, - fake=fake_with_ophyd_sim, +@device_factory() +def panda() -> HDFPanda: + return HDFPanda( + f"{PREFIX.beamline_prefix}-EA-PANDA-01:", path_provider=get_path_provider(), ) -def synchrotron( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> Synchrotron: - return device_instantiation( - Synchrotron, - "synchrotron", - "", - wait_for_connection, - fake_with_ophyd_sim, - bl_prefix=False, - ) +@device_factory() +def synchrotron() -> Synchrotron: + return Synchrotron() -def manta( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> AravisDetector: - return device_instantiation( - AravisDetector, - "manta", - "-DI-DCAM-02:", - wait_for_connection, - fake_with_ophyd_sim, +@device_factory() +def manta() -> AravisDetector: + return AravisDetector( + f"{PREFIX.beamline_prefix}-DI-DCAM-02:", path_provider=get_path_provider(), drv_suffix="CAM:", - hdf_suffix="HDF5:", + hdf_suffix=HDF5_PREFIX, ) From cb7717c2e1f73dc696227e98cb44acb16c4fd7f8 Mon Sep 17 00:00:00 2001 From: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com> Date: Wed, 27 Nov 2024 10:51:46 +0000 Subject: [PATCH 3/4] Update p99 to use device_factory (#920) Co-authored-by: Raymond Fan --- src/dodal/beamlines/p99.py | 75 ++++++++------------------- src/dodal/devices/motors.py | 24 +++++---- src/dodal/devices/p99/sample_stage.py | 28 +++++----- 3 files changed, 48 insertions(+), 79 deletions(-) diff --git a/src/dodal/beamlines/p99.py b/src/dodal/beamlines/p99.py index fd8b9dfbfb..23598c11e8 100644 --- a/src/dodal/beamlines/p99.py +++ b/src/dodal/beamlines/p99.py @@ -1,61 +1,30 @@ -from dodal.common.beamlines.beamline_utils import device_instantiation, set_beamline +from dodal.common.beamlines.beamline_utils import device_factory, set_beamline from dodal.devices.motors import XYZPositioner from dodal.devices.p99.sample_stage import FilterMotor, SampleAngleStage from dodal.log import set_beamline as set_log_beamline -from dodal.utils import get_beamline_name +from dodal.utils import BeamlinePrefix, get_beamline_name -BL = get_beamline_name("BL99P") +BL = get_beamline_name("p99") +PREFIX = BeamlinePrefix(BL) set_log_beamline(BL) set_beamline(BL) -def sample_angle_stage( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> SampleAngleStage: - """Sample stage for p99""" - - return device_instantiation( - SampleAngleStage, - prefix="-MO-STAGE-01:", - name="sample_angle_stage", - wait=wait_for_connection, - fake=fake_with_ophyd_sim, - ) - - -def sample_stage_filer( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> FilterMotor: - """Sample stage for p99""" - - return device_instantiation( - FilterMotor, - prefix="-MO-STAGE-02:MP:SELECT", - name="sample_stage_filer", - wait=wait_for_connection, - fake=fake_with_ophyd_sim, - ) - - -def sample_xyz_stage( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> XYZPositioner: - return device_instantiation( - FilterMotor, - prefix="-MO-STAGE-02:", - name="sample_xyz_stage", - wait=wait_for_connection, - fake=fake_with_ophyd_sim, - ) - - -def sample_lab_xyz_stage( - wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False -) -> XYZPositioner: - return device_instantiation( - FilterMotor, - prefix="-MO-STAGE-02:LAB:", - name="sample_lab_xyz_stage", - wait=wait_for_connection, - fake=fake_with_ophyd_sim, - ) +@device_factory() +def angle_stage() -> SampleAngleStage: + return SampleAngleStage(f"{PREFIX.beamline_prefix}-MO-STAGE-01:") + + +@device_factory() +def filter() -> FilterMotor: + return FilterMotor(f"{PREFIX.beamline_prefix}-MO-STAGE-02:MP:SELECT") + + +@device_factory() +def sample_stage() -> XYZPositioner: + return XYZPositioner(f"{PREFIX.beamline_prefix}-MO-STAGE-02:") + + +@device_factory() +def lab_stage() -> XYZPositioner: + return XYZPositioner(f"{PREFIX.beamline_prefix}-MO-STAGE-02:LAB:") diff --git a/src/dodal/devices/motors.py b/src/dodal/devices/motors.py index 4237082598..c7b7911d91 100644 --- a/src/dodal/devices/motors.py +++ b/src/dodal/devices/motors.py @@ -1,8 +1,8 @@ -from ophyd_async.core import Device +from ophyd_async.core import StandardReadable from ophyd_async.epics.motor import Motor -class XYZPositioner(Device): +class XYZPositioner(StandardReadable): """ Standard ophyd_async xyz motor stage, by combining 3 Motors, @@ -15,7 +15,7 @@ class XYZPositioner(Device): name: name for the stage. infix: - EPICS PV, default is the ["X", "Y", "Z"]. + EPICS PV, default is the ("X", "Y", "Z"). Notes ----- Example usage:: @@ -23,14 +23,18 @@ class XYZPositioner(Device): xyz_stage = XYZPositioner("BLXX-MO-STAGE-XX:") Or:: with DeviceCollector(): - xyz_stage = XYZPositioner("BLXX-MO-STAGE-XX:", suffix = ["A", "B", "C"]) + xyz_stage = XYZPositioner("BLXX-MO-STAGE-XX:", infix = ("A", "B", "C")) """ - def __init__(self, prefix: str, name: str, infix: list[str] | None = None): - if infix is None: - infix = ["X", "Y", "Z"] - self.x = Motor(prefix + infix[0]) - self.y = Motor(prefix + infix[1]) - self.z = Motor(prefix + infix[2]) + def __init__( + self, + prefix: str, + name: str = "", + infix: tuple[str, str, str] = ("X", "Y", "Z"), + ): + with self.add_children_as_readables(): + self.x = Motor(prefix + infix[0]) + self.y = Motor(prefix + infix[1]) + self.z = Motor(prefix + infix[2]) super().__init__(name=name) diff --git a/src/dodal/devices/p99/sample_stage.py b/src/dodal/devices/p99/sample_stage.py index e9f1026d25..a1eff7db45 100644 --- a/src/dodal/devices/p99/sample_stage.py +++ b/src/dodal/devices/p99/sample_stage.py @@ -1,18 +1,13 @@ -from ophyd_async.core import Device, SubsetEnum -from ophyd_async.epics.core import epics_signal_rw +from ophyd_async.core import StandardReadable, SubsetEnum +from ophyd_async.epics.core import epics_signal_rw, epics_signal_rw_rbv -class SampleAngleStage(Device): - def __init__(self, prefix: str, name: str): - self.theta = epics_signal_rw( - float, prefix + "WRITETHETA:RBV", prefix + "WRITETHETA" - ) - self.roll = epics_signal_rw( - float, prefix + "WRITEROLL:RBV", prefix + "WRITEROLL" - ) - self.pitch = epics_signal_rw( - float, prefix + "WRITEPITCH:RBV", prefix + "WRITEPITCH" - ) +class SampleAngleStage(StandardReadable): + def __init__(self, prefix: str, name: str = ""): + with self.add_children_as_readables(): + self.theta = epics_signal_rw_rbv(float, prefix + "WRITETHETA", ":RBV") + self.roll = epics_signal_rw_rbv(float, prefix + "WRITEROLL", ":RBV") + self.pitch = epics_signal_rw_rbv(float, prefix + "WRITEPITCH", ":RBV") super().__init__(name=name) @@ -35,7 +30,8 @@ class p99StageSelections(SubsetEnum): User = "User" -class FilterMotor(Device): - def __init__(self, prefix: str, name: str): - self.user_setpoint = epics_signal_rw(p99StageSelections, prefix) +class FilterMotor(StandardReadable): + def __init__(self, prefix: str, name: str = ""): + with self.add_children_as_readables(): + self.user_setpoint = epics_signal_rw(p99StageSelections, prefix) super().__init__(name=name) From 5e9522108fe9462cab45df3f587406124dd3fa59 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 28 Nov 2024 10:46:33 +0000 Subject: [PATCH 4/4] Add CODEOWNERS file pointing to review team (#902) * Add CODEOWNERS file pointing to review team * Write ADR for CODEOWNERS * Write reviewer criteria and review standards --- .github/CODEOWNERS | 1 + .../explanations/decisions/0003-codeowners.md | 19 +++++++ docs/explanations/reviews.md | 56 +++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 .github/CODEOWNERS create mode 100644 docs/explanations/decisions/0003-codeowners.md create mode 100644 docs/explanations/reviews.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000000..03ec19a5a7 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1 @@ +* @DiamondLightSource/bluesky-reviewers diff --git a/docs/explanations/decisions/0003-codeowners.md b/docs/explanations/decisions/0003-codeowners.md new file mode 100644 index 0000000000..c3b0782054 --- /dev/null +++ b/docs/explanations/decisions/0003-codeowners.md @@ -0,0 +1,19 @@ +# 3. Use CODEOWNERS Github feature to restrict approved reviewers + +## Status + +Accepted + +## Context + +As dodal expands and covers an increasing number of beamlines it is important to maintain quality. So far we have seen good results from following bluesky's own standards: Encouraging high test coverage, readable code (including type annotations, where appropriate) and collective ownership, including via thorough reviews which appropriately balance velocity with technical debt management. We would like to make sure this developer culture is preserved as we expand. + +## Decision + +Have a team of approved reviewers in the DiamondLightSource Github organisation. Every PR into dodal **must** be reviewed by a member of this team before merging (enforced by repo settings and [CODEOWNERS feature](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)). Have a set of [documented criteria](../reviews.md) for becoming part of the team, with the expectation that any team that regularly works on dodal will become members over time. The hope is that a consistent set of standards can be maintained by the approver community. + +## Consequences + +PRs will not be mergable until an approved owner has reviewed them, approved individuals/teams will have to review/support onboarding of new individuals/teams for some time before they are approved for review. + +Once a critical mass of Diamond staff are used to this way of working, this ADR may be obsolete and should be replaced with a more open review system. diff --git a/docs/explanations/reviews.md b/docs/explanations/reviews.md new file mode 100644 index 0000000000..78a9ab48b2 --- /dev/null +++ b/docs/explanations/reviews.md @@ -0,0 +1,56 @@ +# Approved Reviewers + +Code reviews are an important tool for ensuring both quality and collective responsibility for the codebase. Dodal PRs *must* be reviewed by a member of [the approved review team](https://github.com/orgs/DiamondLightSource/teams/bluesky-reviewers) before they are merged. + +## Review Standards and Practices + +When reviewing you should ensure the code meets the [repository standards](../reference/standards.rst) and [device standards](../reference/device-standards.rst). Approved reviewers will adhere to the following, if they do not, junior reviewers and reviewees should feel empowered to hold them to account. + +### Questions and Comments + +Questions and requests for clarification are encouraged. If you have comments for the reviewer you could add them with the following prefixes to be helpful: + +- **Must**: This will negatively effect functionality if not implemented. The PR will not be merged until the comment is addressed to both the reviewer and developer's satisfaction. +- **Should**: This will improve quality/performance/etc. is but unlikely to affect functionality. If the developer doesn't want to do this they should write an explanation. +- **Could**: Similar to **Should** but without the requirement for explicit justification. The developer can dismiss on the grounds that velocity is the priority at the moment. +- **Nit**: Preference only, the developer can dismiss without any justification. + +### The Happy Path + +If the review passes then the reviewer should approve the change and either the reviewer or PR author should merge it. + +### Requesting Changes + +If the review fails the reviewer should request any changes (this issue is said to require rework) +A developer should ideally pick up any rework PRs before starting new work +When a developer is happy that a rework is complete they should press the re-request review button on the PR + +### Making Changes + +Reviewers should feel free to make small changes to a PR as part of a review e.g. typos, poorly named variables, fixing a test. However, if code has been changed in this way they need then need to run it past another developer (ideally the original developer). This means that no code is merged into main without being looked at by at least 2 people, ensuring collective responsibility for the codebase. + +### Dismissing Stale Reviews + +There may be times where one person requests changes on a PR, these changes are addressed and another person then approves it. In this case the PR still cannot be merged until the original review is dismissed. We're happy for people to dismiss stale reviews with the understanding that: + +- The new reviewer should check that the must and should comments have been addressed +- If it's a big change it may still be worth the original reviewer looking again, if so they should make this clear to the original reviewer and the person that wrote the code + +## Joining the Review Team + +At least two members of the review team from two different DLS teams *must* approve adding a new member, neither of them can be on the new member's DLS team. At least one of the approvers should ideally be a senior engineer but this will not always be possible. + +In the event of a rejection the review team should provide concise and actionable feedback and set a date for re-consideration. + +### Criteria for New Members + +New members should be regular contributors to dodal and should have been "shadow-reviewing" approved reviewers for a period of weeks or months before becoming approved. In both their contributions and reviews they should be consistently demonstrating the following *without prompting*: + +- A good understanding of Python, including advanced langauge features such as [generators](https://wiki.python.org/moin/Generators), [coroutines](https://docs.python.org/3/library/asyncio-task.html), [comprehensions](https://www.geeksforgeeks.org/comprehensions-in-python/) and [pattern matching](https://peps.python.org/pep-0636/). +- A pythonic coding style, demonstrated by using appropriate class/variable names, aherence to [PEP 8](https://peps.python.org/pep-0008). +- Adherence to the review standards above as well as the [repository standards](../reference/standards.rst) and [device standards](../reference/device-standards.rst). +- Independent (i.e. not just to satisfy a reviewer) motivation to make sure all code in dodal is well-tested. Use of unit and system tests as appropriate. Clear effort made to keep test coverage high (>90%). +- Advanced understanding of bluesky and ophyd-async including concepts and best practices, such as how to appropriately split logic between devices/plans/callbacks. +- Humility in the use of reviewing as a tool in a way that balances the need to preserve quality with the need for progress. Appropriate use of the Must/Should/Could/Nit system documented above is helpful in showing this, as it gives the reviewer the opportunity to weight their comments in terms of project impact. They should also demonstrate similar humiliary as a reviewee. + +Additionally, they should be regularly raising issues in the repository and demonstrating the ability to write well formed issues, with well defined acceptance criteria, that are understandable without large amounts of context.