ssec-jhu / evolver-ng

Next gen eVolver controller for bioreactor project - wip
BSD 3-Clause "New" or "Revised" License
3 stars 0 forks source link

Auto-resolver for box hardware by name #85

Open amitschang opened 2 months ago

amitschang commented 2 months ago

Controllers and hardware can take hardware either by name (that setup on the box) or by a specific instance (or that from config), it would be convenient to be able to treat these effectively the same by forwarding calls on the by-name instance to the evolver hardware. For example:

HardwareByName(ConfigDescriptor)
hardware_spec = HardwareDriver | ConfigDescriptor | HardwareByName

after bootstrapping, hardware by name should be accessible equivalently to hardware directly passed in. In this way a controller could treat all these equivalent without the need to switch like:

@property
def the_sensor(self):
    self.evolver.hardware[self.sensor] if isinstance(self.sensor, str) else self.sensor
jamienoss commented 2 months ago

I think commoning up this functionality like this will only embed the Evolver class even deeper and deeper into the SDK, which isn't adequate interface separation for clean encapsulation.

As I've noted before, I don't think it's a good idea to pass the evolver instance around at all like this but instead at the creation interface just pass in the the hardware needed from the evolver instance. Not to say, that there may not be another reason to pass a manager around, but this doesn't seem like the correct one to me.

BTW The only reason I added the above switch in #73 is to code against existing implementation that we mutually knowingly haven't completely ironed out, we haven't taken pause to address my voiced concern above and that PR wasn't the place to do so.

The dynamic reconfiguration that this issue gains feels unnecessary at the SDK layer, but would be better suited at the application layer. There it would be a cool feature. However, it would also be good to discuss the necessity for keeping controller instances alive when the hardware that they control changes? Why not just stick with a simple creation pattern and discard the old and re-instantiate new ones, doing so at the outer most layer, i.e., the application layer?

amitschang commented 2 months ago

it would probably be fine if HardwareByName returned the actual resolved instance - there would just have to be bookkeeping somewhere for dependency injection.

Actually the point of the issue is the ergonomics, not the implementation, so its really about being able to spec by name in config that then can be used as if you passed in the required hardware. I'll change the title

jamienoss commented 2 months ago

so its really about being able to spec by name in config that then can be used as if you passed in the required hardware.

Isn't this just the Evolver as is though?

Evolver.hardware is a dict of names to hardware instances functioning as the resolver you describe, no?

The configs for the hardware need to be specified somewhere anyhow, so we have that covered. BaseInterface.Config already has a name field which can be specified in ConfigDescriptor.config so it's not like adding a name field to the ConfigDescriptor model or a derived class of the same thing is something we don't already have, right?

amitschang commented 2 months ago

I don't think we have this resolved, in the controllers/drivers the aim is to avoid having to deal with what kind of object represents the hardware (e.g. a string which references by name, or a passed in driver). Right now each controller for each config element would have to check.

jamienoss commented 2 months ago

in the controllers/drivers the aim is to avoid having to deal with what kind of object represents the hardware (e.g. a string which references by name, or a passed in driver).

Ok so we have the resolver, it is Evolver.hardware, but we just don't have a hookup to do this automatically? Is that a fair summary or are we thinking about something more significant than this?

By "automatically", I mean as we do similarly for driver instances andConfigDescriptor as was handled in #53 - where upon instantiation ConfigDescriptor are converted to their driver instances. We'd want to add an additional condition to allow for names and then BaseInterface.create() and/or ConfigDescriptor.create() does a lookup in Evolver.hardware and returns those instances, right, something along those lines?

amitschang commented 2 months ago

seems a fair summary, and yes something along those lines. I'm sure there are a number of possible implementations. End of the day we would be able to:

class MyControl(Controller):
    class Config(BaseConfig):
        sensor: HardwareSpec

    def control(self):
        self.sensor.get()
        ...

where HardwareSpec would effectively be like: HardwareDriver | ConfigDescriptor | ThingThatGoesByName

jamienoss commented 2 months ago

This is effectively the issue for https://github.com/ssec-jhu/evolver-ng/pull/55/files#r1589373483 and related comments.

When instantiating from Evolver this may not be something that we can entirely avoid, as attempting to address in #55.

The crux is that there's a config collision between Evolver.Config.hardware and Evolver.Config.controllers.config.

I don't think this functionality should go in the base classes so that an evolver instance isn't required there, however, resolution is needed within Evolver, either just at the instantiation layer or also at the config layer, and possibly also Controller.Config.

jamienoss commented 2 months ago

At present, and by example, evolver.controllers.standard.Chemostat.Config.od_sensor allows for HardwareDriver | ConfigDescriptor | str. I think a clean solution would be to only allow str names for the sake of Evolver to parse, i.e., this is disallowed if instantiating a controller directly (which admittedly makes for a less than clean config). That way, this functionality can simply go in either Evolver.__init__ or Evolver.post_init_vars(). This results in clean and separate interfaces such that Evolver doesn't unecessarily bleed into the other interfaces.