fix: raise geolocation API timeout and retry the primary API once - #1384
Open
NoiceHax wants to merge 2 commits into
Open
fix: raise geolocation API timeout and retry the primary API once#1384NoiceHax wants to merge 2 commits into
NoiceHax wants to merge 2 commits into
Conversation
The geolocation lookup used a hardcoded 0.5 second timeout, which is short enough that several trackers starting at the same time push each other past it. The primary API then looks dead and we fall through to the backup one, or to the hardcoded Canada default, which gives the whole run the wrong carbon intensity. Move the timeout into a GEO_API_TIMEOUT constant set to 5 seconds, following the ELECTRICITYMAPS_API_TIMEOUT pattern, and retry the primary API once. The retry only covers timeouts and connection errors, so a reply that parses badly still goes straight to the backup as before.
test_carbon_tracker_timeout asserted the geolocation lookup made exactly two requests, one per API. The primary API is now retried once, so the count is one higher.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The geolocation lookup used a hardcoded 0.5 second timeout for the primary API (get.geojs.io) and for the backup one (ipinfo.io). Half a second is a small budget for a network call, so a healthy API can miss it and codecarbon drops to the backup, or to the hardcoded Canada default when the backup is slow too.
The timeout now lives in a
GEO_API_TIMEOUTconstant set to 5 seconds, following the same pattern asELECTRICITYMAPS_API_TIMEOUTincodecarbon/core/electricitymaps_api.py. The primary API also gets one retry, limited to timeouts and connection errors, so a reply that arrives but does not parse still goes to the backup like before. The Canada default is untouched, its warning now says plainly that emissions will be computed with the Canadian carbon intensity.Related Issue
Fixes #854
Motivation and Context
The reporter started eight trackers in parallel and six of them logged "Unable to access geographical location through primary API". Sleeping up to 10 seconds between processes was the workaround. With half a second to answer, a handful of trackers hitting the same API at once is enough to miss the deadline, and the wrong country means the wrong carbon intensity for the whole run.
How Has This Been Tested?
I ran the package suite on Windows with
CODECARBON_ALLOW_MULTIPLE_RUNS=True pytest -m "not integ_test" tests/, and 614 of the 615 collected tests pass. The one failure,test_task_energy_with_live_update_interference, fails the same way on master on this machine: it mocks CPU and RAM but not the GPU, and this laptop has a real NVIDIA card whose energy lands in the total. Three test modules need optional dev packages I could not install here, so they were left out of the run.I added two tests in
tests/test_geography.pyand checked that both fail before the change. One has the primary API time out on the first call and answer on the second, asserting the backup is never called. The other asserts the request goes out with the constant instead of 0.5. I also bumped the expected request count intest_carbon_tracker_timeout, which counted the calls made when both APIs time out. black and isort are clean on the changed files.Screenshots (if appropriate):
Not applicable.
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:AI Usage Disclosure
Please refer to docs/how-to/ai-policy.md for detailed guidelines on how to disclose AI usage in your PR. Accurately completing this section is mandatory.
An AI coding agent wrote this patch and this description under my direction, and I read every line, ran the tests myself and can explain the change.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.