Multi-controller foundation: Controller.id, multi-class launch, unified transports, REST per-id (#353)#360
Draft
Multi-controller foundation: Controller.id, multi-class launch, unified transports, REST per-id (#353)#360
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>
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>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 91.24% 91.57% +0.33%
==========================================
Files 70 72 +2
Lines 2604 2743 +139
==========================================
+ Hits 2376 2512 +136
- Misses 228 231 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gilesknap
changed the title
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.
gilesknap
commented
May 6, 2026
| num_ramp_controllers: 4 | ||
| controllers: | ||
| TEMPERATURE: | ||
| controller: |
Contributor
Author
There was a problem hiding this comment.
@coretl I'd like your opinion on this - I think this is the right shape but it does not match with what transports do.
the schema is
controllers:
id_str_as_key:
type: class of controller (may be omitted if there is only one class)
controller:
init parameters for the controller classalso - it looks a little odd to me for the simple case of single controller (especially with no init args), like the above.
See discussion with Claude below.
The controller: key sits as a sibling to type: — it's the namespace that holds whatever Pydantic options model the Controller's init declared. So a
config entry has two halves:
- type: — FastCS metadata (which class to instantiate)
- controller: — the author's own options model, plugged in intact as the type they declared
The real tension I see: transports use discriminator-as-key (graphql: { host: ... }), but controllers use type: + controller:. There's an inconsistency in
the YAML's mental model — one discriminated-union shape would be nicer. The discriminator-as-key version of controllers would be:
controllers:
TEMPERATURE:
TempController: # discriminator IS the key
ip_settings: { ip: localhost, port: 25565 }
num_ramp_controllers: 4
Cleaner, parallel to transports, removes the "redundant layer" feel. Cost: harder to add per-id FastCS metadata later without re-introducing a wrapper, and
harder to default type: away in the single-class case (today _build_entry_model makes type literal-with-default so controllers: { TEMPERATURE: { controller:
{...} } } works without naming the class).
My recommendation: keep the current shape. The visual redundancy is a one-line learning curve; the architectural property — the author's options class lives
in its own namespace, untouched by FastCS metadata — pays off the first time we add an entry-level field. But if you'd rather optimise for "the YAML reads
cleanly first time" and live with the constraints, discriminator-as-key is a defensible call and worth doing now before slice #357 lands and lots of
docs/tests reference the shape.
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
1 participant
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
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. All four 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.
Fixes #353 — multi-controller foundation (REST tracer)
5bb366cc0d4a3296893487db6dd956de0b2e1985011bb6836d7ccfccFixes #354 — EPICS CA multi-root softioc with id-based PV prefix
da73eaa27e910632Fixes #355 — EPICS PVA multi-root with N PVI roots
504d5671cc24439aFixes #356 — GraphQL combined schema with id-keyed top-level Query fields
b8a79613Subsequent slices (#357 Tango, #358 GUI/docs, #359 demo+rename) layer on top.
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.
Checks for reviewer
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, multi-classlaunch(),controller_apisplural accessor,Controller.id-seeded paths) 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:→controllers:keyed by id,type:discriminator), theController.id/ path semantics, the unifiedTransport.connect(list[ControllerAPI])signature, and the EPICS CA prefix derivation (pv_prefix_from_path).🤖 Generated with Claude Code