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

display-results tasks yields error: max_measurement_number: unbound variable #43

Closed
marians opened this issue Oct 5, 2023 · 13 comments
Closed

Comments

@marians
Copy link

marians commented Oct 5, 2023

I just added the two steps shown in the example in https://github.com/marketplace/actions/eco-ci-energy-estimation to a workflow and executed it. As a result, I got this error in the "Show energy results" step

Running a measurement to have at least one result to display.
|Label|🖥 avg. CPU utilization [%]|🔋 Total Energy [Joules]|🔌 avg. Power [Watts]|Duration [Seconds]|
|---|---|---|---|---|
|Total Run|2.38538|360.733|1.97122|184|
/home/runner/work/_actions/green-coding-berlin/eco-ci-energy-estimation/v2/scripts/display_results.sh: line 98: max_measurement_number: unbound variable
Error: Process completed with exit code 1.

The workflow snippet:

      - name: Show energy results
        uses: green-coding-berlin/eco-ci-energy-estimation@v2
        with:
          task: display-results
@marians
Copy link
Author

marians commented Oct 5, 2023

FYI This happened in a private github.com repository

@ribalba
Copy link
Member

ribalba commented Oct 5, 2023

Hey, sorry you encountered an error. Did you see the extra setup steps for private repos under:

https://github.com/green-coding-berlin/eco-ci-energy-estimation#note-on-private-repos

@marians
Copy link
Author

marians commented Oct 5, 2023

Got it in the meantime and added get-measurement steps. The error is gone now.

Would it make sense to make the action work so that get-measurement is optional? In other words, a measurement would have to be taken always at the end, before generating the output. That would allow for a simpler workflow and would prevent this error.

@ribalba
Copy link
Member

ribalba commented Oct 5, 2023

@dan-mm is there a way we can just output everything when calling display-results? I remember there was a reason we needed to call get-measurement at least once. But I can't remember why.

@dan-mm
Copy link
Contributor

dan-mm commented Oct 6, 2023

Hm it should already work that way.

Get-measurement is there for the user to explicitly say, here is where I want to make a measurement, and display-results is decoupled from actually making any measurements but just displaying the results of all your previous measurements.

But there is a safety clause in display-measurement where if get-measurement was never called, it makes at least one measurement then and there to have something to display. This clearly has a bug in it so I'll investigate now

@dan-mm
Copy link
Contributor

dan-mm commented Oct 6, 2023

@marians - found and fixed the issue. It should now work again even if make-measurement is never called. Thank you for the bug report!

Lemme know if it works this way for you now, or if you have any other issues!

@marians
Copy link
Author

marians commented Oct 6, 2023

Thanks for that! Closing this issue then.

@marians marians closed this as completed Oct 6, 2023
@ahelwer
Copy link

ahelwer commented Jul 9, 2024

Unfortunately I believe this bug has returned. Without calling get-measurement, I have the following errors/warnings in display-result:

Running a measurement to have at least one result to display.
|Label|🖥 avg. CPU utilization [%]|🔋 Total Energy [Joules]|🔌 avg. Power [Watts]|Duration [Seconds]|
|---|---|---|---|---|
|Total Run|30.7575|478.446|4.02056|120|
/home/runner/work/_actions/green-coding-solutions/eco-ci-energy-estimation/v3/scripts/display_results.sh: line 108: label_1: unbound variable
/home/runner/work/_actions/green-coding-solutions/eco-ci-energy-estimation/v3/scripts/display_results.sh: line 108: cpu_avg_1: unbound variable
/home/runner/work/_actions/green-coding-solutions/eco-ci-energy-estimation/v3/scripts/display_results.sh: line 108: total_energy_1: unbound variable
/home/runner/work/_actions/green-coding-solutions/eco-ci-energy-estimation/v3/scripts/display_results.sh: line 108: power_avg_1: unbound variable
/home/runner/work/_actions/green-coding-solutions/eco-ci-energy-estimation/v3/scripts/display_results.sh: line 108: time_1: unbound variable
||||||

You can see it in this CI run: https://github.com/tlaplus-community/tree-sitter-tlaplus/actions/runs/9863040971/job/27234920568?pr=120

@ahelwer
Copy link

ahelwer commented Jul 9, 2024

A subsequent CI run with a get-measurement task run right before display-results succeeds [link]:

Label 🖥 avg. CPU utilization [%] 🔋 Total Energy [Joules] 🔌 avg. Power [Watts] Duration [Seconds]
Total Run 29.0274 485.607 3.98039 131
Measurement #1 30.2834 485.607 3.98039 123

📈 Energy graph:

 
 8.18 ┤                                                                     ╭─╮
 7.54 ┤                                                                     │ ╰╮
 6.90 ┤                                                                    ╭╯  │                  ╭╮
 6.26 ┤                                                                    │   │                  ││
 5.62 ┤                                                                    │   ╰╮                 ││
 4.97 ┤              ╭╮                                                    │    │                 ││         ╭─╮    ╭╮
 4.33 ┤          ╭╮  │╰─╮                                                  │    │      ╭╮         │╰─╮       │ ╰╮ ╭─╯╰╮
 3.69 ┤         ╭╯│╭─╯  ╰──────────────────────────────────────────────────╯    ╰───╮╭─╯╰─────────╯  ╰───────╯  ╰─╯   ╰─────────
 3.05 ┤    ╭─╮  │ ╰╯                                                                ╰╯
 2.41 ┼╮   │ ╰─╮│
 1.77 ┤╰───╯   ╰╯
                                                            Watts over time

🌳 CO2 Data:
City: Chicago, Lat: 41.8819, Lon: -87.6278
Carbon Intensity for this location: 488 gCO₂eq/kWh
SCI: 0.236976 gCO₂eq / pipeline run emitted

@ArneTR
Copy link
Member

ArneTR commented Jul 10, 2024

Oh ,noes!

Thanks for bringing this our attention again!

There are two things that catch my eye:

  • You are using a version that will soon be outdated. @V3 . We have greatly revamped the functionality with @v4 which is currently in a release candidate (https://github.com/green-coding-solutions/eco-ci-energy-estimation/releases/tag/v4.0-rc3)
    So just a heads up that you might want to try out that version. It will greatly improve performance / reduce overhead and is also more slim in general.

  • Second I think what Dan implemented here is more of a "workaround" than an implementation that we necessarily will continue to support.
    Before I dive into the code: Can you state why you plan to use the action different from the suggested approach in the documentation? Which is having an init, measure and display step? Just for brevitity? Or did you just miss the step and we can maybe improve rather on the documentation side than the integration side?

In any case: I totally agree that the plugin should not fail on you, no matter how you use it. But I am currently trying to discern if a guard clause or a feature is the way to go here.

@ArneTR ArneTR reopened this Jul 10, 2024
@ahelwer
Copy link

ahelwer commented Jul 10, 2024

Thanks! I tried the RC but then I just get this error in the measurement step:

{"success":false,"err":"Technical error with getting data from the API - Please contact us: [email protected]"}

Here is the CI run.

I only really care about getting a single measurement to see the eco impact of the entire CI, and the display-results action seems to insert its own final measurement, so if I insert a measurement step right before display-results those two measurements are basically identical.

@ArneTR
Copy link
Member

ArneTR commented Jul 12, 2024

Hey @ahelwer

I had now time to look at this properly.

First of all thanks for trying out the RC version.

The error you are seeing stems from the fact that the CarbonDB integration is not configured correctly. You must set all three variables.

  • machine-uuid
  • project-uuid
  • company-uuid

This was not the case in the prior version. Since the RC is not release yet this is not documented, but will be added then. Currently it was just stated partially in the release description.

  1. The functionality you are looking for was only available in the non-RC version.

I see it being nice to have though and have crated a PR. Can you give that a try? #87

To use it to have to change the statement in your Workflow File to: green-coding-solutions/eco-ci-energy-estimation@display-results-dedicated

Just a heads up: When you are only using display results the data will never get sent to our dashboard where you can see the aggregated data (https://metrics.green-coding.io/ci-index.html) ... is that what you want?

@ahelwer
Copy link

ahelwer commented Sep 24, 2024

That's a good point, I think I will just do the extra measurement then. Thanks for all the help! This can probably be closed.

@ribalba ribalba closed this as completed Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants