Green-Software-Foundation / if

Impact Framework
https://if.greensoftware.foundation/
MIT License
137 stars 38 forks source link

Update builtins with node-level config to use global scope config #776

Open zanete opened 1 month ago

zanete commented 1 month ago

Why: Sub of #765

What:

Refactor any builtins that use node-level config so that all config comes from global scope

Context

IF has been through several iterations. Originally we just aimed to calculate SCI scores for simple architectures, now its something much more general purpose and more akin to a low level protocol for environmental impact calculations. This means we have redesigned the way we pass config data around IF a couple of times too. We currently support three ways to get data into a plugin:

Node level config was intended to reconfigure already-initialized plugins so that one instance can be re-used in multiple parts of the tree. This was useful before we migrated most of our pipelines over to generics. Now, node-level config is rarely used. Instead, we create named instances of our plugins that are dropped into each pipeline as needed. This is much easier to understand and audit, even though it makes the initialize block more verbose. For this reason, node level config is rarely used nowadays, and is causing some confusion for plugin developers and builders. We think it's time to remove node level config as a concept from IF.

Having multiple places to configure a plugin is confusing, it’s unclear for plugin developers if they should architect for multiple instances of a plugin with different global config options, or single-instance of a plugin with multiple node level configs.

Node level config is difficult to rationalize, node-level config applies to multiple levels of a tree and can be overridden by child component node-level config.

From the hackathon, it looks like very few plugin authors used node-level config, so it’s not a feature which is broadly adopted - perhaps because we do not use node-level config in most built-ins, showing that even the core team don’t get much value from it.

This ticket covers auditing our builtins to find any example where we use node-level config. Where node-level config is used, refactor the code to remove node-level config altogether and pass all config using the global scope.

We can then rename global-config to just config.

Scope of work:

NOTE I have been through and checked our builtins -there is only one (groupby) that uses node level config. Since groupby is going to be refactored anyway in #809 , we can skip the refactoring part of this ticket and make this ticket only renaming globalConfig -> config across our builtins.

Acceptance criteria:

Scenario 1:

Scenario 2

narekhovhannisyan commented 1 month ago

BTW can we rename global config to just config in that case? Otherwise lgtm

jmcook1186 commented 1 month ago

yes - good call.

zanete commented 2 weeks ago

@jmcook1186 to check if this is just a case of needing to update docs. Related issue is #777

jmcook1186 commented 1 week ago

@zanete this task has reduced down to renaming globalConfig -> Config across IF and builtins if we can do this without breaking existing plugins. If this is a breaking change we can park it until much closer to the v1 release. cc @narekhovhannisyan