-
Notifications
You must be signed in to change notification settings - Fork 1k
[AR parser] Add preventive error handling for missing production data #8073
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
base: master
Are you sure you want to change the base?
[AR parser] Add preventive error handling for missing production data #8073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds preventive error handling to the AR production parser to ensure that missing or invalid production data is caught early and handled appropriately.
- Adds enhanced error handling (timeouts, try/except blocks) for both renewables and non-renewables production data retrieval.
- Improves logging for debugging by reporting errors and warnings, and raises ParserException when necessary.
Comments suppressed due to low confidence (2)
parsers/CAMMESA.py:118
- The ParserException's 'parser' field is set to 'AR.py', which may be confusing since this file is named 'CAMMESA.py'. Consider using a consistent identifier that matches the file name to simplify debugging.
raise ParserException(parser="AR.py", message=f"Renewables API error: {str(e)}", zone_key=zone_key)
parsers/CAMMESA.py:152
- Catching a KeyError during the production record parsing only logs a warning and skips the record, which may lead to incomplete production data. Consider implementing a more robust handling strategy or accumulating and reporting these cases to ensure data integrity.
logger.warning(f"[AR Renewables] Missing expected field {e} in production info: {production_info}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see how this solves the problems you describe. If anything it creates more points of failure and complexity as is.
Would love to discuss it more though.
production=ProductionMix( | ||
biomass=production_info.get("biocombustible", 0.0), | ||
hydro=production_info.get("hidraulica", 0.0), | ||
solar=production_info.get("fotovoltaica", 0.0), | ||
wind=production_info.get("eolica", 0.0), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would do the opposite of your intentions here, if we set it to 0 if it's missing there is no way to know if it's actually missing or just 0 when we just look at the data so this we cant do.
api_cammesa_response = session.get( | ||
CAMMESA_DEMANDA_ENDPOINT, params=params, timeout=10 | ||
) | ||
api_cammesa_response.raise_for_status() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why raise for status instead of simply checking if the response is okay? with api_cammesa_response.ok
and directly raise the parser exception. that way we can avoid the try except statement that adds additional overhead.
) | ||
production_year = production_datetime.year | ||
|
||
# use the split from EIA 2022 to split the termico production into gas and oil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this comment removed?
logger.info( | ||
f"[AR Non-Renewables] Successfully retrieved {len(production_list)} production records." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create too many logs, please remove it.
hydro=production_info.get("hidraulico", 0.0), | ||
nuclear=production_info.get("nuclear", 0.0), | ||
gas=production_info.get("gas", 0.0), | ||
oil=production_info.get("oil", 0.0), | ||
coal=production_info.get("coal", 0.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as above.
except KeyError as e: | ||
logger.warning( | ||
f"[AR Renewables] Missing expected field {e} in production info: {production_info}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will only work for momento
now as the others are accessed used get which means it won't throw any errors.
Summary
ParserException
early if data is missing, preventing silent failures.Motivation
Although the AR parser is currently working and Issue #8054 has been closed,
this PR aims to prevent future incidents where missing or invalid production data could silently pass undetected.
By ensuring the parser fails fast and clearly when data is missing,
the ElectricityMap server can detect problems immediately instead of misinterpreting the parser as healthy.
This enhances the robustness and observability of the parser and helps maintain reliable data quality over time.
Changes
renewables_production_mix()
andnon_renewables_production_mix()
to:ParserException
if production data is missing or invalid.Related Issues
Checklist
poetry run test_parser AR production
locally and verified correct outputs.ParserException
when API returns empty or invalid data.