-
Notifications
You must be signed in to change notification settings - Fork 5
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
3.0.0 Schema changes #230
3.0.0 Schema changes #230
Conversation
NOTE: this is breaking the system test because it uses ioc-template-example and that in turn is using ibek-support which has not been updated to the new schema changes yet. NEXT CALL: adding CI to ibek support that checks all of this so I can
|
src/ibek/params.py
Outdated
@@ -33,74 +33,73 @@ def __repr__(self): | |||
return str(self.value) | |||
|
|||
|
|||
class Value(BaseSettings): | |||
class Define(BaseSettings): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I now realise I don't like this name. Classes shouldn't be verbs. And DefinesTypes
is very unclear.
It also leads me to a question. Are pre_defines
and post_defines
different to Definitions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this was one of the points in slack we have definitions that contain pre_defines and post_defines.
Your verb point is also good.
So how about values then!! ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e.
pre_values
post_values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should they not be called pre/post args given that they are inserted into the Entity args?
Or perhaps, the section in the yaml could stay as defines, but the object is Pre/Post Arg. And the description could be Pre/Post -defined Args for Entity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that keen on the trend we have started of using different class names in the code than the key names in yaml.
Maybe we could fix that and this issue by renaming
defs:
to
entityDefinitions:
which are what we called the class in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EntityDefinition in yaml and code sounds good.
These are distinct from Parameters (no longer called Args and now appearing under params: in yaml) because they are calculated - never supplied by writer of an ioc.yaml - thus distinctly not parameters (or args!)
Ah right they are being renamed here. Then should this be updated? Or are there now Args and Params?
It looks like (what is currently called) Defines are converted directly into the EntityFactory args (just a dictionary of builtins) and then that is used to populate the fields of the dynamically generated Entity model, which I think makes them parameters, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes all use of 'arg' in the code should be replaced with param and your link is to an oversight.
You are right that 'defines' get treated like 'params' because they go in the Entity Model. But you are not supposed to be able to use them as params - now I'm worried that you can - just checking that now ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - you can reference definitions in the ioc.yaml so that was unintentional.
Now the question is is that a bad thing. i.e. although they are intended as calculated values or constants, you can still override them in the ioc.yaml if you wish.
I think that probably it is a bad thing because the order of interpretation of jinja strings is supposed to be clear and I'm feeling that this is muddying it. Also - I did not want to confuse the ioc instance writer with many extra parameters that they should not normally set.
So I need to fix that and then still come up with the correct name.
I used pre_defines and post_defines because they are SUPPOSED to be like defines in C ie. calculated values or constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help think about this here is a reminder of an example use of defines.
ibek/tests/samples/support/fastVacuum.ibek.support.yaml
Lines 114 to 140 in d82c40f
post_defines: | |
gaugeNum: | |
description: auto gauge count | |
type: int | |
value: |- | |
{{ _global.incrementor(master, start=1) }} | |
fan: | |
description: fan number | |
value: |- | |
{{ "%02d" % (gaugeNum / 7 + 1) }} | |
mask: | |
description: mask for the channel | |
value: |- | |
{{ _global.incrementor("mask_{}".format(master), 2, 2**gaugeNum) }} | |
lnk_no: | |
description: link number | |
value: |- | |
{{ ((gaugeNum - 1) % 6) + 1 }} | |
gaugePV: | |
description: Gauge PV | |
value: |- | |
{{ master.device }}:GAUGE:{{id}}_0 | |
These are all calcualted values that help to generate values for db templates - not something the individual IOC instance creator wants to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GDYendell please can we spend 20 mins IRL discussing this today as I'd like to resolve ASAP.
* rename definition to entity_model * test_list working * all tests passing * lint * remove redundant changes * revert id_to_entity changes
@GDYendell after around 12 hours in a debugger it finally came out pretty in my opinion. All jina rendering and reference to pre_defines and post_defines are all handled in the Entity model validator here: Lines 97 to 122 in 2bd5cb3
(I think we eventually agreed that define may be used as a noun, right?) |
I think this PR needs to be renamed "Refactor the world" |
Was just thinking what to call it. But here is the cool bit.
Now this has only checked support yaml for those things we have actual IOC instances for but it is a very good cross-section. |
Co-authored-by: Gary Yendell <gary.yendell@diamond.ac.uk>
Co-authored-by: Gary Yendell <gary.yendell@diamond.ac.uk>
Co-authored-by: Gary Yendell <gary.yendell@diamond.ac.uk>
I have clarified the ID discussion as follows: Lines 124 to 139 in db46dba
and Lines 178 to 190 in 0043ef5
|
db46dba
to
0043ef5
Compare
This change allows us to more effectively use anchors and aliases to (multiple) inherit groups of parameters from other definitions or from shared fields.
Also includes a converter tool to convert support yaml and ioc yaml to work with the latest round of changes to ibek.
The names of the classes in the code have also been changed to reflect the name changes in the schema detailed below.
The schema changes are as follows:
defs
are nowentity_models
args
are nowparameters
parameters
are a dictionary of dictionaries instead of a list of dictionariesparameters
no longer have a name field as the key in the dictionary is the namevalues
is nowpost_defines
and is also a dictionary of dictionariespre_values
is nowpre_defines
and is also a dictionary of dictionaries__utils__
is now_global
_global
functions have changed:get_var
->get
set_var
->set
counter
->incrementor
on every invocation of the incrementor function (so non linear counters
are possible)
_global.get(incrementorName)
Backward compatible changes:
attributes (with appropriate care!!)
pre_defines
andpost_defines
now may have optionaltype:
which defaults tostr
and may belist
(of Any),int
,float
,bool
entity_model
may have ashared:
field which is alist
ofAny
shared:
at the root levelshared:
are scratch areas in which to place anchors that can be referred to by aliases - and are therefore useful for placing repeating patterns (especially ofparameters
) and then using aliases where these repeats are required.