Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to use new attribute API and pyright #57

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .copier-answers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ github_org: DiamondLightSource
package_name: fastcs_eiger
pypi: true
repo_name: fastcs-eiger
type_checker: mypy
type_checker: pyright
13 changes: 7 additions & 6 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ classifiers = [
description = "Eiger control system integration with FastCS"
dependencies = [
"aiohttp",
"fastcs~=0.7.0",
"fastcs~=0.8.0",
"numpy",
"pillow",
"typer",
Expand All @@ -27,11 +27,11 @@ dev = [
"tickit-devices @ git+https://github.com/dls-controls/tickit-devices.git@eiger-stream2",
"black",
"copier",
"mypy",
"myst-parser",
"pipdeptree",
"pre-commit",
"pydata-sphinx-theme>=0.12",
"pyright",
"pytest",
"pytest-asyncio",
"pytest-cov",
Expand All @@ -57,8 +57,9 @@ name = "Gary Yendell"
[tool.setuptools_scm]
version_file = "src/fastcs_eiger/_version.py"

[tool.mypy]
ignore_missing_imports = true # Ignore missing stubs in imported modules
[tool.pyright]
typeCheckingMode = "standard"
reportMissingImports = false # Ignore missing stubs in imported modules

[tool.pytest.ini_options]
# Run pytest with all our checkers, and don't spam us with massive tracebacks on error
Expand Down Expand Up @@ -91,12 +92,12 @@ passenv = *
allowlist_externals =
pytest
pre-commit
mypy
pyright
sphinx-build
sphinx-autobuild
commands =
pre-commit: pre-commit run --all-files --show-diff-on-failure {posargs}
type-checking: mypy src tests {posargs}
type-checking: pyright src tests {posargs}
tests: pytest --cov=fastcs_eiger --cov-report term --cov-report xml:cov.xml {posargs}
docs: sphinx-{posargs:build -EW --keep-going} -T docs build/html
"""
Expand Down
30 changes: 15 additions & 15 deletions src/fastcs_eiger/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
from typing import Optional

import typer
from fastcs.backends.asyncio_backend import AsyncioBackend
from fastcs.backends.epics.backend import EpicsBackend
from fastcs.backends.epics.gui import EpicsGUIOptions
from fastcs.launch import FastCS
from fastcs.transport.epics.options import (
EpicsGUIOptions,
EpicsIOCOptions,
EpicsOptions,
)

from fastcs_eiger import __version__
from fastcs_eiger.eiger_controller import EigerController
Expand Down Expand Up @@ -52,19 +55,16 @@

controller = EigerController(ip, port)

backend = EpicsBackend(controller, pv_prefix)
backend.create_gui(
EpicsGUIOptions(output_path=ui_path / "eiger.bob", title=f"Eiger - {pv_prefix}")
options = EpicsOptions(

Check warning on line 58 in src/fastcs_eiger/__main__.py

View check run for this annotation

Codecov / codecov/patch

src/fastcs_eiger/__main__.py#L58

Added line #L58 was not covered by tests
ioc=EpicsIOCOptions(pv_prefix=pv_prefix),
gui=EpicsGUIOptions(
output_path=ui_path / "eiger.bob", title=f"Eiger - {pv_prefix}"
),
)
backend.run()


@app.command()
def asyncio(ip: str = EigerIp, port: int = EigerPort):
controller = EigerController(ip, port)

backend = AsyncioBackend(controller)
backend.run_interactive_session()
launcher = FastCS(controller, options)
launcher.create_docs()
launcher.create_gui()
launcher.run()

Check warning on line 67 in src/fastcs_eiger/__main__.py

View check run for this annotation

Codecov / codecov/patch

src/fastcs_eiger/__main__.py#L64-L67

Added lines #L64 - L67 were not covered by tests


# test with: python -m fastcs_eiger
Expand Down
47 changes: 30 additions & 17 deletions src/fastcs_eiger/eiger_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
from typing import Any, Literal

import numpy as np
from fastcs.attributes import Attribute, AttrR, AttrRW, AttrW
from fastcs.controller import Controller, SubController
from fastcs.attributes import Attribute, AttrR, AttrRW, AttrW, Handler
from fastcs.controller import BaseController, Controller, SubController
from fastcs.datatypes import Bool, Float, Int, String
from fastcs.wrappers import command, scan
from PIL import Image
Expand Down Expand Up @@ -62,10 +62,10 @@
"""

uri: str
update_period: float = 0.2
update_period: float | None = 0.2

async def put(
self, controller: "EigerSubsystemController", _: AttrW, value: Any
self, controller: "EigerSubsystemController", attr: AttrW, value: Any
) -> None:
parameters_to_update = await controller.connection.put(self.uri, value)
if not parameters_to_update:
Expand All @@ -86,10 +86,15 @@

await controller.queue_update(parameters_to_update)

async def update(self, controller: "EigerController", attr: AttrR) -> None:
async def update(self, controller: "EigerSubsystemController", attr: AttrR) -> None:
try:
response = await controller.connection.get(self.uri)
await attr.set(response["value"])
value = response["value"]
if isinstance(value, list) and all(
isinstance(s, str) for s in value
): # error is a list of strings
value = ", ".join(value)

Check warning on line 96 in src/fastcs_eiger/eiger_controller.py

View check run for this annotation

Codecov / codecov/patch

src/fastcs_eiger/eiger_controller.py#L96

Added line #L96 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting it like this might make it look like two messages is actually one, so it is probably clearer to just do str(value) Or maybe use a different separator, like |?

I do wonder if this should be implemented in FastCS with String().validate(value).

Copy link
Collaborator Author

@jsouter jsouter Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that in a previous release lists did automatically get cast to strings, though I may be misremembering and that was just in a dev branch I never pushed anywhere.

await attr.set(value)
except Exception as e:
print(f"Failed to get {self.uri}:\n{e.__class__.__name__} {e}")

Expand All @@ -116,15 +121,16 @@


@dataclass
class LogicHandler:
class LogicHandler(Handler):
"""
Handler for FastCS Attribute Creation

Dataclass that is called using the AttrR, AttrRW function.
Used for dynamically created attributes that are added for additional logic
"""

async def put(self, _: "EigerController", attr: AttrW, value: Any) -> None:
async def put(self, controller: BaseController, attr: AttrW, value: Any) -> None:
assert isinstance(attr, AttrR) # AttrW does not implement set

Check warning on line 133 in src/fastcs_eiger/eiger_controller.py

View check run for this annotation

Codecov / codecov/patch

src/fastcs_eiger/eiger_controller.py#L133

Added line #L133 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary if attr is AttrW?

Copy link
Collaborator Author

@jsouter jsouter Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttrW doesn't have set so pyright complains if we're trying to call set if we don't verify it's an AttrR (or I guess AttrRW).

await attr.set(value)


Expand Down Expand Up @@ -227,13 +233,21 @@
print("\nAn HTTP request failed while introspecting detector:\n")
raise

def get_subsystem_controllers(self) -> list["EigerSubsystemController"]:
return [
controller
for controller in self.get_sub_controllers().values()
if isinstance(controller, EigerSubsystemController)
]

@scan(0.1)
async def update(self):
"""Periodically check for parameters that need updating from the detector."""
subsystem_controllers = self.get_subsystem_controllers()
await self.stale_parameters.set(
any(c.stale_parameters.get() for c in self.get_sub_controllers().values())
any(c.stale_parameters.get() for c in subsystem_controllers)
)
controller_updates = [c.update() for c in self.get_sub_controllers().values()]
controller_updates = [c.update() for c in subsystem_controllers]
await asyncio.gather(*controller_updates)


Expand Down Expand Up @@ -283,7 +297,7 @@
attributes = self._create_attributes(parameters)

for name, attribute in attributes.items():
setattr(self, name, attribute)
self.attributes[name] = attribute

@classmethod
def _group(cls, parameter: EigerParameter):
Expand Down Expand Up @@ -314,18 +328,19 @@
datatype = String()
case _:
print(f"Failed to handle {parameter}")
continue

group = cls._group(parameter)
match parameter.response["access_mode"]:
case "r":
attributes[parameter.attribute_name] = AttrR(
datatype,
datatype, # type: ignore
handler=EIGER_HANDLERS[parameter.mode](parameter.uri),
group=group,
)
case "rw":
attributes[parameter.attribute_name] = AttrRW(
datatype,
datatype, # type: ignore
handler=EIGER_HANDLERS[parameter.mode](parameter.uri),
group=group,
allowed_values=parameter.response.get("allowed_values", None),
Expand Down Expand Up @@ -362,10 +377,8 @@
if key in IGNORED_KEYS:
continue
attr_name = _key_to_attribute_name(key)
match getattr(self, attr_name, None):
# TODO: mypy doesn't understand AttrR as a type for some reason:
# `error: Expected type in class pattern; found "Any" [misc]`
case AttrR(updater=EigerConfigHandler() as updater) as attr: # type: ignore [misc]
match self.attributes.get(attr_name, None):
case AttrR(updater=EigerConfigHandler() as updater) as attr:
jsouter marked this conversation as resolved.
Show resolved Hide resolved
parameter_updates.append(updater.config_update(self, attr))
case _ as attr:
print(f"Failed to handle update for {key}: {attr}")
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def sim_eiger_controller(request):
)

# Wait until ready
while True:
while proc.stdout is not None:
line = proc.stdout.readline()
if "Starting HTTP server..." in line:
break
Expand Down
6 changes: 3 additions & 3 deletions tests/system/parameters.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"Detector": {
"detector": {
"humidity": {
"subsystem": "detector",
"mode": "status",
Expand Down Expand Up @@ -770,7 +770,7 @@
}
}
},
"Stream": {
"stream": {
"dropped": {
"subsystem": "stream",
"mode": "status",
Expand Down Expand Up @@ -863,7 +863,7 @@
}
}
},
"Monitor": {
"monitor": {
"buffer_free": {
"subsystem": "monitor",
"mode": "status",
Expand Down
29 changes: 16 additions & 13 deletions tests/system/test_introspection.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from typing import Any

import pytest
from fastcs.attributes import Attribute, AttrR
from fastcs.attributes import Attribute, AttrR, AttrRW
from fastcs.datatypes import Float

from fastcs_eiger.eiger_controller import (
Expand All @@ -15,6 +15,7 @@
EigerMonitorController,
EigerParameter,
EigerStreamController,
EigerSubsystemController,
)

HERE = Path(__file__).parent
Expand Down Expand Up @@ -42,14 +43,14 @@ async def test_attribute_creation(sim_eiger_controller: EigerController):
await controller.initialise()
serialised_parameters: dict[str, dict[str, Any]] = {}
subsystem_parameters = {}
for subsystem_name, subcontroller in controller.get_sub_controllers().items():
serialised_parameters[subsystem_name] = {}
for subcontroller in controller.get_subsystem_controllers():
serialised_parameters[subcontroller._subsystem] = {}
subsystem_parameters[
subsystem_name
subcontroller._subsystem
] = await subcontroller._introspect_detector_subsystem()
for param in subsystem_parameters[subsystem_name]:
serialised_parameters[subsystem_name][param.key] = _serialise_parameter(
param
for param in subsystem_parameters[subcontroller._subsystem]:
serialised_parameters[subcontroller._subsystem][param.key] = (
_serialise_parameter(param)
)

expected_file = HERE / "parameters.json"
Expand All @@ -61,15 +62,15 @@ async def test_attribute_creation(sim_eiger_controller: EigerController):
assert serialised_parameters == expected_parameters, "Detector API does not match"

detector_attributes = EigerDetectorController._create_attributes(
subsystem_parameters["Detector"]
subsystem_parameters["detector"]
GDYendell marked this conversation as resolved.
Show resolved Hide resolved
)
assert len(detector_attributes) == 76
monitor_attributes = EigerMonitorController._create_attributes(
subsystem_parameters["Monitor"]
subsystem_parameters["monitor"]
)
assert len(monitor_attributes) == 7
stream_attributes = EigerStreamController._create_attributes(
subsystem_parameters["Stream"]
subsystem_parameters["stream"]
)
assert len(stream_attributes) == 8

Expand All @@ -95,6 +96,7 @@ async def test_controller_groups_and_parameters(sim_eiger_controller: EigerContr

for subsystem in MISSING_KEYS:
subcontroller = controller.get_sub_controllers()[subsystem.title()]
assert isinstance(subcontroller, EigerSubsystemController)
parameters = await subcontroller._introspect_detector_subsystem()
if subsystem == "detector":
# ignored keys should not get added to the controller
Expand All @@ -107,9 +109,9 @@ async def test_controller_groups_and_parameters(sim_eiger_controller: EigerContr
if attr_name == "threshold_energy":
continue
assert attr.group and "Threshold" in attr.group

attr = subcontroller.threshold_1_energy
attr: AttrRW = subcontroller.attributes["threshold_1_energy"] # type: ignore
sender = attr.sender
assert sender is not None
await sender.put(subcontroller, attr, 100.0)
# set parameters to update based on response to put request
assert subcontroller._parameter_updates == {
Expand All @@ -123,8 +125,9 @@ async def test_controller_groups_and_parameters(sim_eiger_controller: EigerContr
subcontroller._parameter_updates.clear()

# make sure API inconsistency for threshold/difference/mode is addressed
attr = subcontroller.threshold_difference_mode
attr: AttrRW = subcontroller.attributes["threshold_difference_mode"] # type: ignore
GDYendell marked this conversation as resolved.
Show resolved Hide resolved
sender = attr.sender
assert sender is not None
await sender.put(subcontroller, attr, "enabled")
assert subcontroller._parameter_updates == {"threshold/difference/mode"}

Expand Down
8 changes: 6 additions & 2 deletions tests/test_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import pytest
from pytest_mock import MockerFixture

from fastcs_eiger.eiger_controller import EigerController, EigerHandler
from fastcs_eiger.eiger_controller import (
EigerController,
EigerDetectorController,
EigerHandler,
)

_lock = asyncio.Lock()

Expand Down Expand Up @@ -71,7 +75,7 @@ async def test_stale_parameter_propagates_to_top_controller(mocker: MockerFixtur
await eiger_controller.initialise()

detector_controller = eiger_controller.get_sub_controllers()["Detector"]

assert isinstance(detector_controller, EigerDetectorController)
# queueing update sets subcontroller to stale
assert detector_controller.stale_parameters.get() is False
await detector_controller.queue_update(["dummy_attribute"])
Expand Down
Loading