demo: cut-down Eiger REST sim + introspectable controller example by coretl · Pull Request #410 · DiamondLightSource/fastcs

coretl · 2026-07-22T22:18:33Z

Closes #391

Adds the Example 5 tutorial pieces from #388 §9 / the demo README ladder:

src/fastcs/demo/simulation/eiger.py — a FastAPI fake REST sim shaped like a cut-down Eiger detector parameter tree: config/status subsystems, a keys listing endpoint per subsystem, and per-parameter GET/PUT. No new dependency (fastapi[standard] is already a core dep, httpx already a dev dep).
src/fastcs/demo/eiger.py — an EigerDetector controller. Half its attributes (count_time, state) are declared as type hints and checked by the current HintedAttribute introspection-validation mechanism; the rest of the parameter tree is discovered at initialise() time by walking the sim's keys endpoints and added dynamically with no static check — exercising the current (baseline) introspection mechanism ahead of ControllerFiller (ControllerFiller — declarative/procedural split #394).
tests/demo/test_eiger.py — unit tests against the fake sim (via httpx.ASGITransport, no real sockets/hardware): sim endpoint behaviour (list/get/put/read-only-rejection/404), hinted-vs-introspected attribute typing, and read/write round-trips through the controller.

Baseline uses the current API (AttrR/AttrRW + io_ref/AttributeIO); migrates to ControllerFiller when #394 lands, per the demo README.

Instructions to reviewer on how to test:

uv run pytest tests/demo/test_eiger.py -v

Checks for reviewer

Would the PR title make sense to a user on a set of release notes

Notes

Verified locally with uv run --locked tox -e pre-commit,type-checking, both green. For the tests env, this sandbox can't run docs (needs outbound network) or PVA-touching tests (needs a socket family unavailable here) — same known limitation noted on demo: use ControllerVector for temperature ramp sub-controllers #409. Excluding those, pytest src tests --ignore=tests/benchmarking passes 316/326, with the same 10 pre-existing PVA/p4p failures (RuntimeError: Address family not supported by protocol), none related to this change. Real CI covers docs and PVA.

Generated by Claude Code

Add a FastAPI fake REST sim (demo/simulation/eiger.py) shaped like a detector parameter tree (subsystems of named parameters, a keys listing endpoint, per-parameter GET/PUT), and an EigerDetector controller (demo/eiger.py) that type-hints half its attributes (checked via the current HintedAttribute mechanism) and fills the rest by introspecting the sim's keys endpoints in initialise(). Baseline uses the current API (AttrR/AttrRW + io_ref/AttributeIO); migrates to ControllerFiller when #394 lands. Closes #391

coderabbitai · 2026-07-22T22:18:40Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a3744dc2-27f2-41c8-9623-5cce52bb0084

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor-issue-391

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-07-22T22:20:30Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.25%. Comparing base (3da025f) to head (da9988c).
⚠️ Report is 2 commits behind head on refactor.

Additional details and impacted files

@@            Coverage Diff            @@
##           refactor     #410   +/-   ##
=========================================
  Coverage     91.25%   91.25%           
=========================================
  Files            72       72           
  Lines          2892     2892           
=========================================
  Hits           2639     2639           
  Misses          253      253

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sphinx-build --fail-on-warning was erroring on autodoc cross-references to httpx.AsyncBaseTransport and fastapi.applications.FastAPI, which have no intersphinx mapping - same class of issue already worked around for p4p types.

coretl · 2026-07-22T22:21:31Z

Fixed the docs / build failure: sphinx-build --fail-on-warning was erroring on unresolvable autodoc cross-references to httpx.AsyncBaseTransport and fastapi.applications.FastAPI (no intersphinx mapping for either). Added both to nitpick_ignore in docs/conf.py, following the existing p4p precedent.

Generated by Claude Code

coretl · 2026-07-23T13:10:53Z

+    # Declared (checked): must exist, with this access mode and dtype, after
+    # initialise() introspects the parameter tree.
+    count_time: AttrRW[float]
+    state: AttrR[str]


The reason we put parameters on here is because we want to build code on top of them. Add an idle: AttrR[bool] attribute with value self.state == "idle".

Done in ff6292b. Added a soft idle = AttrR(Bool()) derived from state, kept in sync via an add_on_update_callback on self.state (idle = state == "idle"). Added a test covering it.

We do not currently handle enum creation properly in fastcs-eiger, but I think we should assume that we do for the purposes of this demo. Therefore, for state, the actual response from the detector is: {'access_mode': 'r', 'allowed_values': ['na', 'idle', 'ready', 'acquire', 'configure', 'initialize', 'error'], 'value': 'idle', 'value_type': 'string'}, where we should use the allowed_values metadata to introspect an AttrR[enum.Enum] with the correct options. Can we amend the entry in the sim response, and accept allowed_values as a parameter on EigerParameter, then use this parameter in forming the client response from the server, such that we can correctly make an AttrR[enum.Enum] for state. I think we'll only need [idle, ready, acquire] for now. I'll un-resolve this thread to make my review comment visible.

…l read-only params - eiger.py: add soft `idle: AttrR[bool]` derived from the introspected `state` param (state == "idle"), kept in sync via an on-update callback - shows why we declare `state` as a checked attribute (to build code on top of it). - eiger.py: give read-only params a poll `update_period` in `initialise()`; rw params still read once (ONCE). - simulation/eiger.py: add a lifespan background task that sweeps `temperature` between two values so the front end shows something updating (real server only; the in-process ASGI transport used in tests stays deterministic). - tests: cover idle-from-state, read-only poll vs rw read-once, and oscillation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ests - simulation/eiger.py: replace the sine sweep with a simple flip between two known temperatures every 0.5s (predictable); expose the parameter tree via `app.state.sim` as a test backdoor for read-only params with no PUT route. - tests: drop the sim-only tests; drive everything through the controller attributes. Idle test now pokes `state` via the sim backdoor and polls the attribute (rather than calling AttrR.update directly). Oscillation test builds a controller under the app lifespan and observes temperature via subscribe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

shihab-dls

This looks really useful, I've left a few comments I want addressed before appoving this, so I'll request changes.

shihab-dls · 2026-07-27T09:19:16Z

+
+    async def update(self, attr: AttrR[Any, EigerAttributeIORef]) -> None:
+        data = await self._connection.get(attr.io_ref.subsystem, attr.io_ref.param)
+        await attr.update(attr.dtype(data["value"]))


Here we're casting the value we recieve to the correct datatype before calling update. However, attr.update internally calls datatype.validate() which will try to cast the value anyway, and raise with a msg if it fails. We should not cast here in our update method, as the exception should be handled in one common place (i.e., validate()).

shihab-dls · 2026-07-27T09:54:31Z

+    # Declared (checked): must exist, with this access mode and dtype, after
+    # initialise() introspects the parameter tree.
+    count_time: AttrRW[float]
+    state: AttrR[str]


We do not currently handle enum creation properly in fastcs-eiger, but I think we should assume that we do for the purposes of this demo. Therefore, for state, the actual response from the detector is: {'access_mode': 'r', 'allowed_values': ['na', 'idle', 'ready', 'acquire', 'configure', 'initialize', 'error'], 'value': 'idle', 'value_type': 'string'}, where we should use the allowed_values metadata to introspect an AttrR[enum.Enum] with the correct options. Can we amend the entry in the sim response, and accept allowed_values as a parameter on EigerParameter, then use this parameter in forming the client response from the server, such that we can correctly make an AttrR[enum.Enum] for state. I think we'll only need [idle, ready, acquire] for now. I'll un-resolve this thread to make my review comment visible.

shihab-dls · 2026-07-27T11:17:50Z

+@pytest_asyncio.fixture
+async def sim(_eiger) -> SimState:
+    return _eiger[1]


nit: usually I'd nitpick about putting the fixtures grouped at the top of the page, but in this case there is a very clear separation of concerns between the sim tests and detector tests, so I don't mind it as much.

coretl mentioned this pull request Jul 22, 2026

Example 5 — cut-down Eiger: introspectable controller + REST sim #391

Open

This was referenced Jul 22, 2026

demo: getter/setter-in-init temperature attr example #411

Open

getter/setter IO rework; remove AttributeIORef and DataType #392

Open

coretl commented Jul 23, 2026

View reviewed changes

Comment thread src/fastcs/demo/simulation/eiger.py

coretl commented Jul 23, 2026

View reviewed changes

Comment thread src/fastcs/demo/eiger.py

coretl commented Jul 23, 2026

View reviewed changes

Comment thread tests/demo/test_eiger.py Outdated

Comment thread tests/demo/test_eiger.py Outdated

Comment thread tests/demo/test_eiger.py Outdated

Comment thread src/fastcs/demo/simulation/eiger.py Outdated

Comment thread src/fastcs/demo/simulation/eiger.py

coretl requested a review from shihab-dls July 23, 2026 15:17

coretl mentioned this pull request Jul 23, 2026

attributes: getter/setter IO rework, remove AttributeIORef/AttributeIO #412

Open

shihab-dls requested changes Jul 27, 2026

View reviewed changes

Uh oh!

Conversation

coretl commented Jul 22, 2026

Instructions to reviewer on how to test:

Checks for reviewer

Notes

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coretl commented Jul 22, 2026

Uh oh!

coretl Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

coretl Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

shihab-dls Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shihab-dls left a comment

Choose a reason for hiding this comment

Uh oh!

shihab-dls Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

shihab-dls Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

shihab-dls Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

codecov Bot commented Jul 22, 2026 •

edited

Loading