Changes for the approved custom runtime SUP - #203
Conversation
Signed-off-by: Philip <[email protected]>
|
I've created the draft PR even though I'm still working on these changes, so people can see what is being done with this if they are interested. |
Signed-off-by: Philip <[email protected]>
Signed-off-by: Philip <[email protected]>
Signed-off-by: Philip <[email protected]>
Signed-off-by: Philip <[email protected]>
Signed-off-by: Philip <[email protected]>
phil-abb
changed the title
|
@margo/technical-wg / @margo/development-team - The specification changes to support deploying applications requiring a custom runtime are ready for review. This is a companion PR to add the new folder to the site nav: margo/documentation_website#5 |
|
|
||
| * All `matchExpressions` within a single selector are combined using logical AND. | ||
| * If an eligibility rule contains both `propertySelector` and `labelSelector`, both selectors must match for that rule to match. | ||
| * The `eligibilityRules` array is combined using logical OR. A device matches eligibility rules if at least one eligibility rule matches. |
There was a problem hiding this comment.
This statement seems contradictory to the above :
If I understand it right, an eligilibilityRule contains one or more of propertySelector OR labelSelector.
But these 2 rules are contradictory and will give rise to confusion when multiple selectors are present.
- If the
eligilibilityRulecontains multiplepropertySelector, then the evaluation of the combination will use an OR expression - If the
eligilibilityRulecontains multiplepropertySelectorandlabelSelector, then the evaluation of the combination will use an AND expression.
Since, no grouping / ordering is specified, This may give rise to expressions like -
PS1 OR PS2 OR PS3 AND LS1
PS1 OR PS2 AND LS1 OR PS3 AND LS2
This may be interpreted differently by App Developers, while machine evaluation will likely have a single interpretation
I recommend the following -
- Separate out array-structures for
propertySelectorandlabelSelector - Evaluation of the
eligilibilityRuleshould be done as - AT LEAST ONE OFpropertySelectorAND AT LEAST ONE OFlabelSelector
There was a problem hiding this comment.
@nilanjan-samajdar - Thanks for the feedback. I see what you mean about how the current structure can lead to confusion. I'll work on some changes to address this.
There was a problem hiding this comment.
I'm not going to change the structure because I intended the flexibility this structure provides, but I'm adding more language to the section to make the rules evaluation logic clearer.
There was a problem hiding this comment.
@nilanjan-samajdar I pushed up some changes to add more clarity to the evaluation rules.
Since, no grouping / ordering is specified
We can potentially add a weight or priority to the rules later, but I don't want to do this right now. I would rather get some feedback from the community and have someone come back with a specific use case that needs something like this so we can understand why it's needed and not just assume it's needed.
|
|
||
| This label selector MUST resolve to a device capability label whose value is either: | ||
|
|
||
| * a string, number, or boolean |
There was a problem hiding this comment.
This usage is confusing.
A labelSelector must strictly resolve to a string, any other value is a actually a propertySelector
There was a problem hiding this comment.
@nilanjan-samajdar Labels are key/value pairs, and the label value can be more than strings
A label value MUST be either:
a string, number, or boolean
an array of one or more strings, or numbers
|
|
||
| {%- if c.name.startswith("MatchExpression") %} | ||
|
|
||
| The following rules MUST be followed when evaluating match expressions: |
There was a problem hiding this comment.
In the yaml, these are called operator, we should use the same term here or change it in the examples
There was a problem hiding this comment.
I'll update the language in this section
There was a problem hiding this comment.
@nilanjan-samajdar I pushed up changes for this section
| itemSelector: | ||
| matchExpressions: | ||
| - key: /type | ||
| operator: In |
There was a problem hiding this comment.
The description calls this property selectorOperator
There was a problem hiding this comment.
@nilanjan-samajdar - It's unclear what you are suggesting here. The property name is "operator," and the property type in the LinkML/OpenAPI documentation is "SelectorOperator". Are you suggesting that the property type should also be "Operator," or something else?
| operator: In | ||
| values: ["gpu"] | ||
| - key: /manufacturer | ||
| operator: In |
There was a problem hiding this comment.
The description calls this property selectorOperator
| itemSelector: | ||
| matchExpressions: | ||
| - key: /type | ||
| operator: In |
There was a problem hiding this comment.
The description calls this property selectorOperator
| operator: In | ||
| values: ["gpu"] | ||
| - key: /manufacturer | ||
| operator: In |
There was a problem hiding this comment.
The description calls this property selectorOperator
| operator: In | ||
| values: ["NVIDIA"] | ||
| - key: /interfaces | ||
| operator: ContainsAll |
There was a problem hiding this comment.
The description calls this property selectorOperator
| itemSelector: | ||
| matchExpressions: | ||
| - key: /type | ||
| operator: In |
There was a problem hiding this comment.
The description calls this property selectorOperator
|
|
||
| {%- if c.name.startswith("CapacityRequirements") %} | ||
|
|
||
| The following rules MUST be applied when performing capacity requirements matching: |
There was a problem hiding this comment.
Do the CPU, Memory constraints depend on the choice of 'Runtime' ?
If yes, it makes sense to specify this in the ApplicationDescription. where it will apply to all possible applicationDeploymentProfile types.
Otherwise, if it varies with Runtime, these constraints should be in applicationDeploymentProfile
There was a problem hiding this comment.
The deviceConstraints object is part of the individual deployment profiles. So, different device constraints can be defined for each targeted deployment type.
Signed-off-by: Philip <[email protected]>
| | vendor | string | Y | Defines the device vendor.| | ||
| | modelNumber | string | Y | Defines the model number of the device.| | ||
| | serialNumber | string | Y | Defines the serial number of the device.| | ||
| | cpus | []CPU | Y* | List of CPU entries available on the device. Utilized to match with the required resources defined in the application description. See the [CPU](#cpu-attributes) section below.| |
There was a problem hiding this comment.
Hi @phil-abb ,
One thing I want clarification about, propertySelector is for Margo defined properties. We have capacityRequirements for cpu, mem, storage. However, cpu, mem, storage are part of Margo defined properties for DeviceCapabilities. So, there is a clash between considering cpu/mem/storage as part of capacityRequirements OR propertySelector OR both ? currently, I was unable to find the part where SUP talks about this particular aspect, nor I was able to find something on whether to omit it for propertySelector explicitly, and in case both are provided, then what should be the behavior?
If I am missing something, please let me know!
There was a problem hiding this comment.
Fair point. I can work on adding some content to the PR to address this.
There was a problem hiding this comment.
@spulkit138 - I pushed up an update to add a paragraph to the Capacity Requirements attributes section in the application description page to clarify how this should be handled.
Signed-off-by: Philip <[email protected]>
Signed-off-by: Philip <[email protected]>
3 participants
Description
Changes for the approved custom runtime SUP
Issues Addressed
List and link all the issues addressed by this PR.
Change Type
Please select the relevant options:
Checklist