attributes: replace the DataType family with python types and *Meta typed dicts - #418
attributes: replace the DataType family with python types and *Meta typed dicts#418coretl wants to merge 2 commits into
*Meta typed dicts#418Conversation
…yped dicts An attribute's datatype is now the python type it holds - `float`, an Enum subclass, `Array1D[np.int32]`, `Table` - and everything that used to hang off a `DataType` instance travels separately as metadata on `attr.meta`. - New `fastcs.datatypes`: `Array1D`/`Table` datatype spellings, the per-datatype `*Meta` typed dicts (plus the superset `Meta`), nested `NumericLimits`, and the validation the `DataType` classes used to do. - `Attr*` constructors are overloaded per datatype, so `AttrRW(str, precision=3)` is a static type error; `validate_meta` is the runtime counterpart for metadata that arrives without a static check. - Naming pass (ADR 0017): `prec` -> `precision`, and the flat `min`/`max`/`min_alarm`/`max_alarm` become nested control/display/alarm/warning limits with inheritance. - Every transport repointed from `attr.datatype.*` to `attr.dtype` + `attr.meta`; `add_update_datatype_callback` becomes `add_update_meta_callback`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zpFjvbhUisfVUq3NH8M1G
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
The p4p enum test still read `attr.datatype.members`; it cannot run in the sandbox (no PVA socket family), so CI was the first to see it. Three new docstrings also referenced `ControllerFiller`, which does not exist until #394, and an ambiguous `fastcs.datatypes.meta` - sphinx builds with warnings as errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zpFjvbhUisfVUq3NH8M1G
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor #418 +/- ##
============================================
+ Coverage 91.25% 91.40% +0.15%
============================================
Files 72 67 -5
Lines 2892 3003 +111
============================================
+ Hits 2639 2745 +106
- Misses 253 258 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shihab-dls
left a comment
shihab-dls
left a comment
There was a problem hiding this comment.
This ended up looking pretty good; requesting a few changes, but won't iterate too much afterwards.
| Enum_T = TypeVar("Enum_T", bound=enum.Enum) | ||
| """A TypeVar of any `enum.Enum` subclass an attribute can hold""" | ||
|
|
||
| Array_T = TypeVar("Array_T", bound=np.ndarray) | ||
| """A TypeVar of any numpy array an attribute can hold""" | ||
|
|
||
|
|
||
| Inferred_T = TypeVar("Inferred_T", bound=DType) | ||
| """A TypeVar of `DType` for the constructor overload that infers the datatype | ||
|
|
||
| Distinct from `DType_T` because the overload binds it from the getter or setter | ||
| in the same signature that annotates ``self``, and a class-scoped TypeVar cannot | ||
| be used there. | ||
| """ |
There was a problem hiding this comment.
nit: this should be placed at the top with the other TypeVars
| def convert_datatype(dtype: type[DType]) -> type: | ||
| """Converts a datatype to a rest serialisable type.""" | ||
| match datatype: | ||
| case Waveform(): | ||
| return list | ||
| case _: | ||
| return datatype.dtype | ||
| if issubclass(dtype, np.ndarray): | ||
| return list | ||
|
|
||
| return dtype |
There was a problem hiding this comment.
I think this is also a bug fix as previously Table would've fallen into the return dtype instead of returning list as it is a subclass of np.ndarray
| if dtype is bool: | ||
| record = builder.boolIn(pv, ZNAM="False", ONAM="True", **common_fields) | ||
| elif dtype is int: | ||
| record = builder.longIn( | ||
| pv, | ||
| EGU=meta.get("units"), | ||
| **_display_limit_fields(meta), | ||
| **common_fields, | ||
| ) | ||
| elif dtype is float: | ||
| record = builder.aIn( | ||
| pv, | ||
| EGU=meta.get("units"), | ||
| PREC=meta.get("precision", DEFAULT_PRECISION), | ||
| **_display_limit_fields(meta), | ||
| **common_fields, | ||
| ) | ||
| elif dtype is str: | ||
| record = builder.longStringIn(pv, length=_string_length(meta), **common_fields) | ||
| elif issubclass(dtype, enum.Enum): | ||
| if len(enum_names(dtype)) > MBB_MAX_CHOICES: | ||
| record = builder.longStringIn(pv, **common_fields) | ||
| else: | ||
| common_fields.update(create_state_keys(dtype)) | ||
| record = builder.mbbIn(pv, **common_fields) | ||
| elif issubclass(dtype, np.ndarray): | ||
| record = builder.WaveformIn(pv, length=_array_length(meta), **common_fields) | ||
| else: | ||
| raise FastCSError(f"EPICS unsupported datatype on {attribute}: {dtype}") | ||
|
|
||
| _mirror_meta_onto_record(attribute, record, _in_record_fields) |
There was a problem hiding this comment.
nit: this used to be a match block, but is now a bunch of if.. elif. I think the match block was cleaner.
| def _attribute_of_unsupported_datatype(mocker: MockerFixture): | ||
| attribute = mocker.MagicMock() | ||
| attribute.dtype = object | ||
| attribute.meta = {} |
There was a problem hiding this comment.
nit: this does not need to be set.
| _TABLE_META = Meta( | ||
| structured_dtype=[("int", np.int16), ("bool", np.bool), ("str", np.dtype("S10"))] | ||
| ) |
There was a problem hiding this comment.
nit: move this near the top of the page before all the tests.
| def test_resolve_datatype_takes_an_enum_class(): | ||
| class Colour(Enum): | ||
| RED = "red" | ||
|
|
||
| assert resolve_datatype(Colour) == (Colour, None) |
There was a problem hiding this comment.
should: this is the same as the above test test_resolve_datatype but specific to enum. Instead, we can include this in the above test parameters, defingin class Colour(Enum) near the top of the page before all the tests.
| <display version="2.0.0"> | ||
| <name>Demo Vector</name> | ||
| <x>0</x> | ||
| <y use_class="true">0</y> | ||
| <width>388</width> | ||
| <height>130</height> | ||
| <grid_step_x>4</grid_step_x> | ||
| <grid_step_y>4</grid_step_y> | ||
| <widget type="label" version="2.0.0"> | ||
| <name>Title</name> | ||
| <class>TITLE</class> | ||
| <text>Demo Vector</text> | ||
| <x use_class="true">0</x> | ||
| <y use_class="true">0</y> | ||
| <width>388</width> | ||
| <height>25</height> | ||
| <font use_class="true"> | ||
| <font name="Header 1" family="Liberation Sans" style="BOLD" size="22.0"> | ||
| </font> | ||
| </font> | ||
| <foreground_color use_class="true"> |
There was a problem hiding this comment.
must: we shouldn't include the generated bob files in the repo. Add the opis dir into the .gitignore. Also, example_p4p_ioc should also be provided gui_options not just example_softioc
3 participants
Closes #413
An attribute's datatype is now the python type it holds, and everything that used to hang off a
DataTypeinstance — precision, units, limits, array shape — travels separately as metadata onattr.meta. This completes theDataTypedrop split out of #392, and absorbs the naming pass (#396 / ADR 0017) as that issue directed.Scope
DataTypeis gone.datatype.py,_numeric.py,bool.py,float.py,int.py,string.py,enum.py,waveform.pyandtable.pyare deleted.fastcs.datatypesnow holds:types.py— theDTypeunion,Array1D[np.int32](a subscripted numpy alias that is both the hint and the datatype),Table, andresolve_datatype.meta.py—BoolMeta/IntMeta/FloatMeta/StrMeta/EnumMeta/Array1DMeta/TableMeta, plus the supersetMetafor the declarative path (Example 4 — SCPI device: annotated attributes + per-attribute filler data #405'sSCPIParam, ControllerFiller — declarative/procedural split #394's filler).limits.py— nestedNumericLimitswithLimitsper category.validation.py—validate_value/values_equal/default_value/validate_meta, which is what theDataTypeclasses used to do invalidate/equal/initial_value.attr.metastores the resolved metadata;attr.dtypeis the python type.attr.datatypeis gone, anddescription/groupare meta fields with properties reading them.AttrR/AttrW/AttrRW.__init__are overloaded once per datatype with**meta: Unpack[*Meta], soAttrRW(str, precision=3)does not type check.validate_metais the runtime counterpart — the error names the field, datatype and attribute ('precision' is not valid metadata for str attribute device_id), which is the message ControllerFiller — declarative/procedural split #394/Example 4 — SCPI device: annotated attributes + per-attribute filler data #405 need from the filler.prec→precision; the flatmin/max/min_alarm/max_alarmbecomeNumericLimits(control=…, display=…, alarm=…, warning=…), all optional, with the ADR's inheritance rules (control inherits display, warning inherits alarm, warning ⊆ alarm asserted). Only the control range rejects a value; the rest are served to clients.Waveform→Array1D:AttrR(Array1D[np.int32], shape=(10,))— the element type rides on the subscript. Arrays of rank > 1 keep working, written asAttrR(np.ndarray, array_dtype=np.uint16, shape=(1024, 1024)), sinceArray1Dis by name one dimensional and there is no ophyd-async spelling for higher ranks.util.py,ioc.py), PVA (types.py,_pv_handlers.py,gui.py), the shared EPICS GUI, Tango, REST and GraphQL all dispatch onattr.dtypeand read what they serve fromattr.meta.add_update_datatype_callback→add_update_meta_callback. The cast helpers now take theAttributerather than a datatype object, since validation lives on the attribute.docs/snippets/*.py, and the prose docs (explanations/datatypes.mdrewritten,how-to/table-waveform-data.md,explanations/transports.md, and the rest) migrated.Instructions to reviewer on how to test:
uv run --locked tox -e pre-commit,type-checking— both green.uv run pytest tests/test_datatypes.py tests/test_attributes.py -vpython -m fastcs.demo run src/fastcs/demo/fastcs.yamlagainst the sim (tickit all src/fastcs/demo/simulation/temp_controller.yaml)Checks for reviewer
min_alarm/max_alarmdrove EPICSLOPR/HOPR, which are EPICS' display range, not its alarm limits. With the categories now named, this PR mapsdisplay→LOPR/HOPRandcontrol→DRVL/DRVH(and PVA/Tango likewise), so a driver that previously setmin_alarmto get a display range now setsdisplay. Servingalarm/warningas CALOLO/HIHI/LOW/HIGHis left out — it needs the severity fields set too, and is a behaviour addition rather than this rename.np.ndarray; what separates them is that a table's metadata names its columns, so transports check forstructured_dtypeinattr.meta. DeclaringTablewithout columns, orstructured_dtypewithoutTable, fails fast at construction.Notes
boolmatchesintwhileintmatchesfloat, soAttrR(bool, units=...)resolves to theintoverload rather than failing statically; the constructor's runtime check rejects it. A call whose metadata is valid always picks its own datatype's overload (AttrR(bool)isAttrR[bool],AttrR(int)isAttrR[int]). This is commented at the overload block rather than left for a reader to discover.DataType.all_equalwas unused outside its own test and is not carried over.tests/transports/epics/ca/test_initial_value.pyhad attributes literally namedint/float/bool/str; harmless when the datatype wasInt(), but they shadow the builtins in the class body now, so they are renamed*_rw(PV names follow).uv run --locked tox -e pre-commit,type-checking, both green in full. For thetestsenv, this sandbox can't rundocs(needs outbound network todiamondlightsource.github.io) or the PVA/p4p-backed tests (RuntimeError: Address family not supported by protocol— no PVA-capable socket family), the same known limitation noted on 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/attributes: getter/setter IO rework, remove AttributeIORef/AttributeIO #412. Excluding those two paths,pytest src tests --ignore=tests/benchmarkingpasses 361/371, with only the same 10 pre-existing p4p/socket-family failures — I confirmed those 10 are identical onrefactoritself by running that file in a worktree off the base commit. Real CI coversdocsand PVA.Generated by Claude Code