feat(cabq): cabq backfill implementation - #48
Conversation
…proved error logging in backfill prepare step
|
Your pull request is automatically being deployed to Dagster Cloud.
|
| """ | ||
| cfg = load_source_config("cabq") | ||
| client = build_cabq_client(cfg["api_base_url"]) | ||
| try: |
There was a problem hiding this comment.
The bare raise here is fragile and loses the underlying error info in the common case where _fetch_locations returns (None, err) without raising. In that case the bare raise produces RuntimeError: No active exception to reraise and err is discarded — confirmed by running it directly.
Minimal fix — fold the locations is None check into the same try, so the existing except Exception: client.close(); raise handles both cases (an actual raised exception, and this returned-None case) with one close() call instead of two, and the real err reason included:
try:
locations, err = _fetch_locations(client)
if locations is None:
raise RuntimeError(f"Backfill fetch failed to receive locations: {err}")
except Exception:
client.close()
raise
There was a problem hiding this comment.
adjusted the error logging as suggested
| def _fetch_readings() -> httpx.Response: | ||
| # query for location code = given location id for measurement info | ||
| return client.get( | ||
| query = ( |
There was a problem hiding this comment.
CABQ's _fetch_readings_for_location builds the encoded query string by hand; hydrovu uses httpx's params (client.get(path, params=...)). This divergence is the issue — please switch CABQ to use params= for safety and consistency.
Note: tests/sources/cabq/test_dlt_pipeline.py assert the exact encoded URL string and will need to be updated to check parsed params (e.g. via httpx.URL(...).params) instead of raw string equality.
There was a problem hiding this comment.
has been addressed, and I updated the call for fetch locations to do the same
There was a problem hiding this comment.
Found and fixed one more issue in the params= migration — the query still joined clauses with a literal + (leftover from when the URL was hand-built), which params= now encodes as a literal %2B instead of a space. This broke every real query with a 400 from the API (and crashed with an unhandled KeyError: 'features' instead of a clean error, since the API returns HTTP 200 with an error body).
This wasn't backfill-only — _fetch_readings_for_location is also called from raw_cabq_readings, the daily production ingest asset, so this would have broken the daily pipeline too.
Fixed by joining clauses with real spaces instead of +.
Before merging this PR please Verify:
- Direct call against the live API — returns real data, no more KeyError
- Full cabq_backfill_refetch job run — succeeds
- Full daily asset chain (raw_cabq_readings → canonical_bundles_cabq → frost_load_cabq) — succeeds end-to-end
- Tests/ruff/mypy all clean
There was a problem hiding this comment.
ah, fair enough, honestly thought that the + was still needed. Tested the changes locally and can confirm that everything is working.
…et instead of building url manually, further adjustment to prepare backfill error logging
…get instead of building url manually
likithabommasani21
left a comment
There was a problem hiding this comment.
Looks good to me!
Verify all the CABQ pipelines works as excepted in dagster before merging
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
cabq/backfill.py.cabq/dlt_pipeline.pyto query based on a time range.shared/backfill.pyto enable a source to use different value types as location ids instead of just int.Notes
Any special considerations, workarounds, or follow-up work to note?