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

Update explain behavior to support the visualizer flyout function. #1058

Closed
Tracked by #1028
jawache opened this issue Oct 23, 2024 · 6 comments · Fixed by #1065 or Green-Software-Foundation/if-docs#118
Closed
Tracked by #1028
Assignees
Milestone

Comments

@jawache
Copy link
Contributor

jawache commented Oct 23, 2024

The vizualizer fly out needs to be able to show inputs and outputs properly, e.g. in this example I'm hovering over a pipeline and two outputs are highlighted. One is an input and one is an output, the current underlying data in the manifest file doesn't show inputs/outputs (yes, I know I asked for it in this way, I was wrong!) so we need to add them back in.

Can we also call the field in the manifest, "key" or something other than "explain", I feel explain should do more than just describe the fields.

Screenshot 2024-10-23 at 13 22 04

Details for Narek

We basically want to revert the changes in this PR
#971

So instead of returning

param-name
  plugins: //what plugins use this param?
  unit: 
  description:
  aggregation-method:

we should instead return:

plugin-name:
  inputs:
    param-1:
      description:
      unit: 
      aggregation-method:

So the parameter details are organized by plugin, then inputs/outputs. The aggregation method should still be returned. It is understood that this will lead to repetition of the parameters in the explain block if theyare used in multiple plugins. This is fine, as long as the aggregation method and unit are the same for identically named parameters.

Example

Here's a snippet of the explain block from a manifest I just ran:

explain:
  data-transferred:
    plugins:
      - network-energy-to-transfer-call-data
    unit: GB
    description: >-
      Data transferred over network, in GB. Estimated from values here
      https://tactiq.io/learn/zoom-data-usage-facts.
    aggregation-method:
      time: sum
      component: sum
  kwh-per-gb-network:
    plugins:
      - network-energy-to-transfer-call-data
    unit: kWh/GB
    description: >-
      Coefficient relating energy expendidture to 1GB of data transferred over
      network. Taken from Orbinger et al 2021
      (https://www.sciencedirect.com/science/article/abs/pii/S0921344920307072)
    aggregation-method:
      time: copy
      component: copy
  energy:
    plugins:
      - network-energy-to-transfer-call-data
      - energy-to-carbon
      - sum-energy-components
    unit: kWh
    description: >-
      Total Energy consumed by the server to handle zoom call, for the given
      timestep.
    aggregation-method:
      time: sum
      component: sum

and this is what it should look like after the proposed changes have been made:

explain:
  network-energy-to-transfer-call-data:
    inputs:
      data-transferred:
        unit: GB
        aggregation-method:
          time: sum
          component: sum
        description: >-
          Data transferred over network, in GB. Estimated from values here
          https://tactiq.io/learn/zoom-data-usage-facts.
      kwh-per-gb-network:
        unit: kWh/GB
        aggregation-method:
          time: copy
          component: copy
        description: >-
          Coefficient relating energy expendidture to 1GB of data transferred over
          network. Taken from Orbinger et al 2021
          (https://www.sciencedirect.com/science/article/abs/pii/S0921344920307072)
    outputs:
      energy:
        unit: kWh
        description: >-
          Total Energy consumed by the server to handle zoom call, for the given
          timestep.
        aggregation-method:
          time: sum
          component: sum

  next-plugin...

If two instances of the same parameter are seen with different units or aggregation methods then we should error out.

@zanete
Copy link

zanete commented Oct 23, 2024

@jawache assuming this is one is for @narekhovhannisyan ?

@zanete zanete moved this to Ready in IF Oct 23, 2024
@jawache
Copy link
Contributor Author

jawache commented Oct 23, 2024

@zanete yes, but let's make sure whatever format we come up with works for @osamajandali

@zanete zanete moved this from Ready to In Refinement in IF Oct 24, 2024
@zanete zanete added this to the IF 1.0 milestone Oct 24, 2024
@zanete
Copy link

zanete commented Oct 24, 2024

@jmcook1186 please refine this with acceptance criteria so @narekhovhannisyan could work on this, and after that it will be easier for @osamajandali to adjust visualiser code

@jmcook1186
Copy link
Contributor

@narekhovhannisyan @zanete added detail to the ticket for the IF side of this.

@zanete zanete moved this from In Refinement to Ready in IF Oct 28, 2024
@zanete zanete assigned manushak and unassigned jmcook1186 Oct 28, 2024
@zanete zanete moved this from Ready to In Progress in IF Oct 29, 2024
@zanete
Copy link

zanete commented Oct 29, 2024

PR expected soon

@manushak manushak linked a pull request Oct 29, 2024 that will close this issue
6 tasks
@manushak manushak moved this from In Progress to Pending Review in IF Oct 29, 2024
@manushak manushak linked a pull request Oct 30, 2024 that will close this issue
@jawache
Copy link
Contributor Author

jawache commented Oct 30, 2024

@jmcook1186 I was thinking it makes some sense for the plugins to also have optional textual descriptions same a parameters. Right now we're using long strings in the plugin name itself to describe what it is for, but perhaps we can have a description for the plugins themselves also in the initialise block. Maybe some plugins could return descriptions themselves and the built-ins you can define the description in the config. So energy-in-kwh could have a description that is more human readable with somewhat of an explanation as to why e.g. "convert energy to kwh so that it can be used in the XXX calculation methodology"

Also I see you're point on the replication of data, but the manifest files are already super long. I vaguely remember I was against the idea of just fleshing out the initialize block in an output manifest originally with the explain data, I'm not sure what my argument was then but it's making sense now. It also has the advantage that the visualizer could also just display the config associate with the plugin when you hover over it, or just gives more freedom to Osama to get additional data related to a plugin and display it in the fly out.

@github-project-automation github-project-automation bot moved this from Pending Review to Done in IF Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
5 participants