Skip to content

add: arbitrary-axis HKL chain and profile-driven IOC RSM editor (2/4, closes #132) - #137

Open
pecomyint wants to merge 5 commits into
rsm/01-config-geometryfrom
rsm/02-naxis-issue132
Open

add: arbitrary-axis HKL chain and profile-driven IOC RSM editor (2/4, closes #132)#137
pecomyint wants to merge 5 commits into
rsm/01-config-geometryfrom
rsm/02-naxis-issue132

Conversation

@pecomyint

@pecomyint pecomyint commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Stack 2 of 4. Base rsm/01-config-geometry — retarget to main after PR 1 merges. Simulator only, no beam time.

Closes #132.

@Osayi-ANL — you implemented #132 independently on origin/issue/132 the same day this was written. Peco's call was to go with this stack, so that branch is superseded. Two things from your work are worth carrying over and are not in this PR: you migrated seven production configs (19-ID, 4-ID ×2, 6-ID, 8-ID, SSRL BL17-2) where this only migrates sample_config.toml, and settings.HKL_SAMPLE_CIRCLES/HKL_DETECTOR_CIRCLES is a friendlier accessor than routing everything through rsm_geometry. The six unmigrated configs still work — verified, they resolve to 6 circles each through the legacy numbered-group path — they just have no canonical section yet. Sorry for the duplicated effort.

The problem

Making the IOC editor generic is necessary but not sufficient. Five other places capped the geometry at 4 sample + 2 detector circles, and every one failed silently — wrong physics, no error:

Site Was
hdf5_writer.py:202 literal list of six group names; a 5th circle was never written to the file
rsm_converter.py:200 range(1,5) / range(1,3) on read
hpc_rsm_consumer.py:197 matched pv_name.endswith('Position')
area_det_viewer.py:1889 same suffix matching
settings.py:78-81 NeXus links hardcoded sample_phi/chi/eta/mu, no detector links at all

A beamline whose PV ends in .RBV lost that circle. A kappa goniometer was unrepresentable.

What this changes

Chain. resolved_axis_groups discovers numbered groups per role, sorted by integer suffix so _10 follows _9, with the legacy MU/ETA/CHI/PHI layout kept as a per-role fallback. Dispatch is on semantic config keys (DIRECTION_AXIS, POSITION), never PV-name shape. NeXus transformation links are generated per axis from the stable RECORD_NAME, and detector circles finally get links too. Axis metadata is preflighted before the output file is created, so a missing direction fails by name instead of dropping a circle.

Editor. Ordered SAMPLE_AXES/DETECTOR_AXES arrays; LABEL split from an EPICS-safe RECORD_NAME so renaming an axis in the GUI doesn't rename its PV; per-axis DIRECTION and ANGLE_UNITS; ENERGY_UNITS; SAMPLE_ORIENTATION; SCHEMA_VERSION. Add/remove/rename/reorder, any circle count including zero for a role when the orientation makes that valid. Sample axes accept [xyzk][+-], so kappa goniometers work.

One staged Apply & Save: validate → show the first-adoption diff → CAS-save → read back → restart the IOC, which re-reads the profile from the database. A revision conflict applies nothing and sends no IOC commands.

Per AGENTS.md the database is the config source and TOML is the import/export format, so this PR keeps exactly those two: the Config tab's Import TOML Config / Export Config to File already round-trip the whole profile including IOC_RSM_PARAMETER. There is no separate JSON import, and the IOC child is no longer handed a serialized snapshot file.

Verification

Simulator IOC; 0/3/8-axis geometries; _10 ordering; fifth-axis HDF5 round trip; kappa vectors; cross-role RECORD_NAME uniqueness; .RBV-suffixed sources; CAS conflict behavior; old profile and old file compatibility. Full suite green, ruff clean.

#132)

Removes the fixed four-sample/two-detector circle limit from every stage of
the RSM chain and replaces the hardcoded 6-ID-B axis table in the IOC RSM
Parameter tool with a profile-backed, geometry-agnostic editor.

Chain (no fixed limits, no PV-name sniffing):
- hkl_axes.resolved_axis_groups discovers numbered SAMPLE/DETECTOR_CIRCLE_AXIS_N
  groups per role, sorted by integer suffix so _10 follows _9, with the legacy
  MU/ETA/CHI/PHI and NU/DELTA layout retained as a per-role fallback.
- hdf5_writer, rsm_converter, hpc_rsm_consumer, hpc_metadata_consumer,
  area_det_viewer, hdf5_loader and metadata_converter now dispatch on semantic
  configuration keys (DIRECTION_AXIS, POSITION) instead of matching PV-name
  suffixes, so a circle whose PV ends in .RBV resolves correctly.
- NeXus transformation links are generated per axis from the stable
  RECORD_NAME rather than the beamline-specific sample_phi/chi/eta/mu table,
  and detector circles now get links too.
- Axis metadata is preflighted before the output file is created, so a missing
  direction or position fails by name instead of silently dropping a circle.

Editor (#132):
- IOC_RSM_PARAMETER becomes the canonical geometry section: ordered
  SAMPLE_AXES/DETECTOR_AXES arrays, LABEL split from a stable EPICS-safe
  RECORD_NAME, per-axis DIRECTION and ANGLE_UNITS, ENERGY_UNITS,
  SAMPLE_ORIENTATION and SCHEMA_VERSION.
- Axis add, remove, rename and reorder; any circle count including zero for a
  role when the sample orientation makes the geometry valid.
- Effective [HKL] is generated in memory from the canonical section; the raw
  hand-authored [HKL] on disk is never rewritten, and unknown groups survive.
- One staged Apply & Save: validate, show the first-adoption diff, CAS-save,
  read back, then activate the IOC from the exact saved snapshot. A revision
  conflict applies nothing and sends no IOC commands.
- Explicit, idempotent import of the legacy ~/.config JSON.
- IOC_PREFIX is the single prefix source; generated channels are fully expanded.

Adds test_naxis_data_chain, test_hkl_config_semantics and test_rsm_parameter_ioc
covering 0/3/8-axis geometries, _10 ordering, fifth-axis HDF5 round trip, kappa
vectors, cross-role RECORD_NAME uniqueness, CAS conflict behavior and legacy
migration idempotence.
@pecomyint
pecomyint requested a review from Osayi-ANL August 12, 2026 04:21
Ordered, role-split views of the effective HKL circle groups, rebuilt on
reload. Call sites that only need "the sample circles, in order" no longer have
to know the group-naming or numeric-sort rules.

Carried over from @Osayi-ANL's independent #132 work on origin/issue/132, which
exposed the same convenience. The implementation here derives the views through
resolved_axis_groups -- the same resolution rsm_geometry uses to build the Q
conversion -- so the convenience view can never disagree with the geometry the
physics actually runs on.

@Osayi-ANL Osayi-ANL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For issue 137 on a previously existing profile, I try to reseed, so add the IOC_RSM_PARAMETER onto the profile. It adds it to the profile, but when I attempt to open the HKL Setup I get this error below.

ValueError: IOC_RSM_PARAMETER.SAMPLE_AXES must be a list of axis tables
Traceback (most recent call last):
File "", line 198, in _run_module_as_main
File "", line 88, in _run_code
File "/home/beams0/OODIASEIGIEHON/DashPVA/src/dashpva/consumers/ioc_rsm_parameter.py", line 26, in
import dashpva.settings as app_settings
File "/home/beams0/OODIASEIGIEHON/DashPVA/src/dashpva/settings.py", line 533, in
reload()
File "/home/beams0/OODIASEIGIEHON/DashPVA/src/dashpva/settings.py", line 325, in reload
cfg = resolve_profile_config(raw_cfg)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/beams0/OODIASEIGIEHON/DashPVA/src/dashpva/utils/config/resolver.py", line 115, in resolve_profile_config
for index, axis in enumerate(_canonical_axes(parameters, key), start=1):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/beams0/OODIASEIGIEHON/DashPVA/src/dashpva/utils/config/resolver.py", line 69, in _canonical_axes
raise ValueError(f"IOC_RSM_PARAMETER.{key} must be a list of axis tables")
ValueError: IOC_RSM_PARAMETER.SAMPLE_AXES must be a list of axis tables

… paths

Follow-up to the review fixes on #137.

Empty tables were still corrupted. The previous commit taught the config tree
to round-trip lists, but a table with no children is indistinguishable from a
leaf, so its blank display text was read back as ''. That turned `[HKL]` --
empty by design once IOC_RSM_PARAMETER generates it -- into a string, and
because the same commit added pre-save validation the result was Apply & Save
failing outright with "Refusing to save an unloadable profile: HKL must be a
table" on the very layout this stack ships. Group items now carry a marker so
an empty table extracts as {} rather than falling into the leaf path.

The list round-trip also worked only by accident. PyQt stores a dict as a
key-sorted QVariantMap, so a list of axis tables came back with its fields
reordered and the "text still matches str(original)" guard never matched;
recovery happened via the ast.literal_eval fallback instead, and every save
silently rewrote the axis tables with alphabetised keys. Values now travel in
an opaque wrapper Qt passes through untouched, and edits are detected by
comparing against the text actually rendered. The round-trip is now an exact
identity, key order included.

Removes two redundant configuration paths. AGENTS.md is explicit that the
database is the config source and TOML is the import/export format; both of
these were a third way:

- HKL Setup's "Import legacy JSON..." button, along with migrate_legacy_json
  and the ~/.config path constant. The Config tab's Import TOML Config and
  Export Config to File already round-trip the whole profile, including
  IOC_RSM_PARAMETER.
- The --snapshot temp file. The IOC child was handed the entire profile as a
  serialized JSON file; it now reads the database through the ConfigSource
  path it already had as a fallback. Activation only happens after a
  successful compare-and-swap save, so the database already is the approved
  snapshot -- the file added a format and a cleanup path for nothing.

The stdin/stdout JSON pipe to the IOC child stays: it predates this stack and
carries live PV readings, not configuration.

Adds test_workflow_config_tree.py (25 tests) driving the real QTreeWidget,
since the mechanism under test is how PyQt stores item data. Asserts the
shipped sample_config round-trips identically and still resolves -- Osayi's
crash as a regression test -- plus the same for a pre-canonical profile with a
populated [HKL] and no IOC_RSM_PARAMETER, empty tables staying tables, every
list-valued key keeping type and value, axis field order surviving, and an
edited list recovering structurally.
@pecomyint

Copy link
Copy Markdown
Collaborator Author

Thanks @Osayi-ANL — reproduced exactly, and it's ours, not something pre-existing you tripped over. main had zero list-valued config keys, so nothing could trigger it; this stack adds eight, which activated a latent limitation in the Workflow config tree.

It was broader than reseed. _extract_tree_to_dict is shared by Apply & Save, save-to-legacy-TOML, and export-to-file as well, so anyone who opened the Config tab and saved would have hit it. Good catch before this reached a beamline.

Fixed across two commits:

2493b8e (pushed earlier)

  • resolve_profile_config is guarded at both call sites — the module-level reload() that runs at import, and Settings.__init__. A bad profile now degrades CONFIG to the raw mapping and records CONFIG_ERROR instead of making import dashpva.settings raise. That's what was taking down DashPVA run and HKL Setup together, and it mattered most because it locked you out of the very tool that repairs the profile.
  • The tree keeps the original typed value alongside the display text.

8dc4067 (new)

  • Empty tables were still corrupting. A table with no children is indistinguishable from a leaf, so [HKL] — empty by design once IOC_RSM_PARAMETER generates it — read back as ''. Combined with the new pre-save validation that turned into Refusing to save an unloadable profile: HKL must be a table on the shipped layout. Group items now carry a marker so an empty table extracts as {}.
  • The list round-trip only worked by accident. PyQt stores a dict as a key-sorted QVariantMap, so axis tables came back with fields reordered, the "text still matches" guard never matched, and recovery fell through to ast.literal_eval. Values now travel in an opaque wrapper Qt passes through untouched. The round-trip is an exact identity, key order included.
  • Per AGENTS.md ("the database is the primary config source, TOML is secondary"), two redundant config paths are gone: HKL Setup's Import legacy JSON… button and migrate_legacy_json, and the --snapshot temp file that handed the IOC child a serialized profile. The child now reads the database through the ConfigSource path it already had — activation only happens after a successful CAS save, so the database already is the approved snapshot. The stdin/stdout JSON pipe stays; it predates this stack and carries live PV readings, not config.

New tests/unit/test_workflow_config_tree.py (25 tests) drives the real QTreeWidget, since the mechanism under test is how PyQt stores item data. It asserts the shipped sample_config.toml round-trips identically and still resolves — your crash as a regression test — plus the same for a pre-canonical profile with a populated [HKL] and no IOC_RSM_PARAMETER, which is the backward-compatibility guarantee you asked about.

480 tests pass on the stack tip, ruff check src/dashpva/ clean.

The stack is also rebased so #138/#139 show clean diffs again — rsm/03 was still on the pre-fix rsm/02 and rsm/04 carried a duplicate cherry-pick of the same change.

Worth a git pull before re-reviewing (all three branches were force-updated). And when you have a moment, #138 is still unreviewed — it's the beam-physics gate, so it's the one that most needs your eyes before the trip.

@pecomyint
pecomyint requested a review from Osayi-ANL August 12, 2026 22:12
QPushButton("Apply & Save") left a single unescaped '&' before a space,
which Qt's mnemonic parser consumes and assigns to the space -- some
styles render that as a stray underscore glyph instead of an invisible
underline. Escape it as "Apply && Save".

The Reload/Retry/Apply & Save control row also lived inside the same
QScrollArea as the axis tables, so it scrolled out of view on longer
profiles. Move it into the window's own layout, below the scroll area.

Both reported by Osayi-ANL on #139.
@pecomyint

Copy link
Copy Markdown
Collaborator Author

Traced this to the exact commit: the traceback matches settings.py at 1ff3f10 line-for-line — reload() called resolve_profile_config(raw_cfg) with no fallback, and the config tree was still corrupting empty tables/lists at that point (the specific bug that produced a malformed SAMPLE_AXES).

Both were fixed the same day, ~20–60 min after this review:

  • 2493b8e — guards resolve_profile_config and preserves list/dict values through the config tree
  • 8dc4067 — fixes empty tables surviving the tree extraction (this is what was turning SAMPLE_AXES into something that wasn't a list)

I reproduced your exact scenario against current tip (legacy profile → reseed → adds IOC_RSM_PARAMETER → persist → re-read from DB → resolve_profile_config) with a real SQLite profile and it now round-trips clean with no crash.

Current tip is 91924cf (also picked up two more of your #139 comments — see there). Could you re-pull and retest? Re-requesting your review.

@Osayi-ANL Osayi-ANL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants