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

Raising errors and applying caching #896

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

ArneTR
Copy link
Member

@ArneTR ArneTR commented Sep 15, 2024

@ribalba

I am currently trying to investigate an error we are seeing with the API when adding CarbonDB entries alongside CI measurements.

File "/var/www/startup/venv/lib/python3.12/site-packages/fastapi/routing.py", line 212, in run_endpoint_function
    return await dependant.call(**values)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/www/green-metrics-tool/api/main.py", line 1300, in post_ci_measurement_add
    carbondb_add(client_ip, [energydata], user._id)
  File "/var/www/green-metrics-tool/api/api_helpers.py", line 762, in carbondb_add
    co2_value = energy_kwh * carbon_intensity # results in g
                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
TypeError: unsupported operand type(s) for *: 'float' and 'NoneType'


<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 0_o >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

Error: Error in API call - catch_exceptions_middleware

Url (): http://api.green-coding.io/v1/ci/measurement/add
Query_params (): 
Client (): None
Headers (): MutableHeaders({'host': '[api.green-coding.io](http://api.green-coding.io/)', 'x-real-ip': '172.18.0.1', 'x-forwarded-for': '20.172.29.181, 172.18.0.1', 'x-forwarded-proto': 'http', 'connection': 'close', 'content-length': '954', 'accept-encoding': 'gzip, br', 'cf-ray': '8c372eaceb6d7d71-LAX', 'cf-visitor': '{"scheme":"https"}', 'user-agent': 'curl/7.81.0', 'accept': '*/*', 'content-type': 'application/json', 'cf-connecting-ip': '20.172.29.181', 'cdn-loop': 'cloudflare; loops=1', 'cf-ipcountry': 'US'})
Body (): b'{\n                "energy_value":"200379",\n                "energy_unit":"mJ",\n                "cpu":"EPYC_7763",\n                "commit_hash":"***REDACTED***",\n                "repo":"***REDACTED***",\n                "branch":"master",\n                "workflow":"***REDACTED***",\n                "run_id":"***REDACTED***",\n                "project_id":"",\n                "label":"argopy APIstatus",\n                "source":"github",\n                "cpu_util_avg":"16.2454",\n                "duration":"58",\n                "workflow_name":"api-status",\n                "cb_company_uuid":"***REDACTED***",\n                "cb_project_uuid":"***REDACTED***",\n                "cb_machine_uuid":"***REDACTED***",\n                "lat":"",\n                "lon":"",\n                "city":"",\n                "co2i":"",\n                "co2eq":""\n            }'
Exception (): unsupported operand type(s) for *: 'float' and 'NoneType'

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 0_o >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

As seen the value for carbon_intensity is empty.

What I did

  • I applied more error handling
  • I error now when get_geo() or get_carbon_intensity() fail. This was not in your code. Can you state why? What is the intention of proceeding with a None value?
  • I think I spotted a bug:
    • You precompute some values here:
      latitude, longitude = get_geo(client_ip)
    • Then you overload the values in case they are in the POST body here:
      # An ip has been given with the data. Let's use this:
    • However I believe different to your intention if some frame in the POST body does not have the IP key set it will not fallback to the pre-set carbon_intensity at the beginning of the function but rather use the value from the last frame in the POST data. This I believe is a bug. Can you confirm?
    • What I implemented is a LRU caching. Through @cache I call the functions in an else block and thus achieve that the API is not constantly polled again for identical submissions. The @cache should only be valid for the request and no longer.
  • Furthermore I do not fail the whole /v1/ci/measurement/add call when CarbonDB fails, but rather return a HTTP 207. What do you think of this solution?

@ArneTR ArneTR requested a review from ribalba September 15, 2024 09:32
Copy link

github-actions bot commented Sep 15, 2024

Old Energy Estimation

Eco-CI Output:

Label 🖥 avg. CPU utilization [%] 🔋 Total Energy [Joules] 🔌 avg. Power [Watts] Duration [Seconds]
Total Run (incl. overhead) 24.3606 1600.06 3.91 409
Measurement #1 24.3124 1600.06 3.93 407

🌳 CO2 Data:
City: Chicago, Lat: 41.8874, Lon: -87.6318
IP: 172.214.173.177
CO₂ from energy is: 0.577621660 g
CO₂ from manufacturing (embodied carbon) is: 0.116693347 g
Carbon Intensity for this location: 361 gCO₂eq/kWh
SCI: 0.694315 gCO₂eq / pipeline run emitted

@ArneTR ArneTR force-pushed the carbondb-ip-erroring branch from aaecc33 to d2e1769 Compare September 16, 2024 11:02
@ArneTR ArneTR changed the base branch from main to authentication-token September 16, 2024 11:02
Copy link

Eco-CI Output:

Label 🖥 avg. CPU utilization [%] 🔋 Total Energy [Joules] 🔌 avg. Power [Watts] Duration [Seconds]
Total Run (incl. overhead) 24.2423 1883 3.91 481
Measurement #1 24.1963 1883 3.93 479

🌳 CO2 Data:
City: Chicago, Lat: 41.8874, Lon: -87.6318
IP: 172.183.54.64
CO₂ from energy is: 0.708008000 g
CO₂ from manufacturing (embodied carbon) is: 0.137235942 g
Carbon Intensity for this location: 376 gCO₂eq/kWh
SCI: 0.845244 gCO₂eq / pipeline run emitted

@ArneTR
Copy link
Member Author

ArneTR commented Sep 27, 2024

@ribalba Can you please still review.

Since the code is live for 2 weeks now I will merge in for now, but still need your feedback here

@ArneTR ArneTR merged commit 9cc6c75 into authentication-token Sep 27, 2024
4 checks passed
@ArneTR ArneTR deleted the carbondb-ip-erroring branch September 27, 2024 11:39
ArneTR added a commit that referenced this pull request Sep 27, 2024
* First version

* Added daily to allowed schedule modes for default user

* REmoving user_id from object repr

* SQL typo

* Fixed timeout tests

* Database is now reloaded from structure file instead of truncate

* Authenticate now returns user obj instead of just ID

* Updated diff test signature

* Using TRUNCATE CASCADE to clear DB

* DELETE script for retention expired

* Implemented measurement quota with many tests and refactorings

* Removed noise

* Email adding was not possible without user_id

* migration needs to be wrapped around [skip ci]

* user_id added to hog, ci and carbond

* CarbonDB user_id column [skip ci]

* Adding user_id to structure

* Added more JOINs for delete [skip ci]

* Added machine to error in client.py [skip ci]

* Run-ID link and class name added to errors

* Run-ID Link only of not empty [skip ci]

* Authentication token is now not DEFAULT in frontend. Only in API. Added tests

* Added no-transform class

* Guard claused import_csv

* Added comment

* Raising errors and applying caching (#896)
@ribalba
Copy link
Member

ribalba commented Sep 30, 2024

  • I didn't want to fail if the geo_ip stuff fails as this could always be a network issue or something. But we can also fail everything if this happens.
  • I don't understand "However I believe different to your intention if some frame in the POST body does not have the IP key set it will not fallback to the pre-set carbon_intensity at the beginning of the function but rather use the value from the last frame in the POST data. This I believe is a bug. Can you confirm?"
    What I want is use the ip based data if there is nothing specified in the post. Not quite sure why this would fail.
  • I like the 207 solution

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

Successfully merging this pull request may close these issues.

2 participants