diff --git a/.github/workflows/_container.yml b/.github/workflows/_container.yml index 6e8f1d313..115a21f08 100644 --- a/.github/workflows/_container.yml +++ b/.github/workflows/_container.yml @@ -51,6 +51,7 @@ jobs: # Docker cache and publish it with: context: . + file: Dockerfile.release push: true tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} diff --git a/src/mx_bluesky/hyperion/device_setup_plans/manipulate_sample.py b/src/mx_bluesky/hyperion/device_setup_plans/manipulate_sample.py index b071a7588..858ceadfd 100644 --- a/src/mx_bluesky/hyperion/device_setup_plans/manipulate_sample.py +++ b/src/mx_bluesky/hyperion/device_setup_plans/manipulate_sample.py @@ -78,11 +78,11 @@ def move_x_y_z( axes are optional.""" LOGGER.info(f"Moving smargon to x, y, z: {(x_mm, y_mm, z_mm)}") - if x_mm: + if x_mm is not None: yield from bps.abs_set(smargon.x, x_mm, group=group) - if y_mm: + if y_mm is not None: yield from bps.abs_set(smargon.y, y_mm, group=group) - if z_mm: + if z_mm is not None: yield from bps.abs_set(smargon.z, z_mm, group=group) if wait: yield from bps.wait(group) @@ -100,11 +100,11 @@ def move_phi_chi_omega( axes are optional.""" LOGGER.info(f"Moving smargon to phi, chi, omega: {(phi, chi, omega)}") - if phi: + if phi is not None: yield from bps.abs_set(smargon.phi, phi, group=group) - if chi: + if chi is not None: yield from bps.abs_set(smargon.chi, chi, group=group) - if omega: + if omega is not None: yield from bps.abs_set(smargon.omega, omega, group=group) if wait: yield from bps.wait(group) diff --git a/src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py b/src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py index 2b752ab99..e38e35b30 100755 --- a/src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py +++ b/src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py @@ -79,6 +79,8 @@ from mx_bluesky.hyperion.parameters.gridscan import HyperionThreeDGridScan from mx_bluesky.hyperion.utils.context import device_composite_from_context +ZOCALO_MIN_TOTAL_COUNT_THRESHOLD = 3 + class SmargonSpeedException(Exception): pass @@ -247,10 +249,20 @@ def run_gridscan_and_fetch_results( LOGGER.info("Zocalo triggered and read, interpreting results.") xrc_results = yield from get_full_processing_results(fgs_composite.zocalo) LOGGER.info(f"Got xray centres, top 5: {xrc_results[:5]}") - if xrc_results: + filtered_results = [ + result + for result in xrc_results + if result["total_count"] >= ZOCALO_MIN_TOTAL_COUNT_THRESHOLD + ] + discarded_count = len(xrc_results) - len(filtered_results) + if discarded_count > 0: + LOGGER.info( + f"Removed {discarded_count} results because below threshold" + ) + if filtered_results: flyscan_results = [ _xrc_result_in_boxes_to_result_in_mm(xr, parameters) - for xr in xrc_results + for xr in filtered_results ] else: LOGGER.warning("No X-ray centre received") diff --git a/src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py b/src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py index ac2fa90dc..1534d1db7 100644 --- a/src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py +++ b/src/mx_bluesky/hyperion/experiment_plans/robot_load_then_centre_plan.py @@ -1,10 +1,12 @@ from __future__ import annotations +from math import isclose from typing import cast import bluesky.preprocessors as bpp import pydantic from blueapi.core import BlueskyContext +from bluesky import plan_stubs as bps from bluesky.utils import MsgGenerator from dodal.devices.aperturescatterguard import ApertureScatterguard from dodal.devices.attenuator import Attenuator @@ -174,7 +176,11 @@ def robot_load_then_xray_centre( yield from pin_already_loaded(composite.robot, sample_location) ) - doing_chi_change = parameters.chi_start_deg is not None + current_chi = yield from bps.rd(composite.smargon.chi) + LOGGER.info(f"Read back current smargon chi of {current_chi} degrees.") + doing_chi_change = parameters.chi_start_deg is not None and not isclose( + current_chi, parameters.chi_start_deg, abs_tol=0.001 + ) if doing_sample_load: LOGGER.info("Pin not loaded, loading and centring") diff --git a/tests/system_tests/hyperion/external_interaction/conftest.py b/tests/system_tests/hyperion/external_interaction/conftest.py index a4dabf378..0c45557fc 100644 --- a/tests/system_tests/hyperion/external_interaction/conftest.py +++ b/tests/system_tests/hyperion/external_interaction/conftest.py @@ -69,9 +69,9 @@ { "centre_of_mass": [1, 2, 3], "max_voxel": [2, 4, 5], - "max_count": 105062, + "max_count": 50000, "n_voxels": 35, - "total_count": 2387574, + "total_count": 100000, "bounding_box": [[1, 2, 3], [3, 4, 4]], } ] @@ -79,12 +79,22 @@ { "centre_of_mass": [1, 2, 3], "max_voxel": [1, 2, 3], - "max_count": 105062, + "max_count": 1000, "n_voxels": 35, - "total_count": 1387574, + "total_count": 1000, "bounding_box": [[2, 2, 2], [3, 3, 3]], } ] +TEST_RESULT_BELOW_THRESHOLD = [ + { + "centre_of_mass": [2, 3, 4], + "max_voxel": [2, 3, 4], + "max_count": 2, + "n_voxels": 1, + "total_count": 2, + "bounding_box": [[1, 2, 3], [2, 3, 4]], + } +] def get_current_datacollection_comment(Session: Callable, dcid: int) -> str: diff --git a/tests/unit_tests/hyperion/device_setup_plans/test_manipulate_sample.py b/tests/unit_tests/hyperion/device_setup_plans/test_manipulate_sample.py index e32c0d844..67b611469 100644 --- a/tests/unit_tests/hyperion/device_setup_plans/test_manipulate_sample.py +++ b/tests/unit_tests/hyperion/device_setup_plans/test_manipulate_sample.py @@ -1,12 +1,13 @@ from unittest.mock import MagicMock, call, patch -import numpy as np import pytest from bluesky.run_engine import RunEngine from dodal.devices.aperturescatterguard import ApertureScatterguard, ApertureValue from mx_bluesky.hyperion.device_setup_plans.manipulate_sample import ( move_aperture_if_required, + move_phi_chi_omega, + move_x_y_z, ) from mx_bluesky.hyperion.experiment_plans.flyscan_xray_centre_plan import ( FlyScanXRayCentreComposite, @@ -43,36 +44,85 @@ async def test_move_aperture_does_nothing_when_none_selected( mock_set.assert_not_called() +@pytest.mark.parametrize( + "motor_position, expected_moves", + [ + [[1, 2, 3], [1, 2, 3]], + [[0, 0, 0], [None, None, None]], + [[None, None, None], [None, None, None]], + [[1, 0, 0], [1, 0, 0]], + [[0, 1, 0], [0, 1, 0]], + [[0, 0, 1], [0, 0, 1]], + [[1, None, None], [1, None, None]], + [[None, 1, None], [None, 1, None]], + [[None, None, 1], [None, None, 1]], + ], +) @patch("bluesky.plan_stubs.abs_set", autospec=True) -def test_results_passed_to_move_motors( +def test_move_x_y_z( bps_abs_set: MagicMock, test_fgs_params: HyperionThreeDGridScan, fake_fgs_composite: FlyScanXRayCentreComposite, RE: RunEngine, + motor_position: list[float], + expected_moves: list[float | None], ): - from mx_bluesky.hyperion.device_setup_plans.manipulate_sample import move_x_y_z - - motor_position = test_fgs_params.FGS_params.grid_position_to_motor_position( - np.array([1, 2, 3]) - ) - RE(move_x_y_z(fake_fgs_composite.sample_motors, *motor_position)) - bps_abs_set.assert_has_calls( - [ - call( + RE(move_x_y_z(fake_fgs_composite.sample_motors, *motor_position)) # type: ignore + expected_calls = [ + call(axis, pos, group="move_x_y_z") + for axis, pos in zip( + [ fake_fgs_composite.sample_motors.x, - motor_position[0], - group="move_x_y_z", - ), - call( fake_fgs_composite.sample_motors.y, - motor_position[1], - group="move_x_y_z", - ), - call( fake_fgs_composite.sample_motors.z, - motor_position[2], - group="move_x_y_z", - ), - ], + ], + expected_moves, + strict=False, + ) + if pos is not None + ] + bps_abs_set.assert_has_calls( + expected_calls, + any_order=True, + ) + + +@pytest.mark.parametrize( + "motor_position, expected_moves", + [ + [[1, 2, 3], [1, 2, 3]], + [[0, 0, 0], [0, 0, 0]], + [[0, None, None], [0, None, None]], + [[None, 0, None], [None, 0, None]], + [[None, None, 0], [None, None, 0]], + [[None, None, None], [None, None, None]], + [[1, 0, 0], [1, 0, 0]], + ], +) +@patch("bluesky.plan_stubs.abs_set", autospec=True) +def test_move_phi_chi_omega( + bps_abs_set: MagicMock, + test_fgs_params: HyperionThreeDGridScan, + fake_fgs_composite: FlyScanXRayCentreComposite, + RE: RunEngine, + motor_position: list[float], + expected_moves: list[float | None], +): + RE(move_phi_chi_omega(fake_fgs_composite.sample_motors, *motor_position)) # type: ignore + expected_calls = [ + call(axis, pos, group="move_phi_chi_omega") + for axis, pos in zip( + [ + fake_fgs_composite.sample_motors.phi, + fake_fgs_composite.sample_motors.chi, + fake_fgs_composite.sample_motors.omega, + ], + expected_moves, + strict=False, + ) + if pos is not None + ] + bps_abs_set.assert_has_calls( + expected_calls, any_order=True, ) diff --git a/tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py b/tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py index 64224de4c..63f18f137 100644 --- a/tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py +++ b/tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py @@ -35,6 +35,7 @@ FlyScanXRayCentreComposite, SmargonSpeedException, XRayCentreEventHandler, + _FeatureControlled, _get_feature_controlled, flyscan_xray_centre, flyscan_xray_centre_no_move, @@ -76,6 +77,7 @@ from ....conftest import simulate_xrc_result from ....system_tests.hyperion.external_interaction.conftest import ( + TEST_RESULT_BELOW_THRESHOLD, TEST_RESULT_LARGE, TEST_RESULT_MEDIUM, TEST_RESULT_SMALL, @@ -154,6 +156,14 @@ def mock_ispyb(): return MagicMock() +@pytest.fixture +def feature_controlled( + fake_fgs_composite: FlyScanXRayCentreComposite, + test_fgs_params_panda_zebra: HyperionThreeDGridScan, +) -> _FeatureControlled: + return _get_feature_controlled(fake_fgs_composite, test_fgs_params_panda_zebra) + + def _custom_msg(command_name: str): return lambda *args, **kwargs: iter([Msg(command_name)]) @@ -328,12 +338,9 @@ async def test_results_adjusted_and_event_raised( move_aperture: MagicMock, fgs_composite_with_panda_pcap: FlyScanXRayCentreComposite, test_fgs_params_panda_zebra: HyperionThreeDGridScan, + feature_controlled: _FeatureControlled, RE_with_subs: ReWithSubs, ): - feature_controlled = _get_feature_controlled( - fgs_composite_with_panda_pcap, - test_fgs_params_panda_zebra, - ) RE, _ = RE_with_subs x_ray_centre_event_handler = XRayCentreEventHandler() @@ -508,10 +515,8 @@ async def test_when_gridscan_finished_then_dev_shm_disabled( RE_with_subs: ReWithSubs, test_fgs_params_panda_zebra: HyperionThreeDGridScan, fgs_composite_with_panda_pcap: FlyScanXRayCentreComposite, + feature_controlled: _FeatureControlled, ): - feature_controlled = _get_feature_controlled( - fgs_composite_with_panda_pcap, test_fgs_params_panda_zebra - ) RE, (nexus_cb, ispyb_cb) = RE_with_subs test_fgs_params_panda_zebra.features.set_stub_offsets = True @@ -552,12 +557,9 @@ def test_when_gridscan_succeeds_ispyb_comment_appended_to( RE_with_subs: ReWithSubs, test_fgs_params_panda_zebra: HyperionThreeDGridScan, fgs_composite_with_panda_pcap: FlyScanXRayCentreComposite, + feature_controlled: _FeatureControlled, ): RE, (nexus_cb, ispyb_cb) = RE_with_subs - feature_controlled = _get_feature_controlled( - fgs_composite_with_panda_pcap, - test_fgs_params_panda_zebra, - ) def _wrapped_gridscan_and_move(): run_generic_ispyb_handler_setup(ispyb_cb, test_fgs_params_panda_zebra) @@ -637,12 +639,9 @@ def test_when_gridscan_finds_no_xtal_ispyb_comment_appended_to( RE_with_subs: ReWithSubs, test_fgs_params_panda_zebra: HyperionThreeDGridScan, fgs_composite_with_panda_pcap: FlyScanXRayCentreComposite, + feature_controlled: _FeatureControlled, ): RE, (nexus_cb, ispyb_cb) = RE_with_subs - feature_controlled = _get_feature_controlled( - fgs_composite_with_panda_pcap, - test_fgs_params_panda_zebra, - ) def wrapped_gridscan_and_move(): run_generic_ispyb_handler_setup(ispyb_cb, test_fgs_params_panda_zebra) @@ -682,12 +681,9 @@ def test_when_gridscan_finds_no_xtal_exception_is_raised( RE_with_subs: ReWithSubs, test_fgs_params_panda_zebra: HyperionThreeDGridScan, fgs_composite_with_panda_pcap: FlyScanXRayCentreComposite, + feature_controlled: _FeatureControlled, ): RE, (nexus_cb, ispyb_cb) = RE_with_subs - feature_controlled = _get_feature_controlled( - fgs_composite_with_panda_pcap, - test_fgs_params_panda_zebra, - ) def wrapped_gridscan_and_move(): run_generic_ispyb_handler_setup(ispyb_cb, test_fgs_params_panda_zebra) @@ -900,10 +896,8 @@ def test_fgs_arms_eiger_without_grid_detect( test_fgs_params_panda_zebra: HyperionThreeDGridScan, RE: RunEngine, done_status: Status, + feature_controlled: _FeatureControlled, ): - feature_controlled = _get_feature_controlled( - fake_fgs_composite, test_fgs_params_panda_zebra - ) fake_fgs_composite.eiger.unstage = MagicMock(return_value=done_status) RE( run_gridscan( @@ -938,14 +932,11 @@ def test_when_grid_scan_fails_with_exception_then_detector_disarmed_and_correct_ fake_fgs_composite: FlyScanXRayCentreComposite, test_fgs_params_panda_zebra: HyperionThreeDGridScan, RE: RunEngine, + feature_controlled: _FeatureControlled, ): class CompleteException(Exception): pass - feature_controlled = _get_feature_controlled( - fake_fgs_composite, - test_fgs_params_panda_zebra, - ) mock_complete.side_effect = CompleteException() fake_fgs_composite.eiger.stage = MagicMock( @@ -1052,10 +1043,8 @@ def test_read_hardware_during_collection_occurs_after_eiger_arm( fake_fgs_composite: FlyScanXRayCentreComposite, test_fgs_params_panda_zebra: HyperionThreeDGridScan, sim_run_engine: RunEngineSimulator, + feature_controlled: _FeatureControlled, ): - feature_controlled = _get_feature_controlled( - fake_fgs_composite, test_fgs_params_panda_zebra - ) sim_run_engine.add_handler( "read", lambda msg: {"values": {"value": SynchrotronMode.USER}}, @@ -1093,16 +1082,12 @@ def test_if_smargon_speed_over_limit_then_log_error( mock_kickoff_and_complete: MagicMock, test_fgs_params_panda_zebra: HyperionThreeDGridScan, fake_fgs_composite: FlyScanXRayCentreComposite, + feature_controlled: _FeatureControlled, RE: RunEngine, ): test_fgs_params_panda_zebra.x_step_size_um = 10000 test_fgs_params_panda_zebra.detector_params.exposure_time = 0.01 - feature_controlled = _get_feature_controlled( - fake_fgs_composite, - test_fgs_params_panda_zebra, - ) - # this exception should only be raised if we're using the panda try: RE( @@ -1114,3 +1099,30 @@ def test_if_smargon_speed_over_limit_then_log_error( assert test_fgs_params_panda_zebra.features.use_panda_for_gridscan else: assert not test_fgs_params_panda_zebra.features.use_panda_for_gridscan + + @patch( + "mx_bluesky.hyperion.experiment_plans.flyscan_xray_centre_plan.kickoff_and_complete_gridscan", + MagicMock(), + ) + def test_run_gridscan_and_fetch_results_discards_results_below_threshold( + self, + fake_fgs_composite: FlyScanXRayCentreComposite, + test_fgs_params_panda_zebra: HyperionThreeDGridScan, + feature_controlled: _FeatureControlled, + RE: RunEngine, + ): + callback = XRayCentreEventHandler() + RE.subscribe(callback) + + mock_zocalo_trigger( + fake_fgs_composite.zocalo, + TEST_RESULT_MEDIUM + TEST_RESULT_BELOW_THRESHOLD + TEST_RESULT_SMALL, + ) + RE( + run_gridscan_and_fetch_results( + fake_fgs_composite, test_fgs_params_panda_zebra, feature_controlled + ) + ) + + assert callback.xray_centre_results and len(callback.xray_centre_results) == 2 + assert [r.max_count for r in callback.xray_centre_results] == [50000, 1000] diff --git a/utility_scripts/deploy/create_venv.py b/utility_scripts/deploy/create_venv.py index 3d30250bb..39716a208 100644 --- a/utility_scripts/deploy/create_venv.py +++ b/utility_scripts/deploy/create_venv.py @@ -1,15 +1,11 @@ import os import sys -from subprocess import PIPE, CalledProcessError, Popen +from subprocess import CalledProcessError, Popen def run_process_and_print_output(proc_to_run): - with Popen( - proc_to_run, stdout=PIPE, stderr=PIPE, bufsize=1, universal_newlines=True - ) as p: - if p.stdout is not None: - for line in p.stdout: - print(line, end="") + with Popen(proc_to_run, bufsize=1, universal_newlines=True) as p: + p.communicate() if p.returncode != 0: raise CalledProcessError(p.returncode, p.args) diff --git a/utility_scripts/deploy/deploy_hyperion_to_k8s.sh b/utility_scripts/deploy/deploy_hyperion_to_k8s.sh index d06652276..7cc2c6556 100755 --- a/utility_scripts/deploy/deploy_hyperion_to_k8s.sh +++ b/utility_scripts/deploy/deploy_hyperion_to_k8s.sh @@ -28,6 +28,10 @@ for option in "$@"; do LOGIN=false shift ;; + --dry-run) + DRY_RUN=true + shift + ;; --help|--info|--h) CMD=`basename $0` echo "$CMD [options] " @@ -40,11 +44,12 @@ the container, NOT the directory that you built the container image from. --help This help --appVersion=version Version of the image to fetch from the repository otherwise it is deduced - from the setuptools_scm + from the setuptools_scm. Must be in the format x.y.z -b, --beamline=BEAMLINE Overrides the BEAMLINE environment variable with the given beamline --checkout-to-prod Checkout source folders to the production folder using deploy_mx_bluesky.py --dev Install to a development kubernetes cluster (assumes project checked out under /home) (default cluster is argus in user namespace) + --dry-run Do everything but don't do the final deploy to k8s --no-login Do not attempt to log in to kubernetes instead use the current namespace and cluster --repository=REPOSITORY Override the repository to fetch the image from EOM @@ -98,12 +103,12 @@ else fi fi - NEW_PROJECTDIR=$HYPERION_BASE/hyperion + NEW_PROJECTDIR=$HYPERION_BASE/mx-bluesky echo "Changing directory to $NEW_PROJECTDIR..." cd $NEW_PROJECTDIR PROJECTDIR=$NEW_PROJECTDIR HYPERION_BASENAME=$(basename $HYPERION_BASE) - CHECKED_OUT_VERSION=${HYPERION_BASENAME#mx_bluesky_} + CHECKED_OUT_VERSION=${HYPERION_BASENAME#mx-bluesky_v} fi @@ -125,8 +130,8 @@ ensure_version_py() { module load python/3.11 python -m venv $PROJECTDIR/.venv . $PROJECTDIR/.venv/bin/activate - pip install setuptools_scm fi + pip install setuptools_scm } app_version() { @@ -149,7 +154,9 @@ echo "Checked out version that will be bind-mounted in $PROJECTDIR is $CHECKED_O echo "Container image version that will be pulled is $APP_VERSION" if [[ $APP_VERSION != $CHECKED_OUT_VERSION ]]; then + echo "*****************************************************************" echo "WARNING: Checked out version and container image versions differ!" + echo "*****************************************************************" fi if [[ -n $DEV ]]; then @@ -166,10 +173,10 @@ hyperion.externalHostname=test-hyperion.diamond.ac.uk " mkdir -p $PROJECTDIR/tmp/data DEPLOYMENT_DIR=$PROJECTDIR else - DEPLOYMENT_DIR=/dls_sw/i03/software/bluesky/mx_bluesky_${APP_VERSION}/hyperion + DEPLOYMENT_DIR=/dls_sw/i03/software/bluesky/mx-bluesky_v${APP_VERSION}/mx-bluesky fi -HELM_OPTIONS+="--set hyperion.appVersion=$APP_VERSION,\ +HELM_OPTIONS+="--set hyperion.appVersion=v$APP_VERSION,\ hyperion.projectDir=$DEPLOYMENT_DIR,\ dodal.projectDir=$DEPLOYMENT_DIR/../dodal " @@ -181,4 +188,6 @@ if [[ $LOGIN = true ]]; then module load $CLUSTER kubectl config set-context --current --namespace=$NAMESPACE fi -helm upgrade --install $HELM_OPTIONS $RELEASE hyperion-0.0.1.tgz +if [[ -z $DRY_RUN ]]; then + helm upgrade --install $HELM_OPTIONS $RELEASE hyperion-0.0.1.tgz +fi diff --git a/utility_scripts/deploy/deploy_mx_bluesky.py b/utility_scripts/deploy/deploy_mx_bluesky.py index 58896838c..f78ef2ebe 100644 --- a/utility_scripts/deploy/deploy_mx_bluesky.py +++ b/utility_scripts/deploy/deploy_mx_bluesky.py @@ -6,6 +6,7 @@ import os import re import subprocess +import sys from typing import NamedTuple from uuid import uuid1 @@ -121,14 +122,11 @@ def _create_environment_from_control_machine( process = None try: # Call python script on i03-control to create the environment - process = subprocess.Popen( - cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE - ) - stdout, stderr = process.communicate() + process = subprocess.Popen(cmd, shell=True, text=True, bufsize=1) + process.communicate() if process.returncode != 0: - print(f"Error occurred: {stderr.decode()}") - else: - print(f"Output: {stdout.decode()}") + print("Error occurred, exiting") + sys.exit(1) except Exception as e: print(f"Exception while trying to install venv on i03-control: {e}") finally: