Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #319 +/- ##
==========================================
- Coverage 90.84% 90.70% -0.15%
==========================================
Files 70 70
Lines 2555 2570 +15
==========================================
+ Hits 2321 2331 +10
- Misses 234 239 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
Looks good. Minor comments.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplit the previous unified record constructor into two functions: Changes
Sequence Diagram(s)sequenceDiagram
participant Attr as Attribute
participant IOC as EPICS IOC / builder
participant Rec as RecordWrapper
Attr->>IOC: create in-record (pv, datatype)
IOC->>IOC: record_metadata_from_datatype(datatype)
IOC->>IOC: select in-builder for datatype
IOC->>Rec: instantiate in-record via builder
IOC->>Rec: attach datatype_updater callback
Rec-->>Attr: link in-record (read path)
Attr->>IOC: create out-record (pv, datatype, on_update)
IOC->>IOC: record_metadata_from_datatype(datatype, out_record=true)
IOC->>IOC: select out-builder for datatype
IOC->>Rec: instantiate out-record via builder with on_update
IOC->>Rec: attach datatype_updater callback
Rec-->>Attr: link out-record (write path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@GDYendell ready. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/transports/epics/ca/test_softioc.py (1)
547-578:⚠️ Potential issue | 🟡 MinorAssert the out-record builder after switching to _make_out_record.
The test at line 571 still assertsbuilder.longInwhen it should assertbuilder.longOut, as_make_out_recorduses the out-record builder for write attributes. The assertion also needs to include the correct parameters.🔧 Proposed fix
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
🤖 Fix all issues with AI agents
In `@tests/transports/epics/ca/test_initial_value.py`:
- Around line 68-74: The wait loop condition currently uses "and" which exits as
soon as one list is non-empty; change the loop to wait until both spies have
populated lists by using a condition that continues while either list is empty
(e.g., while not (record_spy.spy_return_list and
record_spy_out.spy_return_list)) so initial_values built from
record_spy.spy_return_list + record_spy_out.spy_return_list is complete; update
the while loop surrounding the asyncio.sleep(0) accordingly (refer to the
variables record_spy, record_spy_out and the initial_values comprehension).
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)
tests/transports/epics/ca/test_softioc.py (1)
546-583:⚠️ Potential issue | 🟡 MinorAssert
longOutfor output records in datatype update test.After switching to
_make_out_record, the test still assertsbuilder.longIn, which doesn’t validate the output record builder path. Consider assertinglongOutwithout_record=Truemetadata instead. Line 573.✅ Suggested test fix
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 241-301: datatype_updater currently passes all items from
record_metadata_from_datatype(..., out_record=True) into record.set_field, but
that metadata can include a callable under the "validate" key (for long Enums)
which RecordWrapper.set_field cannot accept; update the datatype_updater closure
to filter out the "validate" key (or any non-scalar/callable values) before
calling record.set_field so only EPICS DB field values are set; ensure this
change is applied where datatype_updater is added via
attribute.add_update_datatype_callback so long Enum handling and other datatypes
remain correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 295-299: datatype_updater is passing the full dict from
record_metadata_from_datatype(..., out_record=True) into record.set_field, which
can include builder-only keys like validate (used by builder.longStringOut) that
are not settable; update datatype_updater to filter out non-field keys (at
minimum remove "validate") from the metadata dict before iterating/calling
record.set_field (use the output of record_metadata_from_datatype but drop keys
such as "validate" or any known builder-only params) so only actual record field
names are passed to record.set_field.
🧹 Nitpick comments (1)
tests/transports/epics/ca/test_softioc.py (1)
547-572: Assert the out-record builder call to fully cover the new out path.After
_make_out_record, the test reassertslongInwith non‑out metadata, so it doesn’t validate the output builder call orout_record=Truemetadata. Consider assertinglongOutwith the update kwargs to exercise the out‑record path.Suggested test adjustment
- builder.longIn.assert_called_once_with( - pv_name, - **record_metadata_from_attribute(attr_w), - **record_metadata_from_datatype(attr_w.datatype), - ) + builder.longOut.assert_called_once_with( + pv_name, + always_update=True, + blocking=True, + on_update=mocker.ANY, + **record_metadata_from_attribute(attr_w), + **record_metadata_from_datatype(attr_w.datatype, out_record=True), + )
|
The nitpick is also correct! The assertion is wrong. 🤖 Fix all issues with AI agents🧹 Nitpick comments (1)
FastCS/tests/transports/epics/ca/test_softioc.py Lines 547 to 570 in 04097f3 Line 566 should assert longOut is called. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/transports/epics/ca/test_softioc.py (1)
562-569:⚠️ Potential issue | 🟡 MinorTest assertion uses wrong builder method but happens to pass.
The test at lines 562-569 asserts
ioc_builder.aOut.assert_called_once_with(...)insidepytest.raises(AssertionError)for long command names. However, commands usebuilder.Action(seeioc.pyline 404), notaOut. This test passes coincidentally becauseaOutis never called for commands, not because of the long name check.Consider updating to assert against
Actionfor clarity, or remove this redundant check sinceioc_builder.Action.assert_called_once_withearlier in the test already validates that only the short command was created.
🧹 Nitpick comments (4)
src/fastcs/transports/epics/ca/ioc.py (1)
305-315: Validator closure capturesdatatypeat definition time.The
_verify_in_datatypeclosure on line 307-308 capturesdatatypeby reference at definition time. Ifupdate_datatypeis later called with a modifiedEnum(e.g., different member names), the validator will still reference the original datatype's names rather than the updated ones.This is likely acceptable if Enum member changes via
update_datatypeare not a supported use case, but worth noting for awareness.tests/transports/epics/ca/test_initial_value.py (2)
75-75: Remove debug print statement.The
print(f"{initial_values}")statement appears to be leftover debug code that should be removed before merge.Proposed fix
- print(f"{initial_values}")
113-113: Remove debug print statement.The
print(f"{name} => {value} {initial_values[name]}")statement appears to be leftover debug code that should be removed before merge.Proposed fix
- print(f"{name} => {value} {initial_values[name]}")tests/transports/epics/ca/test_softioc.py (1)
98-108: Consider cleaning up commented code.The commented-out code on lines 103-107 suggests the initial_value comparison was problematic. While using
mocker.ANYis a valid workaround, consider adding a brief comment explaining why this is necessary (numpy array comparison issues) or removing the dead comments.
ajgdls
force-pushed
the
split-epics-make-record
branch
from
de53cda to
39c7deb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 345-349: datatype_updater is passing every key from
record_metadata_from_datatype(datatype, out_record=True) into record.set_field,
but for long Enums that metadata includes a "validate" callable which
RecordWrapper.set_field cannot accept; update datatype_updater to skip the
"validate" key (e.g., filter out entries where name == "validate" before calling
record.set_field) so only EPICS DB field values are passed to record.set_field.
🧹 Nitpick comments (3)
tests/transports/epics/ca/test_initial_value.py (2)
75-75: Consider removing debug print statement.This debug print appears to be leftover from development/debugging. Consider removing it before merge for cleaner test output.
Proposed fix
- print(f"{initial_values}")
113-113: Consider removing debug print statement.Similarly, this per-item debug print could be removed for cleaner test output.
Proposed fix
- print(f"{name} => {value} {initial_values[name]}")tests/transports/epics/ca/test_softioc.py (1)
617-641: Consider adding a test case for long enum datatype updates.The current test validates
Intdatatype updates for out-records. Given thatrecord_metadata_from_datatype(datatype, out_record=True)returns avalidatecallable for long enums (>16 members), consider adding a test case that exercises the datatype update path for aLongEnumattribute to ensure thevalidatekey is properly filtered before callingset_field.Would you like me to generate a test case for long enum datatype updates?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 252-255: In _make_in_record, update the inner function
datatype_updater(datatype: DataType) to filter out builder-only metadata keys
(e.g., "length" returned by record_metadata_from_datatype for String/Waveform)
before calling record.set_field; locate datatype_updater in _make_in_record and
apply the same filtering logic used in _make_out_record so you only iterate and
call record.set_field for allowed (non-builder) keys.
| def datatype_updater(datatype: DataType): | ||
| for name, value in record_metadata_from_datatype(datatype).items(): | ||
| record.set_field(name, value) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A40 "def record_metadata_from_datatype" src/fastcs/transports/epics/ca/util.pyRepository: DiamondLightSource/FastCS
Length of output: 1651
🏁 Script executed:
rg -n "def _make_in_record" -A20 src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 896
🏁 Script executed:
rg -n "def _make_out_record" -A25 src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 971
🏁 Script executed:
rg -n "builder_only_keys" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 169
🏁 Script executed:
rg -n "def _make_in_record" -A50 src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 2160
🏁 Script executed:
rg -n -B5 -A15 "def datatype_updater" src/fastcs/transports/epics/ca/ioc.pyRepository: DiamondLightSource/FastCS
Length of output: 1572
Add filtering for builder-only keys in _make_in_record datatype updater.
The datatype_updater in _make_in_record (lines 252–254) calls record_metadata_from_datatype(datatype) without filtering. For String and Waveform types, this returns the "length" key, which is a builder-only metadata field that cannot be updated via record.set_field() after record creation. When a datatype is updated dynamically, this will cause a runtime error.
Apply the same filtering used in _make_out_record:
🔧 Suggested fix
def datatype_updater(datatype: DataType):
+ builder_only_keys = {"validate", "length"}
for name, value in record_metadata_from_datatype(datatype).items():
+ if name in builder_only_keys:
+ continue
record.set_field(name, value)🤖 Prompt for AI Agents
In `@src/fastcs/transports/epics/ca/ioc.py` around lines 252 - 255, In
_make_in_record, update the inner function datatype_updater(datatype: DataType)
to filter out builder-only metadata keys (e.g., "length" returned by
record_metadata_from_datatype for String/Waveform) before calling
record.set_field; locate datatype_updater in _make_in_record and apply the same
filtering logic used in _make_out_record so you only iterate and call
record.set_field for allowed (non-builder) keys.
2 participants
This resolves #288.
The first commit removes the original builder_callable_from_attribute function from the util file in ca transport and instead calls the record creation directly in the ioc file.
The second commit then splits the _make_record function into _make_in_record and _make_out_record.
Tests updated at each commit.
Summary by CodeRabbit
Refactor
Chores