Adding schema registry, validator and generator for RF#279
Adding schema registry, validator and generator for RF#279TeresiaOlsson wants to merge 27 commits into
Conversation
|
@JeanLucPons and @gupichon while I work on rebasing on main and adding the final parts to be able to register the schema in the registry, can you maybe take a look at my suggestion for how to add the validation at object creation? I thought it will be too confusing for the users to have one decorator which registers the schema + dynamically generates it + adds validation at object creation since it's too many thing happening at once. And overwriting the constructor in a decorator didn't feel great. So instead I decided to try to add the validation at object creation using inheritance. There is now a metaclass What do you think about this? |
|
Adding an extra validation class is fine with me. |
|
I plan to also remove PYAMLCLASS. I just haven't dared to do it yet. Burnt by my last attempt I now try to change in as small steps as possible, run the tests, rebase based on current main in case Github says there is a merge conflict and then change the next small thing ;) Hopefully by the time the PR is finished that will mean an easier review. |
TeresiaOlsson
force-pushed
the
schema-registry-rf
branch
from
541509e to
e07b73a
Compare
obj = type(self)(
name=self.name,
cavities=self.cavities,
voltage=self.voltage_str,
phase=self.phase_str,
harmonic=self.harmonic,
distribution=self.distribution,
lattice_names=self.lattice_names,
description=self.description,
)To avoid code duplication you can replace the above code by: obj = copy.copy(self)May be, I would be better to have |
|
I'm almost finished with this PR now. I just need to make some changes to the factory because it no longer needs to handle validation errors since that has been moved into the schema validator which is supposed to be used in a separate step before the dict is passed into the factory. I want validation to be optional so turns out that I need to make some changes to the file loader to not add the location metadata directly into the dict but in a separate structure to avoid the factory having to strip the metadata when validation is not used. When validation is used the schema validator strips that data before returning the validated dict since it is only needed for the validation errors. I will make that change in a separate PR first. |
|
After understanding the file loader better I realised that there already is a path to not have the location metadata inside the dict. I just need to use the fast loader. So the two flows can be like this: If you want validation: load file using SafeLineLoader -> go through validator to get location errors and strip the location metadata -> into factory If you don't want validation: load file using CLoader -> no location metadata is added to dict -> into factory |
TeresiaOlsson
force-pushed
the
schema-registry-rf
branch
from
08d48e1 to
21365b6
Compare
TeresiaOlsson
requested review from
JeanLucPons,
gubaidulinvadim and
gupichon
and removed request for
JeanLucPons
|
This is ready for review now. Some parts are still a bit rough but it's just because making them nicer requires changes in more files. And some can't be completely fixed until the Accelerator has been moved to the new format but that requires changing all other parts first. |
|
I make a try, a chromaticty scan was ok. but the repr output is confusing. sr = Accelerator.load("../tests/config/EBSOrbit.yaml",use_fast_loader=True)
SR = sr.live
print(SR.get_rf_plant("RF"))
print(SR.get_rf_trasnmitter("RFTRA")) |
Yes, that's not correct. The idea of the new repr to have something generic instead of the ConfigModel is to build it from public attributes and properties. But in this case it looks like it failed because there are properties voltage and phase which returns an abstract.ReadWriteFloatScalar. I need to think about a better solution. I also don't really like the voltage_name and phase_name attributes and would prefer if they could be removed somehow. |
|
The nicest solution might be if |
|
For such case, I would prefer to be able to configure repr and exclude some params. i.e: class RFTransmitter(Element, DynamicValidation):
...
def __repr__(self):
return __pyaml_repr__(self,exclude=["voltage","phase"]) |
|
Okay. I will add the exclude option. |
2 participants
Description and motivation
Adding schema registry, validator and generator. It is only implemented for the RF modules as a first step.
There are several purposes:
Validation
The PR contains two types of validation: This is because they have different purposes and therefore different requirements:
Validation of external input :
This is configuration loaded from file, database etc. For this a field
class/typeis required and the models can't include arbitrary types otherwise Pydantic can only validate the type and not the full data. This is handled byConfigurationSchemaclasses that are registered into aSchemaRegistryand validated using aSchemaValidator.This is added to a class using the
register_schemadecorator which both allows to specify which schema class to use or have it dynamically generated.Validation at object creation:
This is validating the input when creating an object. For this there can't be a
class/typefield and arbitrary types must be allowed since a class can take a custom class as input. This is handled by a metaclassValidationMetainherited byDynamicValidationorStaticValidation. These are classes that if inherited adds an attributevalidation_modelwhich specifies the Pydantic BaseModel to use for the validation. The validation model can either be dynamically generated or added explicitly by the user.This is added to a class by inheriting from
DynamicValidationorStaticValidation.Both validation options should be optional so the user can choose to use both, one or none depending on their use case by using flags (not implemented yet). This should also allow to disable the use of Pydantic in case of compatibility issues in the future so if you have a correct configuration use of pyAML is not blocked by changes/bugs in Pydantic.
JSON schema generation
In addition there is a
SchemaGeneratorwhich generates a JSON schema for the configuration based on the schemas registered in theSchemaRegistry. Each schema should be replaced by it's subclasses to make it easy for the user to know which schemas they can choose between.Not changed yet for compatibility reasons
PYAMLCLASSattribute still exists to allow to load configuration files which has thetypefield instead ofclass. The validator converts from the old format to the new. This will be removed when all modules have been moved to the new format and old configuration files translated.