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: 5d5e57b5-6859-4c5b-9e37-a0c41eb54bc7

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 (bff5249).

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.

[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

[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

[shihab-dls]

This looks good now. I've pushed changes to the CA and PVA system tests to check seeding the setpoint, but also left the unit test that checks this. Approving.