diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e0680fe5..a476aa23 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -88,18 +88,6 @@ ruff-checks: - ruff format --check # Only run on changed python files -# Increased checking of the code with ruff this is allowed to fail -# while we improve the code quality. Rules from this are -# slowly moved into the main check -# Only runs when a .py file is edited -ruff-lint-increased: - stage: analysis - extends: .python - allow_failure: true - script: - - ruff --config ruff-increased-checking.toml check - - # Python type checking with Mypy # Only runs when a .py file is edited mypy: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b171d161..a108e8c9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,7 +41,7 @@ Ideally, python code should be: The server code is going through significant flux during the transition from v2 to v3, so currently our test coverage is being rebuilt and type checking does not yet pass. -[Ruff](https://docs.astral.sh/ruff/) is used both for linting and formatting the codebase. Our custom linter rules are in our `pyproject.toml`. As we improve the codebase, more rules are being added to the linter; see `ruff-increased-checking.toml` for checks that we plan to get passing. +[Ruff](https://docs.astral.sh/ruff/) is used both for linting and formatting the codebase. Our custom linter rules are in our `pyproject.toml`. Run `ruff format` to format the code in a way that will pass on our CI. diff --git a/check_version_sync.py b/check_version_sync.py index 23c13b1b..40a4628e 100755 --- a/check_version_sync.py +++ b/check_version_sync.py @@ -14,7 +14,8 @@ with open(os.path.join("webapp", "package.json"), "r", encoding="utf-8") as json py_ver = pyproject_data["project"]["version"] js_ver = webapp_data["version"] -assert py_ver == js_ver, ( +# Allow an assert here as this is a CI script, not production code. +assert py_ver == js_ver, ( # noqa S101 "The python and javascript version numbers should match.\n" f"Python version: {py_ver}\n" f"Javascript version: {js_ver}\n" diff --git a/hardware-specific-tests/picamera2/test_acquisition.py b/hardware-specific-tests/picamera2/test_acquisition.py index 3ab404eb..caf8849a 100644 --- a/hardware-specific-tests/picamera2/test_acquisition.py +++ b/hardware-specific-tests/picamera2/test_acquisition.py @@ -3,14 +3,14 @@ from fastapi.testclient import TestClient from PIL import Image import numpy as np -from pytest import fixture +import pytest import labthings_fastapi as lt from openflexure_microscope_server.things.camera.picamera import StreamingPiCamera2 -@fixture(scope="module") +@pytest.fixture(scope="module") def client() -> lt.ThingClient: """Initialise a test client for the StreamingPiCamera2 Thing. diff --git a/hardware-specific-tests/picamera2/test_calibration.py b/hardware-specific-tests/picamera2/test_calibration.py index 7c56a9c8..6c803aed 100644 --- a/hardware-specific-tests/picamera2/test_calibration.py +++ b/hardware-specific-tests/picamera2/test_calibration.py @@ -14,7 +14,7 @@ from openflexure_microscope_server.things.camera.picamera import ( ) -@pytest.fixture() +@pytest.fixture def picamera_thing() -> StreamingPiCamera2: """Return a StreamingPiCamera2 Thing. @@ -24,7 +24,7 @@ def picamera_thing() -> StreamingPiCamera2: return StreamingPiCamera2() -@pytest.fixture() +@pytest.fixture def client(picamera_thing) -> lt.ThingClient: """Initialise a test client for the StreamingPiCamera2 Thing. diff --git a/hardware-specific-tests/picamera2/test_tuning.py b/hardware-specific-tests/picamera2/test_tuning.py index b193131d..d2733b8f 100644 --- a/hardware-specific-tests/picamera2/test_tuning.py +++ b/hardware-specific-tests/picamera2/test_tuning.py @@ -68,9 +68,6 @@ def _test_bad_tuning_after_good_tuning(configure: bool = False): with pytest.raises(IndexError): # The bad version should cause a problem cam = Picamera2(tuning=bad_tuning) - print_tuning() - print("Success (not expected)!") - del cam recalibrate_utils.recreate_camera_manager() with Picamera2(tuning=default_tuning) as cam: # Reload the camera with working tuning, or it will stop responding diff --git a/picamera_coverage.zip b/picamera_coverage.zip index d20a51c0..95750fa8 100644 Binary files a/picamera_coverage.zip and b/picamera_coverage.zip differ diff --git a/pyproject.toml b/pyproject.toml index 9957a3b6..dffc3831 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,7 +34,7 @@ dependencies = [ [project.optional-dependencies] dev = [ - "ruff", + "ruff~=0.13.0", "mypy[reports]", "mypy-gitlab-code-quality", # "pytest-gitlab-code-quality", # pytest version constraint clashes with labthings @@ -100,32 +100,29 @@ testpaths = ["tests"] line-ending = "native" [tool.ruff.lint] - -# Slowly add linting rules from ruff-increased-checking.toml commented -# out rules can be checked by running -# ruff --config ruff-increased-checking.toml check select = [ "E", # pycodestyle errors "W", #pycodestyle warnings "F", # PyFlakes -# "PL", # Pylint (a subset of, catches far less than pylint does) + "PL", # Pylint (a subset of, catches far less than pylint does) "B", # Flake8 bugbear "A", # Flake8 builtins checker "C", # Flake8 comprehensions -# "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks. + "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks. # If they are not fixed before merge they should be converted to GitLab issues -# "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G") -# "T20", # Warns for print statements, production code should log -# "PT", # pytest linting + "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G") + "T20", # Warns for print statements, production code should log + "PT", # pytest linting "RET", # Consistent clear return statements "RSE", # Raise parentheses -# "SIM", # Simplifications detected + "SIM", # Simplifications detected "ARG", # unused arguments "C90", # McCabe complexity! "NPY", # Numpy linting "N", # PEP8 naming "D", # Docstring checks "ERA001", # Commented out code! + "S101",# No asserts! "ANN", ] @@ -136,20 +133,35 @@ ignore = [ "D400", # A stricter version of #415 that doesn't allow ! "ANN401", # Disalows Any, Any is needed at times, Once MyPy is running Any will be # handled appropriately + "PLR2004", # Ignore magic values in comparison for now. It doesn't seem we can set + # allowed magic values and a number of our magic values are not really + # magic (such as 255 when doing uint8 maths) + "PT011", # Ignore pytest.raises being used on too general expectations without a + # match. We may need to revisit this. + "SIM105", # Not enforcing use of `contextlib.suppress(NotConnectedToServerError)` + # instead of `try`-`except`-`pass` ] [tool.ruff.lint.per-file-ignores] -# Tests are currently not fully docstring-ed, we'll ignore this for now. -"tests/*" = [ +# All testing dirs +"{tests,integration-tests,hardware-specific-tests}/**" = [ "B018", # Complaining about useless attribute access in tests, but we need them to check errors are raised "ANN", # Tests are not typehinted for fixtures etc + "S101", # Allow asserts in tests + "T20", # Allow prints in tests + "LOG015", # Allow tests to use the root logger. + "PLC0415", # Allow tests to import later as this is needed for mocking ] -"hardware-specific-tests/*" = [ - "B018", # Complaining about useless attribute access in tests, but we need them to check errors are raised - "ANN", # Tests are not typehinted for fixtures etc +# Developer CLI scripts +"{pull_webapp,picamera_tests,change_log_helper}.py" = [ + "T20", # Printing is normal in small CLI scripts ] [tool.ruff.lint.pydocstyle] # This lets the D401 checker understand that decorated thing properties and thing # settings act like properties so should be documented as such. property-decorators = ["labthings_fastapi.thing_property", "labthings_fastapi.thing_setting"] + +[tool.ruff.lint.pylint] +max-args = 7 +max-positional-args = 5 \ No newline at end of file diff --git a/ruff-increased-checking.toml b/ruff-increased-checking.toml deleted file mode 100644 index fd63f8fe..00000000 --- a/ruff-increased-checking.toml +++ /dev/null @@ -1,41 +0,0 @@ -# This can be passed into ruff with nano ruff-increased-checking.toml -# -# Once the codebase is cleaner this can be added to pyproject.toml -# under the [tool.ruff.lint] heading -# -# Select more than the bare minimum checking in ruff -# This is only a subset of possible rules -# See: https://docs.astral.sh/ruff/rules -# -# It will take a long time to get this all passing. We should -# slowly move rules into the pyproject.toml as we improve the -# codebase - -select = [ - "E", # pycodestyle errors - "W", #pycodestyle warnings - "F", # PyFlakes - "PL", # Pylint (a subset of, catches far less than pylint does) - "B", # Flake8 bugbear - "A", # Flake8 builtins checker - "C", # Flake8 comprehensions - "FIX", #TODO/FIXME's in code. These should be added during development for ongoing tasks. - # If they are not fixed before merge they should be converted to GitLab issues - "LOG", # Flake8 logging issues (pedantic logger formatting issues can be added with "G") - "T20", # Warns for print statements, production code should log - "PT", # pytest linting - "RET", # Consistent clear return statements - "RSE", # Raise parentheses - "SIM", # Simplifications detected - "ARG", # unused arguments - "C90", # McCabe complexity! - "NPY", # Numpy linting - "N", # PEP8 naming - "D", # Docstring checks these may need to be added gradually - "ERA001", # Commented out code! -] - -# Line length is set to 88 for ruff. This is what the formatter aims for -# some lines are not formatted to 88 if the formatter can't easily break -# them. Allow up to 99 before throwing a long line error -line-length = 99 diff --git a/src/openflexure_microscope_server/logging.py b/src/openflexure_microscope_server/logging.py index 0dd3ee12..ff4f8de7 100644 --- a/src/openflexure_microscope_server/logging.py +++ b/src/openflexure_microscope_server/logging.py @@ -18,6 +18,7 @@ import os from fastapi.responses import PlainTextResponse from fastapi import HTTPException +LOGGER = logging.getLogger(__name__) OFM_LOG_FILE = None @@ -33,7 +34,8 @@ def configure_logging(log_folder: str) -> None: root_logger = logging.getLogger() root_logger.setLevel(logging.INFO) # Explicitly make OFM_LOG_FILE a global so it can be updated based on log settings - global OFM_LOG_FILE + # This requires silencing PLW0603 which disallows globals. + global OFM_LOG_FILE # noqa: PLW0603 OFM_LOG_FILE = os.path.join(log_folder, "openflexure_microscope.log") # Add OFM_HANDLER first so it can capture the error log if the @@ -58,12 +60,9 @@ def configure_logging(log_folder: str) -> None: root_logger.addHandler(handler) except PermissionError as e: - logging.error(f"Cannot create log file at {OFM_LOG_FILE}: {e}") + LOGGER.error(f"Cannot create log file at {OFM_LOG_FILE}: {e}") - logging.info("") - logging.info("****************************************************") - logging.info("OFM server root logger has been set up at INFO level") - logging.info("****************************************************") + LOGGER.info("OFM server root logger has been set up at INFO level") def retrieve_log() -> PlainTextResponse: diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py index 8256f414..e3449588 100644 --- a/src/openflexure_microscope_server/scan_directories.py +++ b/src/openflexure_microscope_server/scan_directories.py @@ -210,9 +210,8 @@ class ScanDirectoryManager: not exist. """ file_path = os.path.join(self.path_for(scan_name), filename) - if check_exists: - if not os.path.exists(file_path): - return None + if check_exists and not os.path.exists(file_path): + return None return file_path @requires_lock @@ -225,9 +224,8 @@ class ScanDirectoryManager: then the path is returned anyway """ file_path = os.path.join(self.img_dir_for(scan_name), filename) - if check_exists: - if not os.path.exists(file_path): - return None + if check_exists and not os.path.exists(file_path): + return None return file_path def get_final_stitch_path(self, scan_name: str) -> Optional[str]: @@ -528,7 +526,8 @@ class ScanDirectory: """ zip_fname = os.path.join(self.dir_path, "images.zip") - if os.path.isfile(zip_fname): + # Use noqa as converting this into a 1 liner is not more readable. + if os.path.isfile(zip_fname): # noqa: SIM108 # get a list of files in the existing zip zip_files = get_files_in_zip(zip_fname) else: diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py index 4ef066bf..7387e70d 100644 --- a/src/openflexure_microscope_server/scan_planners.py +++ b/src/openflexure_microscope_server/scan_planners.py @@ -163,10 +163,7 @@ class ScanPlanner: # If focussed locations exist return closest location, favouring most recent closest_pos = self.closest_focus_site(next_location) - if closest_pos is None: - z = None - else: - z = closest_pos[2] + z = None if closest_pos is None else closest_pos[2] return next_location, z @@ -363,10 +360,7 @@ class SmartSpiral(ScanPlanner): # If focused locations exist, return the neighbour with the lowest z position closest_pos = self.select_nearby_focus_site(next_location) - if closest_pos is None: - z = None - else: - z = closest_pos[2] + z = None if closest_pos is None else closest_pos[2] return next_location, z diff --git a/src/openflexure_microscope_server/server/__init__.py b/src/openflexure_microscope_server/server/__init__.py index 3dfbb721..1f14fbd3 100644 --- a/src/openflexure_microscope_server/server/__init__.py +++ b/src/openflexure_microscope_server/server/__init__.py @@ -14,6 +14,8 @@ from .serve_static_files import add_static_files from .legacy_api import add_v2_endpoints from ..logging import configure_logging, retrieve_log, retrieve_log_from_file +LOGGER = logging.getLogger(__name__) + def set_shutdown_function(shutdown_function: Callable[[], None]) -> None: """Ensure a function is called before the shutdown. @@ -93,7 +95,7 @@ def serve_from_cli(argv: Optional[list[str]] = None) -> None: # Catch anything and log as it is essential that this # function cannot raise an unhandled exception or Uvicorn # will never get a shutdown signal. - logging.error(e, exc_info=True) + LOGGER.error(e, exc_info=True) # Monkey patch uvicorn's exit handling to stop the MJPEG Streams # before waiting for background tasks to complete. @@ -109,9 +111,11 @@ def serve_from_cli(argv: Optional[list[str]] = None) -> None: except BaseException as e: if args.fallback: - print(f"Error: {e}") + # Allow printing to the terminal for fallback errors so they are not + # presented in the fallback logs. + print(f"Error: {e}") # noqa: T201 fallback_server = "labthings_fastapi.server.fallback:app" - print(f"Starting fallback server {fallback_server}.") + print(f"Starting fallback server {fallback_server}.") # noqa: T201 app = lt.cli.object_reference_to_object(fallback_server) app.labthings_config = config app.labthings_server = server diff --git a/src/openflexure_microscope_server/server/legacy_api.py b/src/openflexure_microscope_server/server/legacy_api.py index e877f939..5e939116 100644 --- a/src/openflexure_microscope_server/server/legacy_api.py +++ b/src/openflexure_microscope_server/server/legacy_api.py @@ -21,10 +21,9 @@ def add_v2_endpoints(thing_server: lt.ThingServer) -> None: """Add the v2 API endpoints for OpenFlexure Connect discoverability.""" app = thing_server.app - # TODO: update openflexure connect to make this unnecessary!! # The endpoints below fool OpenFlexure Connect into thinking we are a # v2 microscope, so we show up correctly. - # This is necessary until Connect is rebuilt. + # This is necessary until Connect is rebuilt. See #557. @app.get("/routes") def routes_stub() -> dict[str, dict]: """Return a stub list of routes. diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index c81bd34d..09d72753 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -165,9 +165,7 @@ class PreviewStitcher(BaseStitcher): with self._popen_lock: if self._popen_obj is None: return False - if self._popen_obj.poll() is None: - return True - return False + return self._popen_obj.poll() is None def wait(self, cancel: lt.deps.CancelHook) -> None: """Wait for this preview stitch to return. diff --git a/src/openflexure_microscope_server/things/autofocus.py b/src/openflexure_microscope_server/things/autofocus.py index e0b956f1..88ad91eb 100644 --- a/src/openflexure_microscope_server/things/autofocus.py +++ b/src/openflexure_microscope_server/things/autofocus.py @@ -25,6 +25,8 @@ from .camera import RawCameraDependency as RawCamera from .camera import CameraDependency as CameraClient from .stage import StageDependency as Stage +LOGGER = logging.getLogger(__name__) + class NotStreamingError(RuntimeError): """No images captured from stream. The camera is almost certainly not streaming.""" @@ -228,7 +230,7 @@ class JPEGSharpnessMonitor: self.camera = camera self.stage = stage self.portal = portal - print(f"Created sharpness monitor with {stage}, {camera}, {portal}") + LOGGER.debug(f"Created sharpness monitor with {stage}, {camera}, {portal}") self.stage_positions: list[Mapping[str, int]] = [] self.stage_times: list[float] = [] self.jpeg_times: list[float] = [] @@ -307,7 +309,7 @@ class JPEGSharpnessMonitor: raise e if stop < 1: stop = len(jpeg_times) - logging.debug("changing stop to %s", (stop)) + LOGGER.debug("changing stop to %s", (stop)) jpeg_times = jpeg_times[start:stop] jpeg_heights: np.ndarray = np.interp(jpeg_times, stage_times, stage_heights) return jpeg_times, jpeg_heights, jpeg_sizes[start:stop] @@ -704,7 +706,10 @@ class AutofocusThing(lt.Thing): sharpness=cam.grab_jpeg_size(stream_name="lores"), ) - def check_stack_result( + # Silence too many returns in this situation as refactoring to reduce returns is + # unlikely to improve readability. This function is basically a complex switch + # statement, having an explicit return after each option is clear. + def check_stack_result( # noqa: PLR0911 self, captures: list[CaptureInfo] ) -> tuple[Literal["success", "continue", "restart"], int]: """Check if the sharpest image in a list of captures is central enough. diff --git a/src/openflexure_microscope_server/things/camera/opencv.py b/src/openflexure_microscope_server/things/camera/opencv.py index c159ce41..a4e5f81a 100644 --- a/src/openflexure_microscope_server/things/camera/opencv.py +++ b/src/openflexure_microscope_server/things/camera/opencv.py @@ -21,6 +21,8 @@ from labthings_fastapi.types.numpy import NDArray from . import BaseCamera +LOGGER = logging.getLogger(__name__) + class OpenCVCamera(BaseCamera): """A Thing that provides and interface to an OpenCV Camera.""" @@ -70,9 +72,7 @@ class OpenCVCamera(BaseCamera): while self._capture_enabled: ret, frame = self.cap.read() if not ret: - logging.error( - f"Failed to capture frame from camera {self.camera_index}" - ) + LOGGER.error(f"Failed to capture frame from camera {self.camera_index}") break jpeg = cv2.imencode(".jpg", frame)[1].tobytes() self.mjpeg_stream.add_frame(jpeg, portal) @@ -99,8 +99,8 @@ class OpenCVCamera(BaseCamera): binary image formats will be added in due course. """ if wait is not None: - logging.warning("OpenCV camera has no wait option. Use None.") - logging.warning(f"OpenCV camera doesn't respect {stream_name=}") + LOGGER.warning("OpenCV camera has no wait option. Use None.") + LOGGER.warning(f"OpenCV camera doesn't respect {stream_name=}") ret, frame = self.cap.read() if not ret: raise RuntimeError( @@ -118,6 +118,6 @@ class OpenCVCamera(BaseCamera): This function will produce a JPEG image. """ if wait is not None: - logging.warning("OpenCV camera has no wait option. Use None.") - logging.warning(f"OpenCV camera doesn't respect {stream_name=}") + LOGGER.warning("OpenCV camera has no wait option. Use None.") + LOGGER.warning(f"OpenCV camera doesn't respect {stream_name=}") return Image.fromarray(self.capture_array()) diff --git a/src/openflexure_microscope_server/things/camera/picamera.py b/src/openflexure_microscope_server/things/camera/picamera.py index be01b337..0158c56e 100644 --- a/src/openflexure_microscope_server/things/camera/picamera.py +++ b/src/openflexure_microscope_server/things/camera/picamera.py @@ -47,6 +47,8 @@ from . import picamera_tuning_file_utils as tf_utils from . import BaseCamera, ArrayModel +LOGGER = logging.getLogger(__name__) + SUPPORTED_CAMS_SENSOR_INFO = { "picamera_v2": recalibrate_utils.IMX219_SENSOR_INFO, "picamera_hq": recalibrate_utils.IMX477_SENSOR_INFO, @@ -397,16 +399,16 @@ class StreamingPiCamera2(BaseCamera): os.environ["LIBCAMERA_RPI_TUNING_FILE"] = tuning_file.name if self._picamera is not None: - logging.info("Closing picamera object for reinitialisation") - logging.info( + LOGGER.info("Closing picamera object for reinitialisation") + LOGGER.info( "Camera object already exists, closing for reinitialisation" ) self._picamera.close() - logging.info("Picamera closed, deleting picamera") + LOGGER.info("Picamera closed, deleting picamera") del self._picamera recalibrate_utils.recreate_camera_manager() - logging.info("Creating new Picamera2 object") + LOGGER.info("Creating new Picamera2 object") # Specify tuning file otherwise it will be overwritten with None. self._picamera = Picamera2( camera_num=self._camera_num, @@ -519,7 +521,7 @@ class StreamingPiCamera2(BaseCamera): ) stream_config["buffer_count"] = buffer_count picam.configure(stream_config) - logging.info("Starting picamera MJPEG stream...") + LOGGER.info("Starting picamera MJPEG stream...") stream_name = "lores" if main_resolution[0] > 1280 else "main" picam.start_recording( MJPEGEncoder(self.mjpeg_bitrate), @@ -538,11 +540,11 @@ class StreamingPiCamera2(BaseCamera): name="lores", ) except Exception as e: - logging.exception("Error while starting preview: {e}") - logging.exception(e) + LOGGER.exception("Error while starting preview: {e}") + LOGGER.exception(e) else: self.stream_active = True - logging.debug( + LOGGER.debug( "Started MJPEG stream at %s on port %s", self.stream_resolution, 1 ) @@ -553,15 +555,15 @@ class StreamingPiCamera2(BaseCamera): try: picam.stop_recording() # This should also stop the extra lores encoder except Exception as e: - logging.info("Stopping recording failed") - logging.exception(e) + LOGGER.info("Stopping recording failed") + LOGGER.exception(e) else: self.stream_active = False if stop_web_stream: portal = lt.get_blocking_portal(self) self.mjpeg_stream.stop(portal) self.lores_mjpeg_stream.stop(portal) - logging.info("Stopped MJPEG stream.") + LOGGER.info("Stopped MJPEG stream.") # Adding a sleep to prevent camera getting confused by rapid commands time.sleep(self._sensor_info.short_pause) @@ -579,7 +581,7 @@ class StreamingPiCamera2(BaseCamera): Restarts stream when complete. """ with self._streaming_picamera(pause_stream=True) as cam: - logging.debug("Reconfiguring camera for full resolution capture") + LOGGER.debug("Reconfiguring camera for full resolution capture") cam.configure(cam.create_still_configuration(sensor=self._sensor_mode)) cam.start() time.sleep(self._sensor_info.short_pause) diff --git a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py b/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py index 5f38ee6e..ff15e02d 100644 --- a/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py +++ b/src/openflexure_microscope_server/things/camera/picamera_recalibrate_utils.py @@ -52,6 +52,8 @@ from scipy.ndimage import zoom from picamera2 import Picamera2 import picamera2 +LOGGER = logging.getLogger(__name__) + class SensorInfo(BaseModel): """Information about the sensor used for calibration and property setting.""" @@ -167,7 +169,7 @@ def adjust_shutter_and_gain_from_raw( # Check whether the shutter speed is still going up - if not, we've hit a maximum if camera.capture_metadata()["ExposureTime"] == test.exposure_time: - logging.info(f"Shutter speed has maxed out at {test.exposure_time}") + LOGGER.info(f"Shutter speed has maxed out at {test.exposure_time}") break # Now, if we've not converged, increase gain until we converge or run out of options. @@ -185,13 +187,13 @@ def adjust_shutter_and_gain_from_raw( # Check the gain is still changing - if not, we have probably hit the maximum if camera.capture_metadata()["AnalogueGain"] == test.analog_gain: - logging.info(f"Gain has maxed out at {test.analog_gain}") + LOGGER.info(f"Gain has maxed out at {test.analog_gain}") break if _check_convergence(test, target_white_level, tolerance): - logging.info(f"Brightness has converged to within {tolerance * 100:.0f}%.") + LOGGER.info(f"Brightness has converged to within {tolerance * 100:.0f}%.") else: - logging.warning( + LOGGER.warning( f"Failed to reach target brightness of {target_white_level}." f"Brightness reached {test.level} after {iterations} iterations." ) @@ -199,9 +201,13 @@ def adjust_shutter_and_gain_from_raw( return test.level -def adjust_white_balance_from_raw( +# Explicitly allow this to have 8 arguments as the later arguments are keyword only +# We should be able to enforce this without noqa once PyLint moves PLR0917 out of +# preview +def adjust_white_balance_from_raw( # noqa: PLR0913 camera: Picamera2, sensor_info: SensorInfo, + *, percentile: float = 99, luminance: Optional[np.ndarray] = None, Cr: Optional[np.ndarray] = None, @@ -230,11 +236,9 @@ def adjust_white_balance_from_raw( channel_gains = 1 / grids if channel_gains.shape[1:] != channels.shape[1:]: channel_gains = _upsample_channels(channel_gains, channels.shape[1:]) - logging.info( - f"Before gains, channel maxima are {np.max(channels, axis=(1, 2))}" - ) + LOGGER.info(f"Before gains, channel maxima are {np.max(channels, axis=(1, 2))}") channels = channels * channel_gains - logging.info(f"After gains, channel maxima are {np.max(channels, axis=(1, 2))}") + LOGGER.info(f"After gains, channel maxima are {np.max(channels, axis=(1, 2))}") if method == "centre": _, height, width = channels.shape # Cut out the central 10% from 9/20 to 11/20... @@ -261,7 +265,7 @@ def adjust_white_balance_from_raw( # Here, we decrease the gains by the minimum value of Cr and Cb. new_awb_gains = (green / red * np.min(Cr), green / blue * np.min(Cb)) - logging.info( + LOGGER.info( f"Raw white point is R: {red} G: {green} B: {blue}, " f"setting AWB gains to ({new_awb_gains[0]:.2f}, " f"{new_awb_gains[1]:.2f})." @@ -270,7 +274,7 @@ def adjust_white_balance_from_raw( camera.controls.ColourGains = new_awb_gains time.sleep(sensor_info.long_pause) m = camera.capture_metadata() - print(f"Camera confirms gains are now {m['ColourGains']}") + LOGGER.debug(f"Camera confirms gains are now {m['ColourGains']}") return new_awb_gains @@ -334,7 +338,7 @@ def _test_exposure_settings(camera: Picamera2, percentile: float) -> _ExposureTe # because of black level compensation. The line below forces a # minimum value of 1 which will keep things well-behaved! if max_brightness < 1: - logging.warning( + LOGGER.warning( f"Measured brightness of {max_brightness}. " "This should normally be >= 1, and may indicate the " "camera's black level compensation has gone wrong." @@ -346,7 +350,7 @@ def _test_exposure_settings(camera: Picamera2, percentile: float) -> _ExposureTe exposure_time=int(metadata["ExposureTime"]), analog_gain=float(metadata["AnalogueGain"]), ) - logging.info(f"{result.model_dump()}") + LOGGER.info(f"{result.model_dump()}") return result @@ -490,5 +494,5 @@ def _raw_channels_from_camera( # de-mosaicing has been done, so 2/3 of the values are zero (3/4 for R and B # channels, 1/2 for green because there's twice as many green pixels). raw_format = camera.camera_configuration()["raw"]["format"] - print(f"Acquired a raw image in format {raw_format}") + LOGGER.debug(f"Acquired a raw image in format {raw_format}") return _channels_from_bayer_array(raw_image) diff --git a/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py b/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py index 00e5a0d3..f86753c7 100644 --- a/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py +++ b/src/openflexure_microscope_server/things/camera/picamera_tuning_file_utils.py @@ -38,7 +38,8 @@ def set_static_lst( adaptive tweaking by the algorithm. """ for table in luminance, cr, cb: - assert np.array(table).shape == (12, 16), "Lens shading tables must be 12x16!" + if np.array(table).shape != (12, 16): + raise ValueError("Lens shading tables must be 12x16!") alsc = Picamera2.find_tuning_algo(tuning, "rpi.alsc") alsc["n_iter"] = 0 # disable the adaptive part alsc["luminance_strength"] = 1.0 diff --git a/src/openflexure_microscope_server/things/camera/simulation.py b/src/openflexure_microscope_server/things/camera/simulation.py index 80dadb55..37df5d1c 100644 --- a/src/openflexure_microscope_server/things/camera/simulation.py +++ b/src/openflexure_microscope_server/things/camera/simulation.py @@ -30,6 +30,8 @@ from openflexure_microscope_server.ui import ( from . import BaseCamera, ArrayModel from ..stage import BaseStage +LOGGER = logging.getLogger(__name__) + # The ratio between "motor" steps and pixels # higher related to a faster movement RATIO = 0.2 @@ -204,7 +206,7 @@ class SimulatedCamera(BaseCamera): try: pos = self.get_stage_position() except Exception as e: - print(f"Failed to get stage position: {e}") + LOGGER.debug(f"Failed to get stage position: {e}") pos = {"x": 0, "y": 0, "z": 0} return self.generate_image((pos["y"], pos["x"], pos["z"])) @@ -238,7 +240,7 @@ class SimulatedCamera(BaseCamera): If called while already streaming, the warning will be emitted and no other action will be taken. """ - logging.warning( + LOGGER.warning( f"Simulation camera doesn't respect {main_resolution=} or {buffer_count=} " "arguments." ) @@ -271,7 +273,7 @@ class SimulatedCamera(BaseCamera): jpeg_lores = cv2.imencode(".jpg", ds_frame)[1].tobytes() self.lores_mjpeg_stream.add_frame(jpeg_lores, portal) except Exception as e: - logging.exception(f"Failed to capture frame: {e}, retrying...") + LOGGER.exception(f"Failed to capture frame: {e}, retrying...") @lt.thing_action def discard_frames(self) -> None: @@ -293,8 +295,8 @@ class SimulatedCamera(BaseCamera): binary image formats will be added in due course. """ if wait is not None: - logging.warning("Simulation camera has no wait option. Use None.") - logging.warning(f"Simulation camera camera doesn't respect {stream_name=}") + LOGGER.warning("Simulation camera has no wait option. Use None.") + LOGGER.warning(f"Simulation camera camera doesn't respect {stream_name=}") return self.generate_frame() def capture_image( @@ -307,8 +309,8 @@ class SimulatedCamera(BaseCamera): It is used for capture to memory. """ if wait is not None: - logging.warning("Simulation camera has no wait option. Use None.") - logging.warning(f"Simulation camera camera doesn't respect {stream_name=}") + LOGGER.warning("Simulation camera has no wait option. Use None.") + LOGGER.warning(f"Simulation camera camera doesn't respect {stream_name=}") return Image.fromarray(self.generate_frame()) @lt.thing_action diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py index ae723880..1b760e13 100644 --- a/src/openflexure_microscope_server/things/smart_scan.py +++ b/src/openflexure_microscope_server/things/smart_scan.py @@ -334,9 +334,8 @@ class SmartScanThing(lt.Thing): """Manage the stitching threads, starting them if needed and not already running.""" # Assume 4 images means at least one offset in x and y, making the stitching # well constrained. - if self._scan_data.image_count > 3: - if not self._preview_stitcher.running: - self._preview_stitcher.start() + if self._scan_data.image_count > 3 and not self._preview_stitcher.running: + self._preview_stitcher.start() @_scan_running def _run_scan(self) -> None: diff --git a/src/openflexure_microscope_server/things/stage/sangaboard.py b/src/openflexure_microscope_server/things/stage/sangaboard.py index 932fa373..733d4099 100644 --- a/src/openflexure_microscope_server/things/stage/sangaboard.py +++ b/src/openflexure_microscope_server/things/stage/sangaboard.py @@ -15,6 +15,8 @@ import labthings_fastapi as lt from . import BaseStage +LOGGER = logging.getLogger(__name__) + class SangaboardThing(BaseStage): """A Thing to manage a Sangaboard motor controller. @@ -168,7 +170,7 @@ class SangaboardThing(BaseStage): # cannot be used. intended_brightness = float(return_value[7:]) on_brightness = 0.32 - logging.warning( + LOGGER.warning( "Brightness control is not yet implemented. Desired brightness: " f"{intended_brightness}. Set brightness: {on_brightness}" ) diff --git a/src/openflexure_microscope_server/things/system.py b/src/openflexure_microscope_server/things/system.py index d2b022bb..ba70f065 100644 --- a/src/openflexure_microscope_server/things/system.py +++ b/src/openflexure_microscope_server/things/system.py @@ -52,7 +52,6 @@ class OpenFlexureSystem(lt.Thing): @microscope_id.setter def microscope_id(self, uuid: UUID) -> None: - # TODO make read only but still settable from disk self._microscope_id = uuid @lt.thing_property diff --git a/tests/test_camera.py b/tests/test_camera.py index f3cfd1c7..39a35d89 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -54,7 +54,11 @@ def test_handle_broken_frame(): portal = lt.get_blocking_portal(camera) # Check that this does cause broken frames. - with pytest.raises(OSError, match="broken data stream when reading image file"): + # The noqa is because we don't know exactly when the error is thrown so we + # can't have a single simple statement in the pytest raises. + with pytest.raises( # noqa PT012 + OSError, match="broken data stream when reading image file" + ): for _i in range(15): jpeg = camera.grab_jpeg(portal) np.asarray(Image.open(jpeg.open())) diff --git a/tests/test_dummy_server.py b/tests/test_dummy_server.py index 66230e1b..3f911992 100644 --- a/tests/test_dummy_server.py +++ b/tests/test_dummy_server.py @@ -42,8 +42,8 @@ def thing_server(): server.add_thing(CameraStageMapper(), "/camera_stage_mapping/") assert os.path.exists(os.path.join(temp_folder.name, "camera/")) # Note: yield is important. If return is used the temp folder gets deleted - # before the test runs - yield server + # before the test runs. Silence PT022 as ruff doesn't think yield is needed. + yield server # noqa: PT022 @pytest.fixture diff --git a/tests/test_logging.py b/tests/test_logging.py index bdeec6d2..85d59888 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -22,15 +22,14 @@ def test_no_warnings_if_correct_permissions(caplog): """ # Reset handler at start of test ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler() - with caplog.at_level(logging.WARNING): - with tempfile.TemporaryDirectory() as tmpdir: - ofm_logging.configure_logging(tmpdir) - assert len(caplog.records) == 0 - with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file: - log_txt = log_file.read() - assert "OFM server root logger has been set up at INFO level" in log_txt - root_logger = logging.getLogger() - assert ofm_logging.OFM_HANDLER in root_logger.handlers + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + ofm_logging.configure_logging(tmpdir) + assert len(caplog.records) == 0 + with open(ofm_logging.OFM_LOG_FILE, "r", encoding="utf-8") as log_file: + log_txt = log_file.read() + assert "OFM server root logger has been set up at INFO level" in log_txt + root_logger = logging.getLogger() + assert ofm_logging.OFM_HANDLER in root_logger.handlers def test_permission_error_raises_warning(mocker, caplog): @@ -41,26 +40,24 @@ def test_permission_error_raises_warning(mocker, caplog): ) # Reset handler at start of test ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler() - with caplog.at_level(logging.WARNING): - with tempfile.TemporaryDirectory() as tmpdir: - ofm_logging.configure_logging(tmpdir) - assert len(caplog.records) == 1 - # Check OFM logger is added even if the file logger couldn't be. - root_logger = logging.getLogger() - assert ofm_logging.OFM_HANDLER in root_logger.handlers + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + ofm_logging.configure_logging(tmpdir) + assert len(caplog.records) == 1 + # Check OFM logger is added even if the file logger couldn't be. + root_logger = logging.getLogger() + assert ofm_logging.OFM_HANDLER in root_logger.handlers def test_making_log_dir(caplog): """Check that configure_logging will make a dir if needed.""" # Reset handler at start of test ofm_logging.OFM_HANDLER = ofm_logging.OFMHandler() - with caplog.at_level(logging.WARNING): - with tempfile.TemporaryDirectory() as tmpdir: - log_dir = os.path.join(tmpdir, "new_dir") - assert not os.path.isdir(log_dir) - ofm_logging.configure_logging(log_dir) - assert len(caplog.records) == 0 - assert os.path.isdir(log_dir) + with caplog.at_level(logging.WARNING), tempfile.TemporaryDirectory() as tmpdir: + log_dir = os.path.join(tmpdir, "new_dir") + assert not os.path.isdir(log_dir) + ofm_logging.configure_logging(log_dir) + assert len(caplog.records) == 0 + assert os.path.isdir(log_dir) def test_max_logs(): @@ -161,13 +158,13 @@ FAKE_UVICORN_LOGGER = logging.getLogger("uvicorn.error") @pytest.mark.parametrize( - "log_command, names_in_log, names_not_in_log", + ("log_command", "names_in_log", "names_not_in_log"), [ - [FAKE_UVICORN_LOGGER.debug, [], ["", ""]], - [FAKE_UVICORN_LOGGER.info, [""], [""]], - [FAKE_UVICORN_LOGGER.warning, [""], [""]], - [FAKE_UVICORN_LOGGER.error, [""], [""]], - [FAKE_UVICORN_LOGGER.exception, [""], [""]], + (FAKE_UVICORN_LOGGER.debug, [], ["", ""]), + (FAKE_UVICORN_LOGGER.info, [""], [""]), + (FAKE_UVICORN_LOGGER.warning, [""], [""]), + (FAKE_UVICORN_LOGGER.error, [""], [""]), + (FAKE_UVICORN_LOGGER.exception, [""], [""]), ], ) def test_uvicorn_error_only_says_error_on_error( diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py index 0b1adca2..89996945 100644 --- a/tests/test_scan_planners.py +++ b/tests/test_scan_planners.py @@ -23,8 +23,8 @@ def test_enforce_xy_tuple(): with pytest.raises(TypeError): scan_planners.enforce_xy_tuple(value) - assert (1, 6) == scan_planners.enforce_xy_tuple((1, 6)) - assert (1, 6) == scan_planners.enforce_xy_tuple([1, 6]) + assert scan_planners.enforce_xy_tuple((1, 6)) == (1, 6) + assert scan_planners.enforce_xy_tuple([1, 6]) == (1, 6) def test_enforce_xyz_tuple(): @@ -39,8 +39,8 @@ def test_enforce_xyz_tuple(): with pytest.raises(TypeError): scan_planners.enforce_xyz_tuple(value) - assert (1, 6, 2) == scan_planners.enforce_xyz_tuple((1, 6, 2)) - assert (1, 6, 6) == scan_planners.enforce_xyz_tuple([1, 6, 6]) + assert scan_planners.enforce_xyz_tuple((1, 6, 2)) == (1, 6, 2) + assert scan_planners.enforce_xyz_tuple([1, 6, 6]) == (1, 6, 6) def test_base_class_not_implemented(): diff --git a/tests/test_serve_static_files.py b/tests/test_serve_static_files.py index 8ec1a82c..9755cc1c 100644 --- a/tests/test_serve_static_files.py +++ b/tests/test_serve_static_files.py @@ -26,12 +26,12 @@ def mock_static_dir(): @pytest.mark.parametrize( - "filename, allow_cache", + ("filename", "allow_cache"), [ - ["foo", False], - ["woff2.foo", False], - ["strange_file_ending_in_woff2", False], - ["fontfile.woff2", True], + ("foo", False), + ("woff2.foo", False), + ("strange_file_ending_in_woff2", False), + ("fontfile.woff2", True), ], ) def test_add_static_file(filename, allow_cache, mocker): diff --git a/tests/test_server_config.py b/tests/test_server_config.py index 565f02c6..3074d707 100644 --- a/tests/test_server_config.py +++ b/tests/test_server_config.py @@ -86,8 +86,8 @@ def test_customise_server(mocker): @pytest.mark.parametrize( - "config_file, expected_scan_dir", - [[FULL_CONFIG, "/var/openflexure/scans/"], [SIM_CONFIG, "./openflexure/scans/"]], + ("config_file", "expected_scan_dir"), + [(FULL_CONFIG, "/var/openflexure/scans/"), (SIM_CONFIG, "./openflexure/scans/")], ) def test_get_scans_dir_ok(config_file, expected_scan_dir): """Test the _get_scans_dir function and also check the standard config files.""" diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py index ab619e8a..394778eb 100644 --- a/tests/test_smart_scan.py +++ b/tests/test_smart_scan.py @@ -120,20 +120,20 @@ def test_public_delete_scan(smart_scan_thing, caplog): # Attempt to delete the fake scan. Expect it to fail with pytest.raises(HTTPException) as exc_info: smart_scan_thing.delete_scan(fake_scan_name, LOGGER) - # Should raise a 400 error if the scan doesn't exist, not a 404 as the server - # was not expecting to receive the scan files - assert exc_info.value.status_code == 400 - assert len(caplog.records) == 1 - assert caplog.records[0].levelname == "WARNING" - assert caplog.records[0].name == "mock-invocation_logger" + # Should raise a 400 error if the scan doesn't exist, not a 404 as the server + # was not expecting to receive the scan files + assert exc_info.value.status_code == 400 + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert caplog.records[0].name == "mock-invocation_logger" - # Make a dir for the fake scan and delete it. - os.makedirs(fake_scan_path) - assert os.path.exists(fake_scan_path) - smart_scan_thing.delete_scan(fake_scan_name, LOGGER) - assert not os.path.exists(fake_scan_path) - # Check no extra logs generated - assert len(caplog.records) == 1 + # Make a dir for the fake scan and delete it. + os.makedirs(fake_scan_path) + assert os.path.exists(fake_scan_path) + smart_scan_thing.delete_scan(fake_scan_name, LOGGER) + assert not os.path.exists(fake_scan_path) + # Check no extra logs generated + assert len(caplog.records) == 1 def test_delete_all_scans(smart_scan_thing, caplog): diff --git a/tests/test_version_strings.py b/tests/test_version_strings.py index ea700618..b1a9fa50 100644 --- a/tests/test_version_strings.py +++ b/tests/test_version_strings.py @@ -82,7 +82,9 @@ def git_repo(temp_dir): _git("add -A") _git("commit -m 'message2'") - yield temp_dir + # Yielding here as tempdir is yielding and we want to stay in the tempdir + # context manager. Silencing ruff saying it should be return. + yield temp_dir # noqa: PT022 def test_version_data_git_and_toml(mocker, git_repo):