Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add collections of Entities inside of Definitions #216

Merged
merged 24 commits into from
May 15, 2024

Conversation

gilesknap
Copy link
Member

@gilesknap gilesknap commented May 11, 2024

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:

def _process_collections(self, ioc_instance: IOC):
"""
Process all the collections in the IOC instance
"""
all_entities: List[Entity] = []
for entity in ioc_instance.entities:
definition = entity.__definition__
# add the parent standard entity - just add it to the list
all_entities.append(entity)
# add in SubEntities if any
for sub_entity in definition.sub_entities:
# find the Entity Class that the SubEntity represents
entity_cls = self._entity_classes[sub_entity.type]
# get the SubEntity arguments
sub_args_dict = sub_entity.model_dump()
# jinja render any references to parent Args in the SubEntity Args
for key, arg in sub_args_dict.items():
sub_args_dict[key] = UTILS.render(entity, arg)
# cast the SubEntity to its concrete Entity subclass
cast_entity = entity_cls(**sub_args_dict)
# add it to the IOCs entity list
all_entities.append(cast_entity)
ioc_instance.entities = all_entities

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
Copy link
Member Author

gilesknap commented May 11, 2024

@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
Copy link
Member Author

gilesknap commented May 12, 2024

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
Copy link
Member Author

gilesknap commented May 12, 2024

One nice little addition ...

Now we support recursive SubEntities.
For example:

  • quadEM.Plugins includes 11 NDStats and 11 NDStdArrays
  • but each NDStats includes a single NDTimeSeries

This is now supported by adjusting _process_collections into a recursive form like this:

def _process_collections(self, ioc_instance: IOC):
"""
Process all the sub entity collections in the IOC instance
"""
all_entities: List[Entity] = []
def scan_sub_entities(entity: Entity):
# recursive function to scan for SubEntities in an entity
definition = entity.__definition__
# add the parent standard entity
all_entities.append(entity)
# add in SubEntities if any
for sub_entity in definition.sub_entities:
# find the Entity Class that the SubEntity represents
entity_cls = self._entity_classes[sub_entity.type]
# get the SubEntity arguments
sub_args_dict = sub_entity.model_dump()
# jinja render any references to parent Args in the SubEntity Args
for key, arg in sub_args_dict.items():
sub_args_dict[key] = UTILS.render(entity, arg)
# cast the SubEntity to its concrete Entity subclass
cast_entity = entity_cls(**sub_args_dict)
# recursively scan the SubEntity for more SubEntities
scan_sub_entities(cast_entity)
for entity in ioc_instance.entities:
scan_sub_entities(entity)
ioc_instance.entities = all_entities
)

@JamesOHeaDLS
Copy link

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

src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/runtime_cmds/commands.py Show resolved Hide resolved
src/ibek/render.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
@gilesknap
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

gilesknap commented May 14, 2024

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.

This is a result of the PR review by Gary. Some
other minor review points are also addressed.

Fix up the rest of the tests and remaining PR
comments will be in the next commit.
src/ibek/entity_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
src/ibek/ioc_factory.py Outdated Show resolved Hide resolved
Copy link
Member

@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.

.

tests/conftest.py Outdated Show resolved Hide resolved
@gilesknap gilesknap merged commit 5477c59 into main May 15, 2024
12 checks passed
@GDYendell GDYendell deleted the add-collection-tests2 branch May 15, 2024 13:32
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.

None yet

3 participants