Feat: improve pva support using latest pvi changes#383
Open
ggayDiamond wants to merge 11 commits into
Open
Conversation
Closes #373. `Controller.path` is now a keyword-only ``__init__`` parameter on ``BaseController`` (default ``[]``) and is threaded through user code rather than seeded post-construction. The launcher builds each root with ``cls(options, path=[entry.id])`` (or ``cls(path=[entry.id])`` when no options); a parent constructs its subs with the full path already baked in (e.g. ``Sub(path=self.path + [name])``). ``BaseController.set_path()`` is removed entirely. ``add_sub_controller(name, sub)`` no longer mutates ``sub._path`` -- it sanity-asserts ``sub.path == parent.path + [name]`` and rejects the call with a clear error if the caller forgot to thread the path. ``_build_api()`` reads ``self._path`` directly, eliminating the parent-side path argument and the recursive re-prefix it required. Custom ``Controller.__init__`` (root and sub-controller alike) must now accept ``path`` and forward it to ``super().__init__``. Demo controllers and doc snippets are updated to the new shape; tests either construct with ``path=[...]`` from the start or move the id declaration above the controller construction so sub paths can be threaded explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
It decouples sub-controller's path assignment from controller registration.
The path pre-validation in `add_sub_controller` was removed in 63ffd94 to decouple sub-controller path assignment from registration; the `does not match parent path` ValueError no longer fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update fastcs to use pvi 0.14.0b1 beta which includes: - Group label preservation through SubScreen reconstructions - Flattened Grid group handling for INLINE controller hoisting - Improved support for dynamic GroupLayout modes
The name passed to Group() in extract_api_components() must satisfy the
PascalCase constraint required by PVI ('^([A-Z][a-z0-9]*)*$'). When a
controller's path segment contains characters outside that set (hyphens,
digits in certain positions, etc.) the name has to be sanitised before
it can be used as the Group key. PVI then derives the button/screen
label from that sanitised name via to_title_case(), producing something
unreadable (e.g. 'Bl 04i Ea E1rio 01') rather than the intended string.
ControllerAPI.path already holds the original, unsanitised path
segments. Pass api.path[-1] as the optional label= field on the Group
so that PVI's Component.get_label() returns the original name verbatim
instead of falling back to to_title_case(name).
This is backwards-compatible: label defaults to None, so controllers
whose path is empty are unaffected and existing behaviour is preserved.
Introduce a GroupLayout enum (SUBSCREEN | INLINE) that lets callers
specify, per sub-controller, how its children should be presented on
the parent screen.
SUBSCREEN (default) — children appear on a separate screen opened by a
navigate button, preserving the previous behaviour.
INLINE — children are rendered as an inline Grid block directly on the
parent screen, without an extra navigation level.
The enum is decoupled from PVI: it lives in fastcs.controllers, which
has no dependency on any transport. The EPICS GUI layer (gui.py)
resolves it to the appropriate pvi.device layout object when building
the component tree.
Usage:
from fastcs.controllers import Controller, GroupLayout
class MyController(Controller):
group_layout = GroupLayout.INLINE # class-level default
# or per-instance:
ctrl = MyController(group_layout=GroupLayout.INLINE)
parent.add_sub_controller("Ctrl", ctrl)
Sanitize child controller names for PVA field names by replacing non-alphanumeric characters (e.g. hyphens from beamline PV prefix formats like 'BL04I-EA-E1RIO-01') with underscores. This prevents P4P validation errors. Also make EpicsGUIOptions.title optional (defaults to None) and derive a meaningful default from the controller's path when not explicitly provided, improving upon the hardcoded 'FastCS Devices' placeholder. This allows GUI screens and docs to display the controller ID as the title when no explicit title is set. Updates schema.json files to reflect the title field now accepting null values.
|
Warning Review limit reached
More reviews will be available in 29 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR refactors controller path initialization from post-construction ( ChangesPath and Layout Support
Demo and Transport Implementation
Test Updates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 91.20% 91.26% +0.06%
==========================================
Files 72 72
Lines 2875 2885 +10
==========================================
+ Hits 2622 2633 +11
+ Misses 253 252 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/pva/test_p4p.py (1)
312-312:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing colon and verify PV path structure.
The PV string is missing a colon separator between
pv_prefixandChild. Additionally, based on the controller setup at lines 280-283, the ControllerVector children don't havechild_childattributes—only the direct sub-controllers (child0, child1, etc.) created at lines 286-292 havechild_child. This monitor will fail because the PV doesn't exist.🐛 Proposed fix
If the intent is to monitor the
child_childof the first ControllerVector element, you'll need to addchild_childattributes to those controllers. However, it's more likely you meant to monitorchild0.child_child:- f"{pv_prefix}Child:0:ChildChild:PVI", child_child_child_controller_pvi.append + f"{pv_prefix}:Child0:ChildChild:PVI", child_child_child_controller_pvi.append🤖 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` at line 312, The PV path passed to child_child_child_controller_pvi is malformed and targets a non-existent attribute on ControllerVector children: add the missing colon after pv_prefix and correct the path to reference the first child's child_child (i.e., use pv_prefix + ":Child:0:child_child:PVI" semantics) or, alternatively, modify the ControllerVector children (created around ControllerVector and child0/child1) to expose a child_child attribute so the original PV path becomes valid; ensure you update the PV string and/or the controller setup to match the actual attribute hierarchy (symbols to inspect: pv_prefix, child_child_child_controller_pvi, ControllerVector, child0).
🧹 Nitpick comments (2)
src/fastcs/controllers/controller_vector.py (1)
22-25: ⚡ Quick winExpose
group_layoutinControllerVector.__init__for API symmetry.
Controllersupports per-instancegroup_layout, butControllerVectorcurrently does not. Adding it keeps the new layout feature consistent across controller types.Proposed refactor
from fastcs.controllers.base_controller import BaseController from fastcs.controllers.controller import Controller +from fastcs.controllers.controller_api import GroupLayout from fastcs.util import Controller_T @@ def __init__( self, children: Mapping[int, Controller_T], description: str | None = None, ios: Sequence[AnyAttributeIO] | None = None, *, path: list[str] | None = None, + group_layout: GroupLayout | None = None, ) -> None: - super().__init__(description=description, ios=ios, path=path) + super().__init__( + description=description, + ios=ios, + path=path, + group_layout=group_layout, + )🤖 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/controllers/controller_vector.py` around lines 22 - 25, ControllerVector.__init__ should accept the same per-instance group_layout as Controller: add a new parameter group_layout: GroupLayout | None = None (or matching Controller's type) to the ControllerVector.__init__ signature and forward it into the super().__init__ call (i.e. super().__init__(description=description, ios=ios, path=path, group_layout=group_layout)); ensure imports/typing match the Controller's GroupLayout type and update any callers/tests that construct ControllerVector without breaking defaults.tests/transports/epics/pva/test_p4p.py (1)
281-281: 💤 Low valueConsider spread syntax for list concatenation.
For modern Python style, spread syntax is slightly more explicit than concatenation.
♻️ Optional refactor
sub_controller_vector = ControllerVector( - {i: ChildController(path=child_path + [str(i)]) for i in range(3)}, + {i: ChildController(path=[*child_path, str(i)]) for i in range(3)}, path=child_path, )sub_controller.child_child = ChildChildController( - path=sub_path + ["child_child"] + path=[*sub_path, "child_child"] )Also applies to: 291-291
🤖 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` at line 281, Replace list concatenation using child_path + [str(i)] with Python's spread/unpacking syntax to be more explicit and modern; update the dict comprehension that constructs ChildController(path=child_path + [str(i)]) to ChildController(path=[*child_path, str(i)]) and make the identical change at the other occurrence referenced (the second instance around line 291) so both comprehensions use [*child_path, str(i)].
🤖 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-38: The dependency entries for pvi in pyproject.toml (under
epicsca and epicspva) use an open-ended spec "pvi>=0.14.0b1" which can pull in
future breaking releases; update both entries to constrain the upper bound (for
example "pvi>=0.14.0b1,<0.15") or pin to "pvi==0.14.0b1" depending on whether
you want a range or exact pin so that epicsca and epicspva do not accept
unintended future pvi versions.
In `@src/fastcs/controllers/controller_api.py`:
- Line 45: The docstring in src/fastcs/controllers/controller_api.py (the
triple-quoted string describing "How this controller's children are laid out
when rendered inside a parent screen.") exceeds the configured line length;
shorten or split the sentence into multiple lines to satisfy Ruff E501 (e.g.,
break into two shorter sentences or rephrase) so the docstring lines are within
the max length while preserving the same meaning; update the triple-quoted
docstring in the module/class/method where that text appears.
In `@src/fastcs/transports/epics/emission.py`:
- Around line 148-158: The current call to _render_index_md uses "options.title
or (...)" which treats an empty string as missing; change it to only fall back
when options.title is None (e.g. use a conditional or ternary) so explicit empty
titles "" are preserved; update the invocation of
_render_index_md(controller_apis, options.title) to compute the fallback title
only when options.title is None, leaving options.title unchanged for "" values.
---
Outside diff comments:
In `@tests/transports/epics/pva/test_p4p.py`:
- Line 312: The PV path passed to child_child_child_controller_pvi is malformed
and targets a non-existent attribute on ControllerVector children: add the
missing colon after pv_prefix and correct the path to reference the first
child's child_child (i.e., use pv_prefix + ":Child:0:child_child:PVI" semantics)
or, alternatively, modify the ControllerVector children (created around
ControllerVector and child0/child1) to expose a child_child attribute so the
original PV path becomes valid; ensure you update the PV string and/or the
controller setup to match the actual attribute hierarchy (symbols to inspect:
pv_prefix, child_child_child_controller_pvi, ControllerVector, child0).
---
Nitpick comments:
In `@src/fastcs/controllers/controller_vector.py`:
- Around line 22-25: ControllerVector.__init__ should accept the same
per-instance group_layout as Controller: add a new parameter group_layout:
GroupLayout | None = None (or matching Controller's type) to the
ControllerVector.__init__ signature and forward it into the super().__init__
call (i.e. super().__init__(description=description, ios=ios, path=path,
group_layout=group_layout)); ensure imports/typing match the Controller's
GroupLayout type and update any callers/tests that construct ControllerVector
without breaking defaults.
In `@tests/transports/epics/pva/test_p4p.py`:
- Line 281: Replace list concatenation using child_path + [str(i)] with Python's
spread/unpacking syntax to be more explicit and modern; update the dict
comprehension that constructs ChildController(path=child_path + [str(i)]) to
ChildController(path=[*child_path, str(i)]) and make the identical change at the
other occurrence referenced (the second instance around line 291) so both
comprehensions use [*child_path, str(i)].
🪄 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: 2fd66ca9-6858-4e64-be53-ef47bc31744e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
docs/snippets/dynamic.pydocs/snippets/static04.pydocs/snippets/static05.pydocs/snippets/static06.pydocs/snippets/static07.pydocs/snippets/static08.pydocs/snippets/static09.pydocs/snippets/static10.pydocs/snippets/static11.pydocs/snippets/static12.pydocs/snippets/static13.pydocs/snippets/static14.pydocs/snippets/static15.pypyproject.tomlsrc/fastcs/controllers/__init__.pysrc/fastcs/controllers/base_controller.pysrc/fastcs/controllers/controller.pysrc/fastcs/controllers/controller_api.pysrc/fastcs/controllers/controller_vector.pysrc/fastcs/demo/controllers.pysrc/fastcs/demo/schema.jsonsrc/fastcs/launch.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/emission.pysrc/fastcs/transports/epics/gui.pysrc/fastcs/transports/epics/options.pytests/assertable_controller.pytests/benchmarking/controller.pytests/conftest.pytests/data/schema.jsontests/example_p4p_ioc.pytests/example_softioc.pytests/test_controllers.pytests/test_launch.pytests/test_multi_controller.pytests/transports/epics/ca/test_gui.pytests/transports/epics/ca/test_initial_value.pytests/transports/epics/ca/test_softioc.pytests/transports/epics/pva/test_p4p.pytests/transports/epics/test_emission.pytests/transports/graphQL/test_graphql.pytests/transports/tango/test_dsr.py
ggayDiamond
force-pushed
the
feat/improve-pva-support
branch
from
1380ac2 to
267462b
Compare
- Change return type from Tree to ResolvedTree to match Group.children parameter - Add cast() to handle list[ComponentUnion | Include] to ResolvedTree conversion - The method never appends Include objects, so the cast is semantically safe - Fixes pyright error: Type parameter variance issue with Include in Tree union This resolves the type checking failure when running 'tox -p' without affecting the runtime behavior of the code.
…o-options path Add two targeted tests to improve code coverage: 1. test_controller_with_group_layout() - Tests that the group_layout parameter is properly initialized in BaseController.__init__() when explicitly passed and that it defaults to GroupLayout.SUBSCREEN. Covers line 57 in base_controller.py. 2. test_launch_single_arg_no_options() - Tests the launch system's handling of controllers that take no configuration options. Exercises the code path where registered.cls() is called with only path=[entry.id]. Covers line 236 in launch.py. These tests address CodeCov patch coverage report gaps.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Update fastcs to leverage pvi 0.14.0b1 and enhance PVA field name handling for hyphened PV prefixes.
Adds GroupLayout support for controlling sub-controller GUI layout (inline vs. subscreens).
Changes
Core Features
pvi 0.14.0b1 Upgrade: Incorporates improvements to PVI library:
GroupLayout Support: New GroupLayout enum allows controllers to specify:
PVA Field Name Sanitization: Replace non-alphanumeric characters (e.g., hyphens from beamline-style PV prefixes like BL04I-EA-E1RIO-01) with underscores to comply with P4P field naming requirements
Improvements
GUI Title Handling: EpicsGUIOptions.title now optional with intelligent fallback:
Label Preservation: Fixed downstream sub-controller names now properly preserved as Group display labels through screen transformations
Summary by CodeRabbit
New Features
Refactor
Chores