DiamondLightSource / tickit

Event-based hardware simulation framework
Apache License 2.0
7 stars 0 forks source link

Simulation wrapper #142

Closed abbiemery closed 1 year ago

abbiemery commented 1 year ago

These changes allow the simulation to be constructed in a declarative fashion, choosing either "all" , "scheduler", or "components". The behaviour of "all " and "Scheduler" remains the same. The "component" argument is now "components", which runs a combination of one or more components from a config file to run separately from the scheduler.

abbiemery commented 1 year ago

This came from the talks over the zebra work the idea of a wrapper/runner was posed to make the interface a bit cleaner and more obvious.

codecov[bot] commented 1 year ago

Codecov Report

Merging #142 (6267b04) into master (ad1ca51) will increase coverage by 0.16%. The diff coverage is 98.33%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   94.56%   94.72%   +0.16%     
==========================================
  Files          44       45       +1     
  Lines        1269     1309      +40     
==========================================
+ Hits         1200     1240      +40     
  Misses         69       69              
Impacted Files Coverage Δ
src/tickit/core/simulation.py 97.87% <97.87%> (ø)
src/tickit/cli.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

abbiemery commented 1 year ago

The only thing that happens now which I am not happy with is if you run components amp sink where amp is in the config file but sink is not. then it will only run the amp, but it won't tell you that you tried to run something it doesn't have. I wasn't sure where/how to put this validation in without re-bloating the builder again.

tpoliaw commented 1 year ago

Based on the state at 94af7f28405c would something like this work?

--- a/src/tickit/core/simulation.py
+++ b/src/tickit/core/simulation.py
@@ -87,7 +87,6 @@ def build_simulation(
     """
     scheduler = None
     components = None
-    components_to_run = components_to_run or set()

     configs = read_configs(config_path)
     inverse_wiring = InverseWiring.from_component_configs(configs)
@@ -95,12 +94,15 @@ def build_simulation(
     if include_schedulers:
         scheduler = MasterScheduler(inverse_wiring, *get_interface(backend))
     if include_components:
-        components = {config.name: config() for config in configs}
-        run_all = not components_to_run
+        available_components = {config.name for config in configs}
+        if components_to_run is None:
+           components_to_run = available_components
+        if any(name not in available_components for name in components_to_run):
+            raise ValueError("Requested components are not available in configuration")
         components = {
             config.name: config()
             for config in configs
-            if run_all or config.name in components_to_run
+            if config.name in components_to_run
         }

     return TickitSimulation(backend, scheduler, components
abbiemery commented 1 year ago

@tpoliaw I fixed all the things you raised, so we should be good enough to go now.

abbiemery commented 1 year ago

Looks good. If the type checking is happy with the current state, I'm not fussed enough to block this PR.

The highest praise I could hope for.