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

Enable plugin metadata setting from manifest's initialize block #815

Closed
5 tasks done
Tracked by #761
zanete opened this issue Jun 11, 2024 · 6 comments · Fixed by #911
Closed
5 tasks done
Tracked by #761

Enable plugin metadata setting from manifest's initialize block #815

zanete opened this issue Jun 11, 2024 · 6 comments · Fixed by #911
Assignees
Labels
core-only This issue is reserved for the IF core team only

Comments

@zanete
Copy link

zanete commented Jun 11, 2024

Why: Sub of #761

What: Add metadata to all builtins and enable metadata overwriting from initialize

Context

Instead of defining parameters in params.ts, we can make use of the metadata field we already expose in our plugin interface and move the parameter definitions into the plugins themselves. The plugin interface should looks as follows:

export const MyPlugin = (globalConfig: Config): Plugin => { 
  const metadata = { 
    kind: 'execute', 
    inputs: [
      example: {
          description:' example description'
          unit: 'dimensionless'
        }
      ]
    outputs: [ 
      energy: { 
        description: 'amount of energy utilised by the component', 
        unit: 'kWh'
        }, 
      ] 
    }; 
    
    . . . 
    
    return execute, metadata; }

This ticket covers entering each builtin in the IF repository and adding the necessary parameter metadata. There should be an object inside outputs: for each value output by the plugin, and the same for inputs.

note that these should be optional in the sense that we don't break existing plugins that don't provide plugin metadata here. We will update all our code and our documentation, best practise guides and templates to use this feature, but old plugins should still run - the downstream impact will just be that the explainer feature won't work for them.

The parameter metadata should be updateable by passing in inputs and outputs arrays in the initialize block in the manifest. If no parameter metadata is passed from the manifest, the plugin should use the default hardcoded into the plugin source code (which may be none).

For example, an instance of the Sum plugin might be configured as follows:

    "sum":
      path: "builtin"
      method: Sum
      global-config:
        input-parameters:
          - cpu/energy
          - network/energy
        output-parameter: energy-sum
      parameter-metadata:
        inputs:
          - cpu/energy
            description: energy consumed by the cpu
            unit: kWh
          - network/energy
            description: energy consumed by data ingress and egress
            unit: kWh
        outputs:
          - energy-sum
            description: sum of energy components
            unit: kWh

In this case, the plugin metadata gets updated with the values in parameter-metadata during the initialization. If the parameter-metadata is missing, then the hardcoded defaults for the plugin are used. The same is true if an individual parameter's metadata is missing - just use the hardcoided default. If there is no hardcoded default, don't error out, just ignore (we'll handle reporting these issues later when we built the explainer feature.

SoW

  • Add feature that updates plugin metadata according to the parameter-metadata provided in the manifest's initialize block.

  • For builtins that have known inputs/outputs, add them to the hardcoded defaults in the plugin code

Acceptance criteria:

  • Plugin interface accepts parameter metadata, with arrays for inputs and outputs

    The plugin metadata definition in the plugin source code should look roughly as follows:

type ParameterMetadata = {
    description: string; 
    unit: string
    };

export const Exponent = (globalConfig: ExponentConfig): ExecutePlugin => {
  const metadata = {
    inputs: ParameterMetadata [] | undefined,
    outputs: ParameterMetadata []| undefined,
    kind: 'execute',
  };
  • Plugin metadata is updated using values passed from parameter-metadata feature in manifest's initialize block

    GIVEN the feature is implemented
    WHEN the following metadata is provided in the manifest file

intiialize:
  plugins:
    "sum":
       path: "builtin"
       method: Sum
       global-config:
         input-parameters:
           - cpu/energy
           - network/energy
         output-parameter: energy-sum
         parameter-metadata:
           inputs:
             - cpu/energy
               description: energy consumed by the CPU 
               unit: 'kWh'
             - network/energy
               description: energy consumed by network data ingress/egress  
               unit: 'kWh'
    "coefficient":
       path: "builtin"
       method: Coefficient
       global-config:
         input-parameter: energy
         coefficient: 2
         output-parameter: energy-doubled
      parameter-metadata:
        inputs:
          - energy
            description: energy consumed by the CPU 
            unit: 'kWh'
          - coefficient
            description: coefficient applied to energy to yield energy-doubled  
            unit: 'dimensionless'          
         outputs:
           - energy-doubled
             description: energy multiplied by coefficient  
             unit: 'kWh'

THEN the plugin instance is initialized with the values from the plugin-metadata field from its initialize block loaded into its metadata object.

  • Feature ignores missing metadata in parameter-metadata config
    GIVEN the feature is implemented
    WHEN I run a manifest with the following initialize block
initialize:
  plugins:
    "sum":
       path: "builtin"
       method: Sum
       global-config:
         input-parameters:
           - cpu/energy
           - network/energy
         output-parameter: energy-sum

OR the initialize block loois as follows:

initialize:
  plugins:
    "sum":
       path: "builtin"
       method: Sum
       global-config:
         input-parameters:
           - cpu/energy
           - network/energy
         output-parameter: energy-sum
         parameter-metadata:
           inputs:
           outputs:

THEN: The manifest runs without throwing an error, the plugin's metadata object just looks as follows:

{
  inputs: undefined
  outputs: undefined
}
@zanete zanete added this to the Inputs and Outputs milestone Jun 11, 2024
@zanete zanete moved this to In Design in IF Jun 11, 2024
@zanete zanete added draft The issue is still being written, no need to respond or action on anything. core-only This issue is reserved for the IF core team only labels Jun 11, 2024
@zanete zanete added the blocked The issue is blocked and cannot proceed. label Jun 11, 2024
@jmcook1186 jmcook1186 moved this from In Design to In Refinement in IF Jun 14, 2024
@zanete zanete removed the draft The issue is still being written, no need to respond or action on anything. label Jun 14, 2024
@zanete
Copy link
Author

zanete commented Jun 14, 2024

@manushak @narekhovhannisyan please review AC to see if it all looks good

@jmcook1186 jmcook1186 changed the title Add metadata to all builtins Enable plugin metadata setting from manifest's initialize block Jun 17, 2024
@manushak manushak removed their assignment Jun 27, 2024
@narekhovhannisyan narekhovhannisyan removed their assignment Jun 27, 2024
@zanete zanete moved this from In Refinement to Ready in IF Jun 27, 2024
@manushak manushak moved this from Ready to In Progress in IF Jul 11, 2024
@manushak
Copy link
Contributor

@jmcook1186 there is an inconsistency between the parameter-metadata yaml structure and the type

if we want the yaml to look like this:

outputs:
  - energy-sum:
    description: sum of energy components
    unit: kWh

the data will be:

outputs: [
    {
      "energy-sum": null,
      "description": "sum of energy components",
      "unit": "kWh"
    }
  ]

if we want the type to be an array, the yaml will be:

inputs:
  - cpu/energy:
      description: energy consumed by the cpu
      unit: kWh
  - network/energy:
      description: energy consumed by data ingress and egress
      unit: kWh

the description and unit should have an extra tab

There is another version

inputs:
  cpu/energy:
    description: energy consumed by the cpu
    unit: kWh
  network/energy:
    description: energy consumed by data ingress and egress
    unit: kWh

and in this case, the data will be an object

inputs: {
    "cpu/energy": {
      "description": "energy consumed by the cpu",
      "unit": "kWh"
    },
    "network/energy": {
      "description": "energy consumed by data ingress and egress",
      "unit": "kWh"
    }
  },

Which version do you prefer?

@zanete zanete removed the blocked The issue is blocked and cannot proceed. label Jul 12, 2024
@jmcook1186
Copy link
Contributor

@manushak are there any knock on effects on the plugin side? Assuming not, I prefer the middle option where the yaml is consistent with the other elements in the manifest, i.e. yaml like this:

inputs:
  cpu/energy:
    description: energy consumed by the cpu
    unit: kWh
  network/energy:
    description: energy consumed by data ingress and egress
    unit: kWh

@manushak manushak moved this from In Progress to Pending Review in IF Jul 12, 2024
@manushak manushak linked a pull request Jul 12, 2024 that will close this issue
9 tasks
@zanete
Copy link
Author

zanete commented Jul 17, 2024

@narekhovhannisyan please review the PR in the if-core
@manushak if you could share a link, that would be helpful 🙏

@manushak
Copy link
Contributor

@narekhovhannisyan the PR i if-core was merged. Could you please release it?

@zanete zanete moved this from Pending Review to In Progress in IF Jul 18, 2024
@zanete zanete moved this from In Progress to Pending Review in IF Jul 18, 2024
@zanete
Copy link
Author

zanete commented Jul 18, 2024

@narekhovhannisyan please change the version locally to review the pr 🙏

@github-project-automation github-project-automation bot moved this from Pending Review to Done in IF Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-only This issue is reserved for the IF core team only
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants