epics-containers / ibek

IOC Builder for EPICS and Kubernetes
https://epics-containers.github.io/ibek
Apache License 2.0
10 stars 4 forks source link

Add collections of Entities inside of Definitions #216

Closed gilesknap closed 1 month ago

gilesknap commented 1 month ago

This implements the requirements for Tetramm as discussed at length last week.

It came out rather well after several iterations and the naming might even be OK for Gary!

I did not end up needing two kinds of thing. Instead we still have Support YAML containing a List of Definition. Definition is as before but also contains a List of SubEntity called subentities. This has the bonus that you can specify both startup script and database for your definition as well as SubEntity's. (and therefore we can support the NDTimeSeries within NDStats case - making startup script/db for NDStats and having NDTimeSeries as a SubEntity)

SubEntity is a dictionary containing type and anything else (so no schema checking at edit time other than must have type).

When pydantic deserialises an ioc.yaml we end up with a list of entities as before but each entity may also have a list of subentity. I then manually in code get pydantic to convert all of the SubEntity to the correct Entity class and all the nice field and model validators provide checking of that process and report errors very nicely.

There is major refactoring in this PR, in particular a new IocFactory class contains all of the model management that was scattered as functions in a couple of modules before. Also, larger modules have been broken into approx class per module.

So a lot of code to look at but the key bit that is functionally different is the processing of SubEntities after the IOC Instance has been deserialised. See here: https://github.com/epics-containers/ibek/blob/f941f3c9958da7e3bbb5b1f92a228b73291916d7/src/ibek/ioc_factory.py#L83-L109

And here are the examples that use the new feature

At the time of writing the sub entities in quadem.ibek.support.yaml fully list all their arguments - but next I'll look at using shared references so that only the unique arguments need to be listed for each.

gilesknap commented 1 month ago

@JamesOHeaDLS I'd be interested in your views on this. This is an attempt to make a nice generic way of creating additional ibek objects from within an ibek object.

The advantage of this over our Jinja loop approach is that its less error prone and it brings in all the PVI screens for each of the plugins.

Don't worry about the python - just take a look at https://github.com/epics-containers/ibek/blob/add-collection-tests2/tests/samples/support/quadem.ibek.support.yaml

gilesknap commented 1 month ago

So for use in SubEntities, yaml anchors and aliases work beautifully. See below.

Note that schema checking in the editor understands these dictionary merges so this will probably work even better for defining things in ioc.yaml like multiple motors for a controller.

I have added a 'scratch' field to Definition called 'shared' to place things you want to use repeatedly. It is not strictly necessary as the first user could declare everything and the anchor. But I thought this was prettier because all the stats and arrays declarations line up nicely this way. Other than that the schema and samples are unchanged - this is just a terser way of describing the same things in a DRY fashion.

Another excuse for the choice of YAML!!


    shared:
      - &stats { type: ADCore.NDStats, P: "{{DEVICE.P}}", NCHANS: "{{STAT_NCHAN}}", XSIZE: "{{STAT_XSIZE}}", YSIZE: 0, HIST_SIZE: "{{HIST_SIZE}}", NDARRAY_PORT: "{{DEVICE}}", ENABLED: 1 }
      - &arrays { type: ADCore.NDStdArrays, P: "{{DEVICE.P}}", NDARRAY_PORT: "{{DEVICE}}", ENABLED: 1, TYPE: "Float64", FTVL: "DOUBLE", NELEMENTS: "{{STAT_XSIZE}}" }

    sub_entities:
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.I1", R: Cur1 }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.I2", R: Cur2 }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.I3", R: Cur3 }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.I4", R: Cur4 }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.SumX", R: SumX }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.SumY", R: SumY }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.SumAll", R: SumAll }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.DiffX", R: DiffX }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.DiffY", R: DiffY }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.PosX", R: PosX }
      - { <<: *stats, PORT: "{{PORTPREFIX}}.STATS.PosY", R: PosY }

      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.Arr1", R: Arr1 }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.Arr2", R: Arr2 }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.Arr3", R: Arr3 }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.Arr4", R: Arr4 }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.SumX", R: SumX }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.SumY", R: SumY }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.SumAll", R: SumAll }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.DiffX", R: DiffX }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.DiffY", R: DiffY }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.PosX", R: PosX }
      - { <<: *arrays, PORT: "{{PORTPREFIX}}.ARRAYS.PosY", R: PosY }
gilesknap commented 1 month ago

One nice little addition ...

Now we support recursive SubEntities. For example:

This is now supported by adjusting _process_collections into a recursive form like this: https://github.com/epics-containers/ibek/blob/7e882f87f152b5644e2ab8ec3556315439aae242/src/ibek/ioc_factory.py#L83-L113)

JamesOHeaDLS commented 1 month ago

Looks good to me! Easier to read and work out what is happening than a jinja loop

gilesknap commented 1 month ago

OK it took me ages to sort our the syntax for this. But, for the record my minimal reproduction example fails to reproduce the problem:

import json
from typing import Annotated, Literal, Sequence, Union

from pydantic import BaseModel, Field, create_model
from pydantic.fields import FieldInfo

from ibek.test2 import make_cls_3

class Entity(BaseModel):
    type: str = Field("the type of this entity")
    other_thing: int

def make_cls(base: BaseModel):
    entity_cls1 = create_model(
        "myclass1",
        type=(Literal["one"], FieldInfo(description="first type")),
        __base__=Entity,
    )

    entity_cls2 = create_model(
        "myclass2",
        type=(Literal["two"], FieldInfo(description="second type")),
        __base__=Entity,
    )
    return [entity_cls1, entity_cls2]

classes = make_cls(BaseModel)
classes.append(make_cls_3(Entity))

entity_union = Union[tuple(classes)]
discriminated = Annotated[entity_union, Field(discriminator="type")]  # type: ignore

class NewIOC(BaseModel):
    entities: Sequence[discriminated] = Field(
        description="List of entities this IOC instantiates"
    )

schema = NewIOC.model_json_schema()
print(json.dumps(schema, indent=2))
for i in classes:
    print(i)

prints out

....

  ],
  "title": "NewIOC",
  "type": "object"
}
<class '__main__.myclass1'>
<class '__main__.myclass2'>
<class 'ibek.test2.myclass1'>

So it does exhibit the module prefixes in the class names but it is OK printing the schema of the resulting IOC.

gilesknap commented 1 month ago

OK I figure this is the crux of it. When Entity creation and IOC creation are in the same module. The Untion look like:

typing.Union[ibek.ioc_factory.motorSim_simMotorController, ibek.ioc_factory.motorSim_simMotorAxis]

When they are in separate modules it looks like this:

typing.Union[ForwardRef('motorSim.simMotorController'), ForwardRef('motorSim.simMotorAxis')]

So that's not right but why is it like that?

gilesknap commented 1 month ago

Update: it was my bad. There is no anomalous behaviour when combining dynamic Pydantic models between modules at all. It was just me. I was passing a dictionary of classname->class into the the discriminated union. The tuple of that becomes a list of the keys - which are just class names and thus forward refs. Probably the worst mistake I could have made in order to fully confuse myself. All fixed now.