fix: fallback to backup API when geojs returns empty region#1103
fix: fallback to backup API when geojs returns empty region#1103ucodia wants to merge 2 commits intomlco2:masterfrom
Conversation
benoit-cty
left a comment
There was a problem hiding this comment.
Thanks Lionel for improving CodeCarbon !
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1103 +/- ##
==========================================
+ Coverage 78.22% 78.23% +0.01%
==========================================
Files 38 38
Lines 3632 3634 +2
==========================================
+ Hits 2841 2843 +2
Misses 791 791 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@benoit-cty after testing this in my production environment I discovered a few issues with the existing fallback mechanism. Issue 1The fallback API will always fail when using the HTTPS endpoint Using the HTTP endpoint is not great because this could expose the application location to eavesdropper and be vulnerable to man-in-the-middle. Issue 2If the fallback API call fails, it reports on the wrong URL on the exception: https://github.com/ucodia/codecarbon/blob/ebc8d1a3f5dcae9013dcbe6883d05e711ad6ce28/codecarbon/external/geography.py#L136 ProposalAfter doing some research I found that
Similarly to previous fallback provider it does not provide the 3 letter country code but instead of calling yet another API, I suggest we add a dependency on If you are ok with the suggested new path, I can close this PR and open a new issue to properly track this. |
|
@benoit-cty created a new PR for the proposed changes and updated the original issue with context: #1104 |
Description
Updated
GeoMetadata.from_geo_js()to handle responses where thegeojsAPI returns a successful HTTP 200 response but with an empty region field. When this happens, it now logs a warning and falls through to the existingip-api.combackup chain instead of returning incomplete metadata.Related Issue
Resolves #1101
Motivation and Context
For certain IP ranges,
get.geojs.ioreturns a successful lookup but with no region field. Because no exception was raised, from_geo_js() previously returned this incomplete data immediately. Downstream, this causedget_region_emissions()to perform lookups with an empty string, triggeringKeyError: ''and falling back to country-level emissions with incorrect geographic coordinates logged.How Has This Been Tested?
GEO_METADATA_CANADA_EMPTY_REGIONtest fixtures in tests/testdata.py.uv run pytest tests/test_geography.py -vsuccessfully.Screenshots (if appropriate):
N/A
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist:
Go over all the following points, and put an
xin all the boxes that apply.