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

[coretl]

Closes #392

Implements the getter/setter half of #392 (ADR 0014's AttributeIO/AttributeIORef removal), scoped per the issue's own "Note on size": the DataType/*Meta TypedDict replacement and the precision/Limits naming pass (ADR 0017) are left for a follow-up PR rather than risking an incoherent halfway state in one session — see "Notes" below.

Scope

• Per-attribute IO is now getter/setter callables passed straight to AttrR(getter=...)/AttrW(setter=...)/AttrRW(getter=..., setter=...). Access mode is enforced structurally by which of getter/setter are present - there's no more io=/AttributeIO class hierarchy or AttributeIORef-keyed dispatch registry.
• Datatype is now optional on the constructors when it can be inferred from the getter's return annotation or the setter's value-parameter annotation (AttrR(getter=get_value) where get_value() -> float); not inferable ⇒ positional datatype still required, fails fast at construction.
• New Update[T] dataclass (value, timestamp) a getter/setter may return/accept instead of a bare value - update()/poll() unwrap it. (Native persistence of the timestamp, and a severity field, are left to AttrW setpoint cache, native timestamps + severity, ControllerRunner #395, which explicitly owns that.)
• Runtime surface rename (ADR 0014): get() → .readback/.setpoint read-only properties (AttrR has readback, AttrW has setpoint, AttrRW has both); no-arg update() → poll() (does the getter read, caches, returns the value); update(value) stays as a pure cache-push (now also accepting Update[T]); put(value[, sync_setpoint]) → set(value) (caches .setpoint, runs the setter; a non-None setter return updates .readback too - the sanctioned replacement for the old private _call_sync_setpoint_callbacks/sync_setpoint= mechanism).
• Removed: AttributeIO, AttributeIORef, ios=, _connect_attribute_ios, _validate_io, _attribute_ref_io_map, set_update_callback/bind_update_callback/set_on_put_callback/_call_sync_setpoint_callbacks/add_sync_setpoint_callback, and the second Attribute/AttrR/AttrW/AttrRW TypeVar (Attribute[DType_T, AttributeIORefT] → Attribute[DType_T]).
• Controller.create_api_and_tasks now schedules polling directly off getter-bearing attributes (poll_period, defaulting to ONCE when a getter is given) instead of pattern-matching an AttributeIORef.
• Migrated: the demo composition example (src/fastcs/demo/controllers.py), every docs/snippets/*.py tutorial snippet that used io_ref=, and all the tests that constructed attributes the old way (tests/assertable_controller.py, tests/test_attributes.py, tests/test_control_system.py, tests/conftest.py, tests/example_p4p_ioc.py, and the transport test files that built attributes directly).
• .datatype/DataType themselves are untouched in this PR (see Notes).

Notes

• Deliberately out of scope, left for a follow-up: the DataType family → python type + *Meta TypedDict replacement and the precision/nested-Limits naming pass (ADR 0017). The issue's own "Note on size" calls out this split explicitly. attr.datatype/Float/Int/etc. are unchanged.
• Verified locally with uv run --locked tox -e pre-commit,type-checking, both green in full. For the tests env, this sandbox can't run docs (needs outbound network to diamondlightsource.github.io) or the PVA/p4p-backed tests (RuntimeError: Address family not supported by protocol - no PVA-capable socket family here), the same known limitation noted on prior PRs (demo: use ControllerVector for temperature ramp sub-controllers #409/demo: cut-down Eiger REST sim + introspectable controller example #410/demo: convert temperature controller to getter/setter style #411) in this epic. Excluding those two paths, pytest src tests --ignore=tests/benchmarking passes 314/324, with only the same 10 pre-existing PVA (p4p)/socket-family failures, unrelated to this change. Real CI covers docs and PVA.
• Also spot-checked the demo end-to-end against the real tickit temperature-controller simulator (docs/snippets/*.py's test_docs_snippets.py already exercises this - all 16 snippets pass) and confirmed TemperatureController/TemperatureRampController construct and wire up correctly with the new getter/setter API.

---

Generated by Claude Code

[coderabbitai]

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: 923dddb0-88b4-402c-a218-6f5622523942

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-392

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

❌ Patch coverage is 93.11927% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.94%. Comparing base (326d31f) to head (9c1ad3f).

Files with missing lines	Patch %	Lines
src/fastcs/attributes/_infer_datatype.py	82.35%	6 Missing ⚠️
src/fastcs/attributes/attr_rw.py	85.18%	4 Missing ⚠️
src/fastcs/attributes/attr_r.py	95.45%	3 Missing ⚠️
src/fastcs/attributes/attr_w.py	95.34%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           refactor     #412      +/-   ##
============================================
- Coverage     91.25%   90.94%   -0.32%     
============================================
  Files            72       72              
  Lines          2892     2936      +44     
============================================
+ Hits           2639     2670      +31     
- Misses          253      266      +13     

☔ 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.

[shihab-dls]

I'm requesting changes with comments. Moreover, I've found that doing:

        self.a = AttrRW(Int(), getter=self.getter)

    async def getter(self) -> int:
        return 10

for example, results in the error TypeError: '<=' not supported between instances of 'object' and 'int', but I thought not providing a poll_period should default to ONCE. Explicitly adding poll_period does not raise the error.

Another thing; doing something like:

        self.a = AttrRW(Int(), getter=self.getter, poll_period=ONCE, setter=self.setter)

    async def getter(self) -> int:
        return 10

    async def setter(self, value: int):
        return value + 1

result in the IOC starting up with A_RBV=10 A=0, then setting A to 12 results in A_RBV=13 A=13, then setting A to 15 results in A_RBV=16 and A=15. This is because the seed support that was added seems to trigger on the first put on the attrRW, so the setpoint gets synced to the readback after the put, then subsequent puts dont affect the setpoint as the value is seeded. However, the required behaviour is that the first update (which should be a value of 10) will get seeded into the setpoint, but this is ignored.

[shihab-dls]

should: the always parameter determines if the callback is called even if the new value is equal to the current cached value. A description of this should be included in the docstring, as this is only inferrable from operations in update().

[coretl]

Done in ec2f44c — always now has an Args: entry saying it controls whether the callback fires on every update() rather than only when the value changes, and what it is for (a callback that timestamps or counts updates rather than displaying them).

---

Generated by Claude Code

[shihab-dls]

nit: I don't see why "the sanctioned replacement for the old private setpoint-echo mechanism" should be included in the docstring for set(). This seems like the sort of sentence you'd see in an ADR, not in a method docstring.

[coretl]

Agreed — dropped in ec2f44c. That sentence was justifying the design against what it replaced, which belongs in ADR 0014/0020 where it already is. The docstring now just states the behaviour: a returned value is the device's accepted/clamped value and is applied to the readback as well as the setpoint.

---

Generated by Claude Code

[shihab-dls]

Should: we've gotten rid of the IOs, but have not replaced them with anything. Add an attribute that uses a simple setter.

[coretl]

Good catch — this IOC lost all of its IO when AttributeIO went, so nothing in it exercised the replacement by hand any more.

Added in ec2f44c: ChildController.clamped, an AttrRW with a getter/setter pair over an in-memory value. The setter clamps to 0..100 and returns what it accepted:

async def get_clamped(self) -> int:
    return self._clamped

async def set_clamped(self, value: int) -> int:
    self._clamped = min(max(value, 0), 100)
    return self._clamped

I picked a clamping setter rather than a plain one so the IOC demonstrates both halves of ADR 0020 interactively: the getter seeds the setpoint at connect (Clamped and Clamped_RBV both read 0, not the datatype default arriving late), and the clamped return drives readback and setpoint together, so putting 150 leaves both at 100 rather than the setpoint sitting at 150. Locally:

after connect:  readback=0   setpoint=0
after set(150): readback=100 setpoint=100
after set(42):  readback=42  setpoint=42

test_ioc's child PVI assertion is updated for the new Child:N:Clamped PV. Note I could not run the p4p tests in my sandbox (RuntimeError: Address family not supported by protocol — no PVA-capable socket family), so that assertion update is verified by construction rather than by running it; CI is the real check. Say the word if you would rather have a plain non-clamping setter here.

---

Generated by Claude Code

[shihab-dls]

Should: we shouldn't be checking against only the exception type. Match this with the full or subset of the exception message.

[coretl]

Done in ec2f44c — now pytest.raises(RuntimeError, match="has no getter").

I also applied the same treatment to the four other bare pytest.raises in this file (the two set_name/set_path re-registration checks and the two wait_for_* timeouts), so the file is consistent rather than half-converted. Happy to narrow that back to just the two you flagged if you would rather keep the diff tight.

---

Generated by Claude Code

[shihab-dls]

Should: same here, we should match with the expected exception message or a subset of it.

[coretl]

Done in ec2f44c — now pytest.raises(ValueError, match="do_update failed"), which also pins that the getter's own exception is the one propagating rather than some other ValueError raised on the way through update()/validate().

---

Generated by Claude Code

[coretl]

Thanks @shihab-dls — the five inline comments are all addressed in ec2f44c (replies on each thread).

On the two runtime findings in your review body: I believe you were running a checkout from before the Polled rework, and both behave as you want on the current head. The giveaway is poll_period= — it stopped being a constructor argument in ab60286 (3 Aug), replaced by Polled/NotPolled, so on the head your second snippet raises before it gets anywhere:

TypeError: Attribute.__init__() got an unexpected keyword argument 'poll_period'

1. Bare getter defaulting to ONCE. No TypeError on the head — it constructs, reports ONCE, and schedules an initial poll:

self.a = AttrRW(Int(), getter=self.getter)   # bare getter IS the ONCE schedule

constructed ok, poll_period = inf
api built; initial coros: 1 periodic: 0
after initial poll: readback = 10 setpoint = 10

I could not reproduce a '<=' not supported between instances of 'object' and 'int' anywhere on the head — there is no object() sentinel left in this path (ONCE is float("inf")), and the only <= against a period is the @scan decorator's positive-period check, which attributes do not go through.

2. Setpoint seeding. Your exact scenario, spelled for the head (getter returns 10, setter returns value + 1):

before connect:      readback=0  setpoint=0
after initial poll:  readback=10 setpoint=10    (you saw A_RBV=10 A=0)
after set(12):       readback=13 setpoint=13    (you saw A_RBV=13 A=13)
after set(15):       readback=16 setpoint=16    (you saw A_RBV=16 A=15)

This is the behaviour you asked for: the first update seeds the setpoint, not the first put. Transport-side seeding is gone entirely — AttrRW.update mirrors readback into the setpoint while _setpoint_known is False, and transports just register add_setpoint_callback (ADR 0020). The ordering holds at the IOC too: serve() calls transport.connect() (which registers the callbacks) before _run_initial_coros(), so the seeded setpoint reaches the records rather than being posted before anything is listening.

The set(15) → A_RBV=16 A=15 inconsistency is gone for the same reason: a setter's bare return is the device's accepted value, so it drives both.

Since I am inferring the stale-checkout explanation rather than knowing it, could you re-pull and re-test at ec2f44c? If either still misbehaves for you there I would very much like the traceback, because it would mean I am wrong about the cause.

---

Generated by Claude Code

[shihab-dls]

Thanks @shihab-dls — the five inline comments are all addressed in ec2f44c (replies on each thread).

On the two runtime findings in your review body: I believe you were running a checkout from before the Polled rework, and both behave as you want on the current head. The giveaway is poll_period= — it stopped being a constructor argument in ab60286 (3 Aug), replaced by Polled/NotPolled, so on the head your second snippet raises before it gets anywhere:

TypeError: Attribute.__init__() got an unexpected keyword argument 'poll_period'

1. Bare getter defaulting to ONCE. No TypeError on the head — it constructs, reports ONCE, and schedules an initial poll:

self.a = AttrRW(Int(), getter=self.getter)   # bare getter IS the ONCE schedule

constructed ok, poll_period = inf
api built; initial coros: 1 periodic: 0
after initial poll: readback = 10 setpoint = 10

I could not reproduce a '<=' not supported between instances of 'object' and 'int' anywhere on the head — there is no object() sentinel left in this path (ONCE is float("inf")), and the only <= against a period is the @scan decorator's positive-period check, which attributes do not go through.

2. Setpoint seeding. Your exact scenario, spelled for the head (getter returns 10, setter returns value + 1):

before connect:      readback=0  setpoint=0
after initial poll:  readback=10 setpoint=10    (you saw A_RBV=10 A=0)
after set(12):       readback=13 setpoint=13    (you saw A_RBV=13 A=13)
after set(15):       readback=16 setpoint=16    (you saw A_RBV=16 A=15)

This is the behaviour you asked for: the first update seeds the setpoint, not the first put. Transport-side seeding is gone entirely — AttrRW.update mirrors readback into the setpoint while _setpoint_known is False, and transports just register add_setpoint_callback (ADR 0020). The ordering holds at the IOC too: serve() calls transport.connect() (which registers the callbacks) before _run_initial_coros(), so the seeded setpoint reaches the records rather than being posted before anything is listening.

The set(15) → A_RBV=16 A=15 inconsistency is gone for the same reason: a setter's bare return is the device's accepted value, so it drives both.

Since I am inferring the stale-checkout explanation rather than knowing it, could you re-pull and re-test at ec2f44c? If either still misbehaves for you there I would very much like the traceback, because it would mean I am wrong about the cause.

Generated by Claude Code

The first bug is fixed in the current head. However, the second bug is still present, but not for the reason I initially described. It seems that an AttrRW will sync it's setpoint with it's first readback value at startup, but this is not reflected on the PV. So, checking the actual setpoint value on the attribute will return the synced value, but doing a pvget/caget/etc on the setpoint PV will return 0. This is because, during the first update that is called via _run_inital_coros(), the PV is yet to be created, so the attributes setpoint_callbacks are empty. The callback to set the transport value is added to the attribute during transport.serve() which is in a coros list that is gathered after _run_initial_coros() is called, so the order results in the synced setpoint not being reflected on the transport.

[shihab-dls]

A few comments about tests, and I've also left a comment clarifying the sync setpoint bug

[shihab-dls]

Should: we shouldn't input the same string multiple times like this, instead we should make a variable with the expected message at the start of the test, then match against the variable.

[coretl]

Done in 9c1ad3f — the message is hoisted to expected_message at the top of the test and all three pytest.raises match against it.

---

Generated by Claude Code

[shihab-dls]

Should: this test only implies that the exception is caught and logged, but doesn't test that behaviour. One way to do this would be to also mock out the logger for this test, and check that it is called with the exception we expect.

[coretl]

Agreed — the test only proved the exception did not escape. Done in 9c1ad3f, mocking the logger as you suggested:

mock_logger = mocker.patch("fastcs.attributes.attr_w.logger")

await attr.set(5)
assert attr.setpoint == 5

logged_exception = mock_logger.opt.call_args.kwargs["exception"]
assert isinstance(logged_exception, ValueError)
assert str(logged_exception) == "do_set failed"
mock_logger.opt.return_value.error.assert_called_once_with(
    "Set failed", attribute=attr, setpoint=5
)

Asserting on logger.opt(exception=...) rather than just the message pins that it is the setter's ValueError being logged, not some other exception raised on the way through set().

---

Generated by Claude Code

[coretl]

You were right, and your second diagnosis was exactly right — thank you for chasing it down to the ordering. Fixed in 9c1ad3f.

To confirm your analysis: it is PVA-specific. EpicsCAIOC builds its records in __init__, which runs during transport.connect() — before _run_initial_coros() — so CA already had its setpoint callback registered in time. P4PIOC.__init__ only stored the APIs, and the PVs were built by parse_attributes inside P4PIOC.run(), i.e. inside transport.serve(), which is appended to coros and only awaited by the asyncio.gather after the initial polls have run. So make_shared_write_pv — and with it add_setpoint_callback — did not exist yet when the seed happened, and the seeded value went to an empty callback list. attribute.setpoint was correct; the PV served the datatype default.

This is why my earlier check did not catch it: I was asserting on attribute.setpoint, which is seeded correctly either way.

The fix is to make PVA follow the same contract CA already does — connect() builds the PVs, serve() runs the server:

def __init__(self, controller_apis: list[ControllerAPI]):
    self._controller_apis = controller_apis
    self._providers = [parse_attributes(api) for api in self._controller_apis]

async def run(self):
    endless_event = asyncio.Event()
    with Server(self._providers):
        await endless_event.wait()

parse_attributes had no await in it, so it becomes a plain function rather than an async def that never yielded.

Regression test — test_setpoint_seeded_by_initial_poll_reaches_transport in tests/transports/epics/pva/test_p4p.py. It wraps the attribute's setpoint callback registration to record what gets published, calls transport.connect(), then runs the initial coros without ever awaiting serve() — mirroring the ordering in FastCS.serve:

assert attribute.setpoint == 10
assert published == [10]

I checked it fails on the parent commit (published == [], i.e. your bug) and passes with the fix. It deliberately needs no p4p Server, so unlike the rest of test_p4p.py it runs in a sandbox without a PVA-capable socket family — and it would have caught this originally.

I also verified CA directly rather than assuming: capturing the out-record and running the initial poll gives 0 before the poll and 10 after, so record.set() before iocInit() does hold, and CA needs no change.

Verified with uv run --locked tox -e pre-commit,type-checking — both green. For tests, pytest src tests --ignore=tests/benchmarking gives 334 passed / 10 failed, and I confirmed the same 10 fail identically on the parent commit — all RuntimeError: Address family not supported by protocol, the sandbox's missing PVA socket family. Real CI is the check for those.

The two test comments from your latest review are addressed in the same commit, with replies on each thread.

---

Generated by Claude Code