feat(elt-pipelines): Port electricity_sharepoint pipeline to elt#403
feat(elt-pipelines): Port electricity_sharepoint pipeline to elt#403WHTaylor wants to merge 6 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an ETL extractor for electricity exports from SharePoint, supporting configurable backfills, watermark filtering, concurrent downloads, CSV/XLSX parsing, UTC normalisation, combined DataFrame output, and Arrow table emission. ChangesElectricity SharePoint ingestion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/electricity_sharepoint.py (1)
132-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo per-file error isolation in the parsing loop.
If
read_as_dataframeraises for any single file (unsupported extension, or an unhandled exception fromparsing.py), the whole loop aborts and no data is emitted for the run, including files that parsed fine before the failure. Consider wrapping the per-file call in a try/except that logs and skips the offending file, as a defence-in-depth complement to hardening the individual parsers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/electricity_sharepoint.py` around lines 132 - 138, Update the per-file parsing loop around read_as_dataframe to catch exceptions for individual files, log the file-specific failure, and continue processing subsequent files. Preserve the existing df_batch concatenation for successfully parsed DataFrames and return any accumulated results instead of aborting the entire run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/electricity_sharepoint.py`:
- Around line 71-96: Update _extract_electricity_usage after
read_contents_to_dataframe(resolved) to handle a None result before calling
pa.Table.from_pandas. When no files are available or none parse successfully,
yield the pipeline’s appropriate empty PyArrow table; otherwise preserve the
existing DataFrame conversion for valid results.
In `@elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/parsing.py`:
- Around line 116-118: Replace the bare structure assertions in
read_power_consumption_csv with explicit validation that handles malformed
sections locally: verify the CSV has exactly three columns and that the third
column name contains “power”, then skip or return the section using the same
recoverable behavior as the DST branch. Ensure failures do not raise uncaught
AssertionError or terminate read_contents_to_dataframe’s batch loop, and keep
validation active regardless of Python optimization.
- Around line 53-60: Update read_power_consumption_excel to handle pandas DST
ambiguous/nonexistent-time ValueError from _to_utc consistently with
_csv_section_to_df: catch the localization error, log or skip the affected Excel
file using the existing error-handling path, and return the same failure
representation expected by read_contents_to_dataframe so one invalid file does
not abort the batch.
---
Nitpick comments:
In
`@elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/electricity_sharepoint.py`:
- Around line 132-138: Update the per-file parsing loop around read_as_dataframe
to catch exceptions for individual files, log the file-specific failure, and
continue processing subsequent files. Preserve the existing df_batch
concatenation for successfully parsed DataFrames and return any accumulated
results instead of aborting the entire run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2d91bdb-9b20-4fb0-aca8-5e526b65de91
⛔ Files ignored due to path filters (1)
elt-pipelines/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
elt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/electricity_sharepoint.pyelt-pipelines/facility_ops/ingest/estates/electricity_sharepoint/parsing.pyelt-pipelines/pyproject.toml
1 participant
ref #321
Ports the
electricity_sharepointpipeline toelt-pipelines.ELECTRICITY_SHAREPOINT__CLIENT_ID,ELECTRICITY_SHAREPOINT__CLIENT_SECRET, andELECTRICITY_SHAREPOINT__TENANT_IDneed to be set with the sharepoint client details (I've been testing with the ones from vault), and thesharepointoptional-dependencies group needs to be installed.To backfill, set
ELECTRICITY_SHAREPOINT__BACKFILL=True, and optionally setELECTRICITY_SHAREPOINT__BACKFILL_GLOBSto only pick up specific files.Most of it is copied directly from the previous pipeline, with these changes:
qadb the excel files have read their data into anisis_elec_total_power_mwxcolumn, whereas the csvs read it intoisis_elec_total_power_mw. With this version of the pipeline, everything is read into the latter.I've also combined this pipelines optional dependencies with the ones for the other sharepoint pipeline added in #394, as I figure they'll always be the same (reading from sharepoint, and reading from excel files).
I'm not totally happy with the way watermarking is handled. The watermark is set based on the
date_timecolumn from the data, and is used as a threshold for themodified_aftertime on the files to read, but then because the files are uploaded some time after the last measurement they contain this means we keep rereading the files.Fixing this will require implementing some way for pipelines to specify how watermarks should be set, rather than always being the max value of a column in the data. Don't think it's high priority, but probably a nice-to-have.