Raise Exceptions on Hardware I/O to Set Record Alarms#389
Conversation
📝 WalkthroughWalkthroughAttribute write failures now propagate, and EPICS CA/PVA write and command handlers report failures through operation errors and major alarm states. Tests cover callback propagation, SoftIOC command alarms, and PVA put failures. ChangesEPICS error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)EPICS CA failure flowsequenceDiagram
participant Client
participant EPICS_CA
participant AttrW
participant EPICS_Record
Client->>EPICS_CA: write PV or command PV
EPICS_CA->>AttrW: put attribute or run method
AttrW-->>EPICS_CA: exception
EPICS_CA->>EPICS_Record: set current value and MAJOR_ALARM
EPICS PVA failure flowsequenceDiagram
participant Client
participant WritePvHandler
participant AttrW
participant PVA
Client->>WritePvHandler: put value
WritePvHandler->>AttrW: await put
AttrW-->>WritePvHandler: exception
WritePvHandler->>PVA: post major alarm
WritePvHandler-->>Client: op.done(error)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
25e95c9 to
b0466d2
Compare
shihab-dls
changed the title
b0466d2 to
b2b5e9b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 91.25% 91.29% +0.04%
==========================================
Files 72 72
Lines 2892 2919 +27
==========================================
+ Hits 2639 2665 +26
- Misses 253 254 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e2fb84c to
48791cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/transports/epics/pva/test_p4p.py (1)
215-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using an
async deftest function.Since other tests in this file are async (e.g.,
test_command_method), you can define this test asasync defto avoid manually interacting with the event loop. This is more idiomatic and prevents potentialRuntimeErrorissues withget_event_loop()in Python 3.12+.♻️ Proposed refactor to make the test natively async
-def test_alarms_set_if_put_fails(caplog): +async def test_alarms_set_if_put_fails(caplog): `@dataclass` class SimpleAttributeIORef(AttributeIORef): pass class SimpleAttributeIO(AttributeIO[DType_T, SimpleAttributeIORef]): async def send(self, attr: AttrW[DType_T, SimpleAttributeIORef], value): raise ValueError("Failed") class SomeController(Controller): a = AttrW(Int(), io_ref=SimpleAttributeIORef()) controller = SomeController(ios=[SimpleAttributeIO()]) pv_prefix = str(uuid4()) fastcs = make_fastcs(pv_prefix, controller) async def put_pvs(): await asyncio.sleep(0.1) ctxt = Context("pva") await ctxt.put(f"{pv_prefix}:A", 1) - serve = asyncio.ensure_future(fastcs.serve(interactive=False)) - try: - asyncio.get_event_loop().run_until_complete( - asyncio.wait_for(asyncio.gather(serve, put_pvs()), timeout=1) - ) - except RuntimeError as exc: - assert str(exc) == "Exception raised during put operation: ValueError('Failed')" - serve.cancel() + serve = asyncio.create_task(fastcs.serve(interactive=False)) + try: + await asyncio.wait_for(asyncio.gather(serve, put_pvs()), timeout=1) + except RuntimeError as exc: + assert str(exc) == "Exception raised during put operation: ValueError('Failed')" + finally: + serve.cancel()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transports/epics/pva/test_p4p.py` around lines 215 - 245, Convert test_alarms_set_if_put_fails to an async test and await the concurrent serve and put_pvs tasks directly, following the pattern used by test_command_method. Remove the manual get_event_loop().run_until_complete and wait_for usage while preserving the existing timeout, exception assertion, and serve cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 37-40: Update the epicsca dependency list in pyproject.toml to
replace the softioc Git master reference with a released softioc version
specifier, preserving the existing pvi dependency and ensuring installs use the
stable published package.
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 231-239: Update both write PV and command PV callbacks in
src/fastcs/transports/epics/ca/ioc.py at lines 231-239 and 291-299: bind caught
exceptions explicitly, log each with the existing logger and relevant PV context
before setting the MAJOR_ALARM record state, and avoid blind exception catches.
---
Nitpick comments:
In `@tests/transports/epics/pva/test_p4p.py`:
- Around line 215-245: Convert test_alarms_set_if_put_fails to an async test and
await the concurrent serve and put_pvs tasks directly, following the pattern
used by test_command_method. Remove the manual
get_event_loop().run_until_complete and wait_for usage while preserving the
existing timeout, exception assertion, and serve cancellation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3502c74e-caf1-455e-af90-0199f1affa9b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlsrc/fastcs/attributes/attr_w.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pytests/example_softioc.pytests/test_attributes.pytests/transports/epics/ca/test_softioc_system.pytests/transports/epics/pva/test_p4p.py
| epicsca = [ | ||
| "pvi~=0.12.0", | ||
| "softioc @ git+https://github.com/DiamondLightSource/pythonSoftIOC.git@master", | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' pyproject.tomlRepository: DiamondLightSource/fastcs
Length of output: 3639
🏁 Script executed:
rg -n "epicsca|softioc|pythonSoftIOC|master" .Repository: DiamondLightSource/fastcs
Length of output: 5315
🌐 Web query:
pythonSoftIOC PyPI release softioc stable version
💡 Result:
As of July 15, 2026, the latest stable release of the pythonSoftIOC library (available on PyPI as softioc) is version 4.7.0, which was released on January 14, 2026 [1][2]. pythonSoftIOC is a library that allows an EPICS (Experimental Physics and Industrial Control System) IOC (Input/Output Controller) with Python device support to be run directly from within a Python interpreter [3][4]. It enables the programmatic creation of EPICS records and allows arbitrary Python code to update those records and respond to channel access requests [3][5]. Key details include: - PyPI Project Name: softioc [3] - Latest Stable Version: 4.7.0 [1][2] - Primary Repository: DiamondLightSource/pythonSoftIOC (maintained by Diamond Light Source) [6][5] - Documentation: Available on the official GitHub Pages site [7][8] You can install the library using pip: pip install softioc For further details regarding changes in specific versions, you can consult the project's official changelog [1][2].
Citations:
- 1: https://github.com/DiamondLightSource/pythonSoftIOC/blob/master/CHANGELOG.rst
- 2: https://diamondlightsource.github.io/pythonSoftIOC/master/reference/changelog.html
- 3: https://pypi.org/project/softioc/
- 4: https://pypi.org/project/softioc/4.6.1/
- 5: https://github.com/dls-controls/pythonSoftIOC
- 6: https://github.com/DiamondLightSource/pythonSoftIOC/releases
- 7: https://diamondlightsource.github.io/pythonSoftIOC/master/index.html
- 8: https://diamondlightsource.github.io/pythonSoftIOC/master/tutorials/creating-an-ioc.html
Pin softioc to a released version before merging. fastcs[epicsca] currently depends on softioc @ git+https://github.com/DiamondLightSource/pythonSoftIOC.git@master, which makes installs non-deterministic for anyone using the extra. softioc has a stable release available, so this should use a version specifier instead of a branch ref.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 37 - 40, Update the epicsca dependency list in
pyproject.toml to replace the softioc Git master reference with a released
softioc version specifier, preserving the existing pvi dependency and ensuring
installs use the stable published package.
| try: | ||
| await attribute.put(cast_from_epics_type(attribute.datatype, value)) | ||
| except Exception: | ||
| record.set( | ||
| record.get(), | ||
| process=False, | ||
| severity=alarm.MAJOR_ALARM, | ||
| alarm=alarm.MAJOR_ALARM, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log caught exceptions to prevent silent failures.
Both the write PV and command PV update callbacks catch Exception to set alarm states but do not log the exception. This causes the error details to be completely swallowed, making it difficult to debug why an EPICS major alarm was triggered.
src/fastcs/transports/epics/ca/ioc.py#L231-L239: catch the exception explicitly (except Exception as e:) and log it before setting the record state (e.g.,logger.opt(exception=e).error("Exception during PV put", pv=pv)).src/fastcs/transports/epics/ca/ioc.py#L291-L299: catch the exception explicitly (except Exception as e:) and log it before setting the record state (e.g.,logger.opt(exception=e).error("Exception during command PV put", pv=pv)).
As per static analysis hints, do not catch a blind exception.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 233-233: Do not catch blind exception: Exception
(BLE001)
📍 Affects 1 file
src/fastcs/transports/epics/ca/ioc.py#L231-L239(this comment)src/fastcs/transports/epics/ca/ioc.py#L291-L299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fastcs/transports/epics/ca/ioc.py` around lines 231 - 239, Update both
write PV and command PV callbacks in src/fastcs/transports/epics/ca/ioc.py at
lines 231-239 and 291-299: bind caught exceptions explicitly, log each with the
existing logger and relevant PV context before setting the MAJOR_ALARM record
state, and avoid blind exception catches.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastcs/transports/epics/pva/_pv_handlers.py (1)
51-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not clear alarms before the hardware put succeeds.
The new default
p4p_alarm_states()is posted beforeawait self._attr_w.put(cast_value), so an existing major alarm is temporarily cleared while I/O is still pending. A stalled or cancelled operation can leave the PV falsely healthy; post the value/reset alarm only in the success path, or preserve the current alarm until the operation completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fastcs/transports/epics/pva/_pv_handlers.py` around lines 51 - 64, Move the normal value/alarm post in the handler around the put operation so it occurs only after await self._attr_w.put(cast_value) succeeds. Preserve the existing alarm state while the put is pending or fails, and keep the exception path’s error alarm posting and op.done(error=...) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 234-235: Update the write PV on_update callback at
src/fastcs/transports/epics/ca/ioc.py lines 234-235 to bind the caught exception
and log its details before setting the MAJOR alarm. Apply the same explicit
exception binding and logging in the command PV wrapped_method callback at lines
290-291, preserving the existing alarm behavior.
---
Outside diff comments:
In `@src/fastcs/transports/epics/pva/_pv_handlers.py`:
- Around line 51-64: Move the normal value/alarm post in the handler around the
put operation so it occurs only after await self._attr_w.put(cast_value)
succeeds. Preserve the existing alarm state while the put is pending or fails,
and keep the exception path’s error alarm posting and op.done(error=...)
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f2f9465-f2a4-4206-a3bb-21b55a1f826e
📒 Files selected for processing (4)
src/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pysrc/fastcs/transports/epics/pva/pvi.pysrc/fastcs/transports/epics/pva/types.py
| except Exception: | ||
| _set_alarm(record, alarm.MAJOR_ALARM) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log caught exceptions to prevent silent failures.
Both the write PV and command PV update callbacks catch Exception to set alarm states but do not log the exception. This causes the error details to be completely swallowed, making it difficult to debug why an EPICS major alarm was triggered.
src/fastcs/transports/epics/ca/ioc.py#L234-L235: catch the exception explicitly (except Exception as e:) and log it in theon_updatecallback.src/fastcs/transports/epics/ca/ioc.py#L290-L291: catch the exception explicitly (except Exception as e:) and log it in thewrapped_methodcallback.
🐛 Proposed fixes
For on_update (Write PV):
- except Exception:
+ except Exception as e:
+ logger.opt(exception=e).error("Exception during PV put", pv=pv)
_set_alarm(record, alarm.MAJOR_ALARM)For wrapped_method (Command PV):
- except Exception:
+ except Exception as e:
+ logger.opt(exception=e).error("Exception during command PV put", pv=pv)
_set_alarm(record, alarm.MAJOR_ALARM)🧰 Tools
🪛 Ruff (0.15.21)
[warning] 234-234: Do not catch blind exception: Exception
(BLE001)
📍 Affects 1 file
src/fastcs/transports/epics/ca/ioc.py#L234-L235(this comment)src/fastcs/transports/epics/ca/ioc.py#L290-L291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fastcs/transports/epics/ca/ioc.py` around lines 234 - 235, Update the
write PV on_update callback at src/fastcs/transports/epics/ca/ioc.py lines
234-235 to bind the caught exception and log its details before setting the
MAJOR alarm. Apply the same explicit exception binding and logging in the
command PV wrapped_method callback at lines 290-291, preserving the existing
alarm behavior.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/transports/epics/pva/_pv_handlers.py`:
- Around line 192-199: Update _set_alarm to avoid mutating alarm_states["alarm"]
in place by copying the nested alarm dictionary. Only remove "status" from that
copy for Table or Waveform datatypes, then call pv.post with one positional,
properly nested dictionary containing the value and alarm structure; preserve
the existing alarm_states structure for other datatypes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e97e013-1f54-4c51-b55d-88e09ab22a43
📒 Files selected for processing (1)
src/fastcs/transports/epics/pva/_pv_handlers.py
1 participant
Closes #303
This depends on a new release of
softioc.This PR sets and resets alarm states on failed and successful PV puts for CA and PVA.
This addresses the underlying bug in ophyd-async, where a failed command does not raise an error. However, only PVA allows these errors to immediately propagate up to the signal call, otherwise requiring a check on the PV status after the put on CA. Therefore, fastcs-eiger/odin must be converted to use PVA to close the ophyd-async bug.
Summary by CodeRabbit
Bug Fixes
Tests