Skip to content

Conversation

@marcelldls
Copy link
Contributor

Now that im working on a device with both an epics and tango option, im finding the current public interface a bit clumsy

Particularly:

  • The dot paths are awkward
  • If you give Epics/Tango options, you have already enough information to describe the backend needed but you still have to import the specific backend
  • If each backend will init with args like "pv_prefix" then the interfaces will never be consistent
  • Backend.run calls self._run() - Therefore EpicsIOCOptions are never passed (https://github.com/DiamondLightSource/FastCS/blob/main/src/fastcs/backends/epics/backend.py#L22)
  • Its not explicit what each backend should implement
  • Backend specific imports are being imported regardless and there are interdependencies (tango devices get INFO: PVXS QSRV2 is loaded, permitted, and ENABLED) .

To think about:

  • We might consider adding [project.optional-dependencies] to install only what is needed in the manner done by the Polars library pip install 'fastcs[tango]'
  • What is "asyncio backend"?
  • Fix tests when happy

This is opinionated so heres a draft

Im proposing to go from:

from fastcs.backends.epics.backend import EpicsBackend, EpicsGUIOptions
from fastcs.connections.serial_connection import SerialConnectionSettings

backend = EpicsBackend(get_controller(port, baud), pv_prefix)
backend.create_gui(EpicsGUIOptions(output_path / "index.bob"))
backend.run()

To:

from fastcs import FastCS
from fastcs.backends import EpicsGUIOptions, EpicsIOCOptions, EpicsOptions
from fastcs.connections import SerialConnectionSettings

options = EpicsOptions(
    ioc=EpicsIOCOptions(pv_prefix=pv_prefix),
    gui=EpicsGUIOptions(output_path / "index.bob"),
)

fast_cs = FastCS(get_controller(port, baud), options)
fast_cs.create_gui()
fast_cs.run()

A tango instance will then look like:

from fastcs.backends import TangoDSROptions, TangoOptions

options = TangoOptions(
    TangoDSROptions(
        ...
    )
)

fast_cs = FastCS(get_controller(port, baud), options)
fast_cs.run()

Heres my example device DiamondLightSource/thorlabs-mff-fastcs#8

@marcelldls marcelldls requested a review from GDYendell October 29, 2024 11:00
@marcelldls marcelldls marked this pull request as draft October 29, 2024 11:00
@marcelldls
Copy link
Contributor Author

marcelldls commented Nov 4, 2024

Considering that the Epics/Tango control system support is intended to give the external interface for the controller rather than carry out the "business logic" - the name backends does not feel appropriate? Tom has suggested transport

@marcelldls
Copy link
Contributor Author

marcelldls commented Nov 12, 2024

I believe in the same topic there is merit in:

  • A launcher that creates a consistent way to start fastcs applications. Especially so we can share a simple reusable helm chart. A discussion with Tom and Giles has resulted in a vision of fastcs devices being able to load a config.yaml or helm values.yaml with an embedded config.yaml
  • Exposing a demo device

@codecov
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 92.46575% with 22 lines in your changes missing coverage. Please review.

Project coverage is 87.52%. Comparing base (cec9fda) to head (a238937).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fastcs/launch.py 93.90% 5 Missing ⚠️
src/fastcs/transport/adapter.py 72.72% 3 Missing ⚠️
src/fastcs/transport/epics/docs.py 62.50% 3 Missing ⚠️
src/fastcs/transport/rest/adapter.py 78.57% 3 Missing ⚠️
src/fastcs/transport/tango/adapter.py 78.57% 3 Missing ⚠️
src/fastcs/transport/epics/adapter.py 90.00% 2 Missing ⚠️
src/fastcs/controller.py 97.05% 1 Missing ⚠️
src/fastcs/transport/epics/gui.py 80.00% 1 Missing ⚠️
src/fastcs/transport/rest/rest.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   86.74%   87.52%   +0.77%     
==========================================
  Files          25       28       +3     
  Lines        1056     1162     +106     
==========================================
+ Hits          916     1017     +101     
- Misses        140      145       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GDYendell
Copy link
Contributor

I broadly agree with these changes.

It is interesting that you have repurposed the Backend class to just be a wrapper of the Controller. I think we could build on this to also merge Mapping and SingleMapping into Backend. I have wanted to get rid of these classes somehow for a while now because it is pretty opaque what they do and we couldn't come up with good names for them. This could be in a separate PR.

@GDYendell
Copy link
Contributor

What is "asyncio backend"?

This was also difficult to name. It is basically no backend, just an interactive python shell where you can manually poke things.

@marcelldls
Copy link
Contributor Author

I broadly agree with these changes.

It is interesting that you have repurposed the Backend class to just be a wrapper of the Controller. I think we could build on this to also merge Mapping and SingleMapping into Backend. I have wanted to get rid of these classes somehow for a while now because it is pretty opaque what they do and we couldn't come up with good names for them. This could be in a separate PR.

Thanks Gary. This is indeed still a bit coarse. Hmm that merge is a very interesting idea - I like it.

There is another thing to consider which is that Tom has proposed that we should produce a schema for each device (controller config and transport config)

@marcelldls
Copy link
Contributor Author

marcelldls commented Nov 14, 2024

What is "asyncio backend"?

This was also difficult to name. It is basically no backend, just an interactive python shell where you can manually poke things.

I see! The soft ioc dependency confused me, I would hope to try break interdependency. Tom has supported both the idea of dropping the interactive terminal and just using a debugger but equally also the idea of an interactive terminal that works for all transport options to just poke variables. Im not sure we will be able to do an interactive terminal for Tango (or webserver) without solving the higher problem of running multiple transports at once because I run the server in the main thread as I dont get an option to pass an event loop.

@marcelldls
Copy link
Contributor Author

@GDYendell
Additionally I am including a fastcs launcher (as proposed by @coretl), which can be used as a common entrypoint by using typehinting to discover the expected inputs

A minimal example:

from dataclasses import dataclass

from fastcs import launch
from fastcs.attributes import AttrRW
from fastcs.controller import Controller
from fastcs.datatypes import Bool


@dataclass
class TestControllerConfig:
    name: str


class TestController(Controller):
    read_write_bool: AttrRW = AttrRW(Bool())

    def startup(self, config: TestControllerConfig):
        self._config = config

    async def connect(self):
        print(f"Hello {self._config.name}")


if __name__ == "__main__":
    launch(TestController)

When executed:

$ python example.py --help
                                                                                                                                                             
 Usage: example.py [OPTIONS] COMMAND [ARGS]...                                                                                                               
                                                                                                                                                             
╭─ Options ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --install-completion          Install completion for the current shell.                                                                                   │
│ --show-completion             Show completion for the current shell, to copy it or customize the installation.                                            │
│ --help                        Show this message and exit.                                                                                                 │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Commands ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ schema   Produce json schema for a TestController                                                                                                         │
│ run      Start up a TestController                                                                                                                        │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
$ python example.py run --help
                                                                                                                                                                     
 Usage: example.py run [OPTIONS] CONFIG                                                                                                                              
                                                                                                                                                                     
 Start up a TestController                                                                                                                                           
                                                                                                                                                                     
╭─ Arguments ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ *    config      PATH  A yaml file matching the TestController schema [default: None] [required]                                                                  │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Options ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --help          Show this message and exit.                                                                                                                       │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Example of the config which conforms to the expected schema (to do: handling of a k8s values.yaml)

# yaml-language-server: $schema=schema.yaml
controller:
  name: World
transport:
  ioc:
    terminal: true
    pv_prefix: MY-DEVICE-PREFIX
  gui:
    output_path: /home/esq51579/WIP/thorlabs-mff-fastcs/src/thorlabs_mff_fastcs/output.bob
    file_format: .bob
    title: Simple Device

@marcelldls marcelldls force-pushed the public-interface branch 2 times, most recently from 4eb422a to b099d41 Compare November 26, 2024 19:47
Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

Comments on first commit

@GDYendell
Copy link
Contributor

GDYendell commented Nov 27, 2024

I am broadly in agreement with the changes so far, but this adds more use of "mapping" within controller, which I don't like.

I think we should have a new class ControllerAPI that will replace SingleMapping, but as a tree (i.e. each node has its path and children: list[ControllerAPI]) and have some additional logic. Then we can have a concept of walking the controller API to create the transport layer and the UIs. Because when a Controller is first instantiated it is partially initialised (attributes can be added during initialise) I think it would make sense for the Backend to convert it to ControllerAPI that is static and then pass that to the transport layer, which should simplify the logic there. In fact, maybe initialise should return the ControllerAPI?

Also I think this is beginning to overlap with #87 so we should keep that in mind.

@marcelldls marcelldls marked this pull request as ready for review November 28, 2024 13:34
@marcelldls
Copy link
Contributor Author

@GDYendell Ready

Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

Happy to squash merge because it is not clear what commits this could be broken into that would still be functional in isolation, but could you write a good summary of all the things that are being added/changed here when you do the merge?

@GDYendell GDYendell changed the title Proposal API Refactor developer API with transports and app launcher Nov 29, 2024
@marcelldls
Copy link
Contributor Author

Agreed. Thanks for all the review comments @GDYendell - I think its looking neat now. Can do

@marcelldls marcelldls merged commit 7d602ce into main Nov 29, 2024
17 checks passed
@marcelldls marcelldls deleted the public-interface branch November 29, 2024 15:55
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.

3 participants