Multiple top-level Controllers per FastCS app (#351)#360
Merged
Conversation
Introduce a stable per-controller identifier set once by the launcher between __init__ and initialise(). Reading id before it is set raises a RuntimeError, and setting twice raises. __repr__ surfaces the id once set, and create_api_and_tasks now seeds the root ControllerAPI path with [id] so sub-APIs become [id, sub]. Backwards compatible: when id is unset (existing single-controller launcher path), the API path remains empty and behaviour is unchanged. Part of #353. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure utility that derives an EPICS PV prefix from a controller path: the first segment (controller id) is used verbatim, while later segments are converted snake_case -> PascalCase. EPICS adopts this in #354 to replace the existing root-prefix-plus-pascalled-path approach for multi-controller IOCs. D2 module of #353. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reject controller ids that aren't safe in a REST URL path: empty or containing characters outside the loosest URL-safe set ([A-Za-z0-9_-]+). The error message includes the offending id so startup failures are unambiguous. Hookup into RestTransport.connect follows in the multi-controller routing slice. D3-REST module of #353. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every transport's connect() now takes list[ControllerAPI] uniformly. The existing single-controller transports (EPICS CA, EPICS PVA, GraphQL, Tango, REST) accept a list-of-one via a shared _expect_single helper and behave as before. FastCS.serve passes [self.controller_api]. True multi-controller support per transport will be wired in subsequent slices. This is a pure refactor: existing tests are updated to the new list-of-one call shape, no behaviour changes for any transport. Part of #353. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RestServer now accepts list[ControllerAPI] and adds attribute and
command routes for each. RestTransport hooks validate_rest_id into
connect() so illegal ids fail fast with a clear startup error. Existing
path-based routing already prefixes routes with controller_api.path[0],
so once Controller.set_id seeds the API path, REST URLs become
GET /{id}/{sub}/{attr} for free.
Two new tests in tests/test_multi_controller.py cover routing two
distinct ids in one process and rejecting an id with an illegal
character.
Part of #353.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`launch()` now accepts either a single Controller class or a list of classes; the generated `fastcs.yaml` schema replaces the top-level `controller:` key with a dict of `controllers:` keyed by id. Each value carries a `type:` discriminator (defaults to the class `__name__`, overridable via `type_name: ClassVar[str]`) and an optional `controller:` options block. Single-class registration may omit `type:` via a default. Duplicate ids are rejected at YAML load time by ruamel's safe loader. Wiring through `FastCS` for >1 controller lands in the next slice; for now multi-entry configs validate cleanly but the run command exits with a clear LaunchError pointing at the deferred work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (74)
📝 WalkthroughWalkthroughFastCS refactored for multi-controller operation: launch schema and CLI accept multiple controllers; FastCS and controllers manage per-controller APIs/lifecycle; transports accept lists of ControllerAPI and validate ids; EPICS PV prefix derivation and emission were added; Tango naming and many tests/docs/examples updated. ChangesMulti-Controller Support
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
- Coverage 91.24% 91.20% -0.05%
==========================================
Files 70 72 +2
Lines 2604 2875 +271
==========================================
+ Hits 2376 2622 +246
- Misses 228 253 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gilesknap
changed the title
gilesknap
commented
May 6, 2026
This was referenced May 6, 2026
FastCS.__init__ now accepts Controller | Sequence[Controller]; serve() loops initialise/post_initialise/connect/disconnect over every controller, builds list[ControllerAPI], and hands the full list to each transport.connect(). IPython context exposes parallel dicts (controllers, controller_apis) keyed by controller id (falling back to class name when no id is set), and the startup log line lists controller ids. fastcs.controller_api singular accessor is replaced with the controller_apis list. The temporary >1-controller LaunchError stub in launch._launch.run is removed; multi-entry configs are wired through FastCS directly. Single Controller direct construction continues to work via the union arg, so docs snippets are unchanged. A new end-to-end test in tests/test_multi_controller.py drives FastCS.serve with two id-tagged controllers and a RestTransport, asserts all four lifecycle hooks fire on each, and verifies /<id>/<attr> routing plus combined OpenAPI through TestClient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EpicsCATransport now hosts every configured controller in a single softioc, with each controller's id used verbatim as its PV prefix. EpicsCAIOC takes list[ControllerAPI] and loops the existing record/PVI/command builders per controller, deriving each prefix from pv_prefix_from_path(api.path) (the D2 utility introduced in #353). EpicsCATransport.connect drops _expect_single in favour of true multi-controller; validate_ca_id runs at connect time and rejects ids with illegal characters as well as setups whose longest derivable PV prefix already exceeds the 60-character EPICS limit. EpicsIOCOptions and its pv_prefix field are deleted. EpicsCAOptions and EpicsPVAOptions empty placeholders preserve epicsca: / epicspva: as fastcs.yaml discriminator keys (Pydantic union resolution is positional, so a unique field name per transport is still load-bearing). EpicsGUI no longer takes a separate prefix argument; PVs derive from the controller path. PVA temporarily continues via _expect_single but adopts pv_prefix_from_path so it gets the same id-based prefix and no longer needs EpicsIOCOptions; full PVA multi-root work lands in #355. tests/test_multi_controller.py grows a CA two-controllers-no-clash scenario and a CA id-validation fail-fast case. tests/example_softioc, tests/example_p4p_ioc, tests/benchmarking/controller, tests/conftest, test_initial_value, test_p4p, test_softioc, test_gui, test_pva_gui and the AssertableControllerAPI fixture all migrate to id-based naming (controllers set their id, transports take no prefix). Demo controller.yaml and both regenerated schema.json files reflect the new EpicsCAOptions / EpicsPVAOptions schemas and removal of pv_prefix. The 13 docs/snippets are exercised by tests/test_docs_snippets.py via runpy, so they migrate in this commit too to keep the suite green at every commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
multiple-transports.md and launch-framework.md still showed EpicsIOCOptions(pv_prefix=...) in their Python and YAML examples. Replace those with the id-based shape: controllers set their id (or inherit it from the YAML controllers: dict key), and EpicsCATransport / EpicsPVATransport take no prefix argument. The prose follows the API that landed in the previous commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EpicsPVATransport now hosts every configured controller in one p4p server, with each controller's id used verbatim as its PV prefix. P4PIOC takes list[ControllerAPI] and builds one StaticProvider per controller via the existing parse_attributes helper, so each controller gets an independent :PVI root with no super-parent (per the PRD). EpicsPVATransport.connect drops _expect_single in favour of true multi-controller; validate_pva_id runs at connect time and rejects ids with illegal characters as well as setups whose longest derivable PV prefix already exceeds the 60-character EPICS limit. validate_pva_id mirrors validate_ca_id and lives in transports/epics/ pva/util.py to keep id validation a per-transport concern. To share the 60-char constant without a cross-transport import, EPICS_MAX_NAME_LENGTH moves up from ca/util.py to epics/util.py; ca/util.py re-imports it so existing ca.util consumers (ca/ioc.py, test_softioc) are unaffected. tests/test_multi_controller.py grows a PVA two-controllers-distinct-PVI scenario (asserts each StaticProvider exposes its own root) and a PVA id-validation fail-fast case. test_pva_util.py mirrors test_ca_util.py's validator coverage. test_p4p.py::test_pvi_grouping shortens its UUID id to 8 hex chars so the deepest derived prefix (<id>:AdditionalChild:ChildChild) no longer trips the new 60-char check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sphinx is configured with `nitpicky = True` and `--fail-on-warning`, so single-backticks in the new P4PIOC docstring (`StaticProvider`, `:PVI`) were treated as :any: cross-references and failed to resolve. Switch to double-backticks so they're inline literals instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the GraphQL transport into the multi-controller foundation: - New `validate_graphql_id` enforces GraphQL `Name` syntax (the most restrictive of FastCS's transports — drives the lowest-common-denominator id-naming guidance for users mixing transports). - `GraphQLServer` now accepts `list[ControllerAPI]` and assembles a single combined schema with one top-level Query (and Mutation, where applicable) field per controller id. Sub-API type names are path-joined to keep two controllers' identically-named sub-controllers from clashing in the schema. - `GraphQLTransport.connect` validates ids fail-fast at startup. - `tests/test_multi_controller.py` gains a two-controller combined-schema scenario and a per-transport id-validation case. - The single-controller transport test is updated to namespace its queries under a controller id, matching the new contract; a latent bug in its `nest_mutation` helper (recursing through `nest_query`) is fixed in passing. - `docs/how-to/multiple-transports.md` adds a charset table and notes GraphQL as the lowest common denominator for cross-transport ids.
D4 of #351 lands as a single transport-level module. Both EPICS transports now invoke `emit_gui_files(controller_apis, options, builder)` once with the full controller list, replacing the per-controller loop that wrote everything to the same file. The module produces: - One screen/docs file per controller at `output_dir/{id}.{ext}`, preserving the order in which controllers were declared in `fastcs.yaml`. - An index file at the root of `output_dir` -- emitted even for a single controller, so the file layout is stable as the controller count changes. The GUI index uses pvi's `DLSFormatter` directly (rather than the convenience `format_index` wrapper) so that `DeviceRef.name` can be coerced to satisfy pvi's `PascalStr` constraint when controller ids legitimately start with a digit (e.g. UUID-flavoured test prefixes). The docs side mirrors the GUI shape with a minimal markdown emitter -- just enough to lift `EpicsDocs.create_docs` off its prior no-op stub. Knock-on schema/option changes: - `EpicsGUIOptions.output_path` (single file) becomes `output_dir` (directory); ditto `EpicsDocsOptions.path` -> `output_dir`. The per-controller filename is derived from the controller id. - The bundled demo, the 13 docs snippets, `tests/example_softioc.py`, the multi-transport how-to and the `launch-framework` how-to all migrate to the new field name. Both `schema.json` files are regenerated. - `EpicsGUI` loses its `create_gui()` file-writing entry point in favour of a smaller `build_device(title) -> Device` helper that the emission module composes per controller. - `tests/data/config.yaml` drops its `gui: {}` / `docs: {}` blocks -- they were schema-fixture noise that now leaks generated files into the repo CWD when the launcher tests exercise `connect()`. Tests: - New `tests/transports/epics/test_emission.py` (D4 unit tests): per-id files plus index for single and multi-controller cases, declaration order preservation, missing-output-dir creation, PVA builder propagation, and the digit-leading-id coercion. - `tests/transports/epics/ca/test_gui.py` gains a transport-level assertion that the index file is generated alongside per-controller files (per #358 acceptance criteria). - `tests/test_multi_controller.py` gains a CA scenario that drives `EpicsCATransport.connect` end-to-end and asserts both per-id and index files for GUI and docs land in their configured `output_dir`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gilesknap
force-pushed
the
multiple-controllers
branch
from
8b7cf38 to
fc8e710
Compare
The #358 commit migrated the docs/snippets EpicsGUIOptions(...) calls from a 3-line to a 1-line form via ruff format, shifting every line below `gui_options = ...` up by 2 in static05/06/10/14/15.py. The tutorial's `:emphasize-lines:` references for those snippets in docs/tutorials/static-drivers.md were left pointing at the old positions, which made `sphinx -W` fail with two "line number spec is out of range" warnings on static05 and static10. Update each affected range so the highlighted lines correspond to the same code as before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 6, 2026
…n guide
Bundled demo now hosts two TemperatureController instances (MAIN/AUX) on
distinct ports so the multi-controller feature is visible end-to-end. The
demo simulation grows a second TempController on port 25566 with its own
sink to drive the AUX controller; the main one stays on 25565.
Renames src/fastcs/demo/controller.yaml -> src/fastcs/demo/fastcs.yaml.
The launcher already takes the config path as a CLI argument, so nothing
hard-codes the new name. .vscode/launch.json updated; schema.json
unchanged (the dict-by-id form was already in place).
docs/how-to/launch-framework.md examples migrate to fastcs.yaml and gain
a "Hosting multiple controllers" section. New
docs/how-to/migrate-to-multi-controller.md covers the breaking-change
manual migration steps: file rename, controller: -> controllers:{id}
dict, EpicsIOCOptions.pv_prefix removal, type: discriminator with
single-class inference, GUI/docs output_dir rename.
Fixes #359
This was referenced May 6, 2026
The IOC publishes PVs using the controller id verbatim (see pv_prefix_from_path in transports/epics/util.py), but the per-controller DeviceRef in the GUI index was upper-casing the id when writing the pv attribute. For id="alpha" the per-controller .bob referenced alpha:Foo (matching the IOC) while the index .bob referenced ALPHA, which the IOC never publishes. Drop the .upper() so the index agrees with the IOC. Fixes #368 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The EPICS CA id validator accepts `[A-Za-z0-9_-]+`, so ids like `"___"` and `"-"` pass validation and reach `_coerce_pascal_name` in the GUI emission path. That helper delegates to `pvi.device.enforce_pascal_case`, which strips non-Pascal characters and unconditionally indexes `s[0]` on the result. When every character is stripped the index raises `IndexError`, blowing up GUI emission at `connect()` time with an opaque traceback. Pre-strip the id with the same regex pvi uses (`NON_PASCAL_CHARS_RE`) and raise `ValueError` with a message that names the offending id when the strip yields the empty string. Choosing fail-fast over an `"X"` fallback keeps the failure traceable: a silent fallback would generate nonsense GUI names that the user would have to reverse-engineer back to the bad id. Fixes #369 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restores the path-propagation flow to its pre-#360 shape: same `set_path` / `add_sub_controller(name).set_path(parent.path + [name])` / `_build_api(self._path)` machinery, with the single addition that the launcher seeds each root Controller's path via `set_path([entry.id])` once before `serve`. The "root of the path" moved from transport-config (`pv_prefix`, gone after #360) to controller-config (the YAML `id:`, seeded into `_path`). Drops `id` from Python entirely. `Controller._id`, `set_id()`, the `id` property and the `_id` repr branch are removed. `id` is now a YAML-label-only concept; `path` is the real Controller concept and the launcher is the only place that translates one to the other. User `Controller.__init__` is unchanged. YAML schema flip (consistent with the multi-controller direction of PR #360): `controllers:` becomes a list of entries, each carrying `id:` as a sibling of the existing `type:` discriminator. Duplicate ids are rejected at run time by `_instantiate_controllers` (replaces the dict mapping-key safety net that no longer exists). controllers: - id: MAIN type: TemperatureController ip_settings: { ip: localhost, port: 25565 } num_ramp_controllers: 4 The transport-side change made by #360 stays — `EpicsCATransport.connect` reads `pv_prefix_from_path(api.path)` instead of a configured `pv_prefix`. Multi-controller cardinality (also #360 territory) stays. `FastCS._context_key` and direct-construction call sites read `controller.path[0]` (with `IndexError` falling back to the class name); tests, examples and snippets call `set_path([id])` instead of `set_id(id)`. Migration guide, launch-framework guide, multiple-transports guide, demo, all docs snippets, and the YAML fixtures are updated to the list form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Default the fastcs.yaml `type:` discriminator to `<top-level-package>.<ClassName>` so Controllers shipped from independently-distributed packages (e.g. fastcs_eiger, fastcs_pmac) cannot collide on a short class name. An explicit `type_name: ClassVar[str]` on the class still wins verbatim with no prefix added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gilesknap
force-pushed
the
multiple-controllers
branch
from
5a0fe58 to
74aa39b
Compare
Contributor
Author
Contributor
Author
coretl
approved these changes
May 19, 2026
Contributor
coretl
left a comment
coretl
left a comment
There was a problem hiding this comment.
Discussed in person, Tango, GQL and REST are likely to need changes, but let's merge as is and handle those in another PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/rest/rest.py (1)
154-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBug: Command routes always use root controller's methods instead of current controller.
Line 158 references
root_controller_api.command_methodsbut should usecontroller_api.command_methodsto match the iteration variable. This causes all command routes to be registered from the root controller only, ignoring nested controller commands.Compare with
_add_attribute_api_routes(line 108) which correctly usescontroller_api.attributes.🐛 Proposed fix
def _add_command_api_routes(app: FastAPI, root_controller_api: ControllerAPI) -> None: for controller_api in root_controller_api.walk_api(): path = controller_api.path - for name, method in root_controller_api.command_methods.items(): + for name, method in controller_api.command_methods.items(): cmd_name = name.replace("_", "-") route = f"/{'/'.join(path)}/{cmd_name}" if path else cmd_name app.add_api_route( f"/{route}", _wrap_command(method.fn), methods=["PUT"], status_code=204, )🤖 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/rest/rest.py` around lines 154 - 166, In _add_command_api_routes, the loop is incorrectly iterating over root_controller_api.command_methods causing all command routes to come from the root controller; change the iteration to use controller_api.command_methods so each controller_api.walk_api() uses its own commands (i.e., in function _add_command_api_routes replace root_controller_api.command_methods with controller_api.command_methods when building cmd_name and calling app.add_api_route with _wrap_command(method.fn) and the existing PUT/204 options).
🧹 Nitpick comments (1)
src/fastcs/launch.py (1)
35-39: 💤 Low valueModule-level registry accumulates entries across test runs without cleanup.
_ENTRY_REGISTRYgrows each time_build_entry_modelis called. Since each call creates a new model class, tests callinglaunch()or_build_options_model()with the same controller classes will add new entries to the registry without clearing them. While this doesn't affect functionality (each entry maps to the correct controller), adding a pytest fixture to clear the registry between test runs would improve hygiene and prevent unbounded growth in long-running test suites.🤖 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/launch.py` around lines 35 - 39, The module-level _ENTRY_REGISTRY is never cleared and accumulates classes when _build_entry_model (and thus launch or _build_options_model) is called repeatedly; add a cleanup hook for tests by exposing a clear function or resetting the registry from tests: implement a small utility like clear_entry_registry() that sets _ENTRY_REGISTRY = {} (or clears it in-place) and call that from your pytest fixture (e.g., in a teardown/fixture that runs between tests) so repeated calls to _build_entry_model/launch/_build_options_model won't cause unbounded growth.
🤖 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/demo/schema.json`:
- Around line 3-7: The JSON schema objects (e.g., EpicsCAOptions and the various
transport wrapper and empty option object definitions added in this diff)
currently allow unknown keys; update each object schema by adding
"additionalProperties": false to harden validation so unknown/typo fields are
rejected. Locate each object definition introduced (names such as EpicsCAOptions
and the transport wrapper/option object entries referenced in the comment) and
add the additionalProperties flag at the same level as
"properties"/"type"/"title" for each schema definition.
In `@src/fastcs/transports/__init__.py`:
- Around line 4-10: The current __init__.py swallows ImportError for the whole
block so EpicsCAOptions and EpicsPVAOptions are not exported when
.epics.ca.transport fails; instead import and expose option classes separately
from the transport import: import EpicsCAOptions, EpicsDocsOptions,
EpicsGUIOptions, EpicsPVAOptions in their own try/except or top-level import so
they always get exported, and import EpicsCATransport in a separate try/except
that sets EpicsCATransport = None (or omits it) on failure; update references to
EpicsCATransport, EpicsCAOptions, and EpicsPVAOptions accordingly so config
consumers still receive the option classes even if the CA transport import
fails.
In `@src/fastcs/transports/epics/util.py`:
- Around line 35-39: The local variable named id shadows Python's builtin and
should be renamed (e.g., controller_id) to satisfy Ruff A001; update the
assignment from controller_api.path[0] to controller_id and replace subsequent
uses in the validation check using id_re.fullmatch(controller_id) and the
f-string (f"Controller id {controller_id!r} ...") while preserving
transport_label and the raise ValueError behavior.
In `@src/fastcs/transports/graphql/graphql.py`:
- Around line 35-44: The variable named id shadows the Python builtin; rename
the local variable assigned from controller_api.path[0] to controller_id and
update its usage in this block (the GraphQLAPI instantiation and the subsequent
calls to _wrap_as_field and create_type), e.g. replace references to id with
controller_id when creating f"{id}Query"/f"{id}Mutation" and when passing the
name into _wrap_as_field so GraphQLAPI(controller_api), sub_tree.queries,
sub_tree.mutations, queries.append(...), and mutations.append(...) use
controller_id instead of id.
In `@src/fastcs/transports/graphql/transport.py`:
- Around line 23-24: Check for an empty path before calling validate_graphql_id
to avoid IndexError: in the loop over controller_apis, guard access to
api.path[0] by first verifying api.path is truthy (non-empty) — e.g., only call
validate_graphql_id(api.path[0]) when api.path exists — mirroring the guard used
in RestTransport.connect(); update the loop that references controller_apis and
api.path to perform this check.
In `@src/fastcs/transports/tango/transport.py`:
- Around line 23-24: The loop over controller_apis in the Tango transport
accesses api.path[0] without guarding for empty paths (see the line assigning id
= api.path[0]); add a guard like "if api.path:" before attempting to read
path[0] or, if empty paths are invalid for this transport, raise a clear
ValueError with context (include controller/api identifier). Update the code in
the same loop (the controller_apis iteration) to either skip/handle empty
api.path entries or raise the explicit error so an IndexError cannot occur.
In `@tests/test_multi_controller.py`:
- Around line 366-367: Replace the fixed sleep after cancelling the background
task with an explicit await of the cancelled task to ensure deterministic
teardown: after calling task.cancel(), do try: await task except
asyncio.CancelledError: pass so the serve task (the variable task) has fully
finished/shutdown before the test proceeds, rather than relying on await
asyncio.sleep(0.1).
In `@tests/transports/epics/pva/test_pva_util.py`:
- Around line 7-15: Rename the test parameter named "id" to avoid shadowing the
built-in id() function (e.g., use "pva_id"); update both parametrized decorators
and the test function signatures and bodies (test_validate_pva_id_accepts_valid
and test_validate_pva_id_rejects_illegal_characters) so they pass
ControllerAPI(path=[pva_id]) to validate_pva_id instead of using "id".
In `@tests/transports/epics/test_emission.py`:
- Around line 34-37: Rename the parameter id in the helper function _api_with_id
to avoid shadowing the built-in id(); update the function signature and every
place the parameter is referenced (e.g., the call to controller.set_path([id])
and any callers) to use a non-built-in name like obj_id or resource_id; ensure
the function still constructs the controller via controller_class(), calls
controller.set_path([<new_name>]) and returns api, _, _ from
controller.create_api_and_tasks() with the new parameter name consistently
applied.
In `@tests/transports/tango/test_tango_util.py`:
- Around line 11-28: Rename the pytest parameter and local variable from id to
controller_id throughout this test file (update the parametrize decorators and
the test function signatures like test_accepts_valid_ids and
test_rejects_illegal_chars) to avoid shadowing the built-in, and change the
pytest.raises match argument to use re.escape(controller_id) instead of match=id
so literal strings like "bad.id" are matched literally; add an import for re at
top if missing and keep validate_tango_id referenced as-is.
---
Outside diff comments:
In `@src/fastcs/transports/rest/rest.py`:
- Around line 154-166: In _add_command_api_routes, the loop is incorrectly
iterating over root_controller_api.command_methods causing all command routes to
come from the root controller; change the iteration to use
controller_api.command_methods so each controller_api.walk_api() uses its own
commands (i.e., in function _add_command_api_routes replace
root_controller_api.command_methods with controller_api.command_methods when
building cmd_name and calling app.add_api_route with _wrap_command(method.fn)
and the existing PUT/204 options).
---
Nitpick comments:
In `@src/fastcs/launch.py`:
- Around line 35-39: The module-level _ENTRY_REGISTRY is never cleared and
accumulates classes when _build_entry_model (and thus launch or
_build_options_model) is called repeatedly; add a cleanup hook for tests by
exposing a clear function or resetting the registry from tests: implement a
small utility like clear_entry_registry() that sets _ENTRY_REGISTRY = {} (or
clears it in-place) and call that from your pytest fixture (e.g., in a
teardown/fixture that runs between tests) so repeated calls to
_build_entry_model/launch/_build_options_model won't cause unbounded growth.
🪄 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: 2349537a-a589-43e3-a926-064097409116
📒 Files selected for processing (74)
.vscode/launch.jsondocs/conf.pydocs/how-to/launch-framework.mddocs/how-to/migrate-to-multi-controller.mddocs/how-to/multiple-transports.mddocs/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.pydocs/tutorials/static-drivers.mdsrc/fastcs/control_system.pysrc/fastcs/controllers/controller.pysrc/fastcs/demo/controller.yamlsrc/fastcs/demo/fastcs.yamlsrc/fastcs/demo/schema.jsonsrc/fastcs/demo/simulation/temp_controller.yamlsrc/fastcs/launch.pysrc/fastcs/transports/__init__.pysrc/fastcs/transports/epics/__init__.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/ca/transport.pysrc/fastcs/transports/epics/ca/util.pysrc/fastcs/transports/epics/docs.pysrc/fastcs/transports/epics/emission.pysrc/fastcs/transports/epics/gui.pysrc/fastcs/transports/epics/options.pysrc/fastcs/transports/epics/pva/ioc.pysrc/fastcs/transports/epics/pva/transport.pysrc/fastcs/transports/epics/pva/util.pysrc/fastcs/transports/epics/util.pysrc/fastcs/transports/graphql/graphql.pysrc/fastcs/transports/graphql/transport.pysrc/fastcs/transports/graphql/util.pysrc/fastcs/transports/rest/rest.pysrc/fastcs/transports/rest/transport.pysrc/fastcs/transports/rest/util.pysrc/fastcs/transports/tango/dsr.pysrc/fastcs/transports/tango/options.pysrc/fastcs/transports/tango/transport.pysrc/fastcs/transports/tango/util.pysrc/fastcs/transports/transport.pytests/assertable_controller.pytests/benchmarking/controller.pytests/conftest.pytests/data/config.yamltests/data/schema.jsontests/example_p4p_ioc.pytests/example_softioc.pytests/test_control_system.pytests/test_launch.pytests/test_multi_controller.pytests/transports/epics/ca/test_ca_util.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/pva/test_pva_gui.pytests/transports/epics/pva/test_pva_util.pytests/transports/epics/test_emission.pytests/transports/epics/test_pv_prefix.pytests/transports/graphQL/test_graphql.pytests/transports/rest/test_id_validator.pytests/transports/rest/test_rest.pytests/transports/tango/test_dsr.pytests/transports/tango/test_tango_util.py
💤 Files with no reviewable changes (4)
- src/fastcs/demo/controller.yaml
- src/fastcs/transports/epics/docs.py
- src/fastcs/transports/tango/options.py
- docs/conf.py
Local p4p IOC shutdown could exceed the 5s pytest-timeout, tripping fixture teardown and leaving the subprocess (and its threads) alive, which cascaded into Tango test errors. Fall back to SIGKILL after 2s so teardown always completes promptly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_add_command_api_routes` walked the controller tree but then registered the root controller's commands at each visited path, instead of the per-controller commands. This added phantom `/<sub>/<root-cmd>` routes that aliased back to the root method, and left sub-controller commands unrouted entirely. Iterate `controller_api.command_methods` (matching `_add_attribute_api_routes`) so commands are registered against the controller that owns them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`RestTransport.connect` guards `api.path[0]` with `if api.path:` so that a controller without a seeded path is skipped instead of raising IndexError before validation runs. The GraphQL and Tango variants dereferenced `api.path[0]` unconditionally, leaving an inconsistent edge: a misconfigured controller would crash with a confusing IndexError in CA-or-PVA-style transports but be skipped cleanly in REST. Apply the same guard in both — GraphQL gates the `validate_graphql_id` call, Tango skips the entry entirely so the `seen` collision map only sees entries with a real id. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`EpicsCAOptions`, `EpicsDocsOptions`, `EpicsGUIOptions` and `EpicsPVAOptions` are pure-Python dataclasses with no EPICS runtime dependency, but they shared a `try: ... except ImportError: pass` block with `EpicsCATransport`. A missing softioc would suppress the ImportError and silently drop the option classes from the public `fastcs.transports` surface, breaking config-schema consumers that don't care whether the CA runtime is available. Import the option classes unconditionally and give each transport its own try/except. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five places newly introduced for multi-controller routing assigned to a local named `id`, shadowing Python's builtin and triggering Ruff A001/A002: - `src/fastcs/transports/epics/util.py` validate_epics_pv_id - `src/fastcs/transports/graphql/graphql.py` GraphQLServer query/mutation field naming - `tests/transports/epics/pva/test_pva_util.py` parametrize args - `tests/transports/epics/test_emission.py` `_api_with_id` helper - `tests/transports/tango/test_tango_util.py` parametrize args Rename the local in each to `name`. In `test_tango_util.py` also `re.escape` the match string so chars like `.` aren't interpreted as regex. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`test_fastcs_serves_two_controllers_end_to_end` cancelled the `fastcs.serve` task and then slept 100ms before asserting `controller.connected is False`. The sleep is non-deterministic — it relies on the cancelled task's cleanup fitting inside that window — and contributes flakiness if the event loop is busy. Await the task directly so the assertions only run once teardown has actually completed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new transport wrappers (`EpicsCATransport`, `EpicsPVATransport`,
`GraphQLTransport`, `RestTransport`, `TangoTransport`) and the new
empty option dataclasses (`EpicsCAOptions`, `EpicsPVAOptions`)
accepted unknown keys when Pydantic processed them from
`fastcs.yaml`, so a typo like `epicsca: { typo_key: oops }` slipped
through validation silently. The top-level `_build_options_model`
already sets `extra="forbid"`, but that doesn't propagate into nested
stdlib dataclasses.
Attach `__pydantic_config__ = ConfigDict(extra="forbid")` to each new
dataclass and regenerate `src/fastcs/demo/schema.json` and
`tests/data/schema.json`. Runtime validation now rejects unknown
sub-keys with a `Unexpected keyword argument` error, and the published
JSON schemas grow `additionalProperties: false` for the same nodes so
yaml-language-server flags typos in the editor too.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai I've pushed fixes for your comments above. Verify them. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll fetch my previous review comments on this PR to verify the fixes. 🏁 Script executed: Length of output: 115663 Let me check the current state of the flagged files to verify the fixes. 🏁 Script executed: Length of output: 7056 🏁 Script executed: Length of output: 1851 🏁 Script executed: Length of output: 3070 🏁 Script executed: Length of output: 543
✅ All critical/actionable fixes confirmed:
controller_id = api.path[0](with corresponding replacements of ℹ️ 🐇 ～(∎ω∎)～ [resolve] |
The earlier `id` → `name` rename pass missed `TangoTransport.connect`'s `id = api.path[0]` assignment, which still trips Ruff A001. CodeRabbit's follow-up review flagged it. Rename to `name` (not `controller_id`) to match the convention adopted in `graphql/graphql.py`, `epics/util.py` and the renamed test parameters from the earlier pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For consistency with the rest of the codebase — every other place that reads the first path segment as the controller's id now calls that local `name` (after the Ruff A001 pass on `epics/util.py`, `graphql/graphql.py` and `tango/transport.py`). Pure rename in `_coerce_pascal_name`, `emit_gui_files`, `emit_docs_files` and `_render_index_md`; no behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai final review before merge. |
|
✅ Actions performedFull review triggered. |
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.
Fixes #353
Fixes #354
Fixes #355
Fixes #356
Fixes #357
Fixes #358
Fixes #359
Foundation slice (#353) wires multiple top-level Controllers end-to-end through REST. The EPICS CA slice (#354) layers the first transport-side multi-root on top. The EPICS PVA slice (#355) layers the second transport-side multi-root on top. The GraphQL slice (#356) layers the third — one combined schema with id-keyed top-level Query fields. The Tango slice (#357) layers the fourth — one Tango Device Server hosts N devices, one per controller, with the id forming the leading device-name segment. The GUI/docs slice (#358) layers the fifth — per-id GUI/docs files plus an always-emitted index file. The demo+rename slice (#359) lands the user-facing surface — bundled demo hosts two controllers, the demo config is renamed
controller.yaml→fastcs.yaml, and a manual migration guide is added. All seven are on this branch, so each commit below names the slice it serves; commits within a slice are independent sub-modules and are green individually. Two further commits added during review tighten the YAML shape — flat per-id options with a requiredtype:discriminator — and are folded in here so the migration guide describes the final shape in one place. A tail commit (c8adfee7) further refines the shape:controllers:is a list of entries (each carrying its ownid:), andidis gone from Python entirely — the launcher seeds each root Controller's path viaset_path([entry.id])once beforeserve.Fixes #353 — multi-controller foundation (REST tracer)
5bb366cc0d4a3296893487db6dd956de0b2e1985011bb683d4ff267eFixes #354 — EPICS CA multi-root softioc with id-based PV prefix
c8adb33bc1b95a29Fixes #355 — EPICS PVA multi-root with N PVI roots
439560505317473fFixes #356 — GraphQL combined schema with id-keyed top-level Query fields
e5217785Fixes #358 — GUI/docs emission: per-id files plus index file
fc8e710bf2c7cef9Fixes #359 — Demo, migration guide, and config file rename to fastcs.yaml
f6600bc9Fixes #357 — Tango multi-device with id in device name
207d68d8Refinements (in addition to the original PRD)
Tighten the multi-controller YAML shape introduced above: each entry under
controllers:now exposes the controller's options fields directly as siblings of a requiredtype:discriminator, instead of a nestedcontroller:block. Pydantic's discriminated union does the dispatch and per-class validation in one pass; consumers reading the published JSON schema can identify each entry's class without knowing howlaunch()was called.4de23e6b439f2fe9type:discriminator mandatory on every controllers entry7ab10feeFixes after code review
A second pass after the initial review surfaced eleven follow-up issues (#361–#371). Each was triaged in parallel and fixed on its own branch; the commits below land them on
multiple-controllers. #362 was decision-only and resolved as already-implemented by commit439f2fe9above (option 2:type:discriminator mandatory in all modes).c8be8d69142e4e8dd341fff82cf219bcf055141004d36b2cc87711ef6907741db284afe54853d0eb6db26ccfTail refinement: list-form
controllers:, no PythonidA final tail commit restores the pre-#360 path-propagation flow and removes the transient
id-as-Python-concept that the PR introduced.controllers:becomes a list of entries (each carrying its ownid:sibling oftype:); the launcher seeds each root Controller's_pathviaset_path([entry.id])once beforeserve, andidis gone from Python entirely (noController.id, noset_id(), no_idattribute). UserController.__init__is unchanged from the rest of this PR — only the launcher and direct-construction call sites changed. Duplicate ids in the list are rejected at run time by_instantiate_controllers(replacing the dict-mapping safety net that no longer exists). Earlier review commit #22 (d341fff8, "Call set_id on controllers in tutorial snippets") is superseded by this tail; the snippets now callset_path(["DEMO"]).c8adfee7Final YAML shape per entry:
How to review
Recommend commit-by-commit — each commit's body explains its sub-module and the tests that cover it, and the suite is green at every commit.
(GK: yeah - or maybe just go through the tutorial!)
Suggested follow-up: real-world validation via fastcs-catio
A good shake-down for this branch is to bump
fastcs-catioto a build of this version and run it against real hardware:controllers:YAML with per-entryid:, multi-classlaunch(),controller_apisplural accessor, launcher-seededController._path) reads sensibly in a non-trivial app.Feed the resulting diff and any rough edges back into a new docs page "How to migrate to fastcs 0.11.0" — at minimum covering the YAML schema change (
controller:→ list-formcontrollers:withid:andtype:per entry), the Controllerpathsemantics (id-as-path-root seeded by the launcher; noController.idin user code), the unifiedTransport.connect(list[ControllerAPI])signature, and the EPICS CA prefix derivation (pv_prefix_from_path).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes & Changes
controller:→controllers:list (each requiresidandtype).fastcs.yamlandoutput_dir.