Repository navigation
ENH: New webpoda data gap checker - #354
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Repoint lookup, upload-failure cleanup, local-file deletion, and outdated tests introduce data-loss and execution risks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds WebPODA gap detection and versioned L0 generation as groundwork for a future Dagster asset.
Changes:
- Compares WebPODA packets with production L0 files.
- Supports daily and repoint-based data grouping.
- Extends the CLI to select the appropriate workflow.
File summaries
| File | Description |
|---|---|
imap_data_access/webpoda.py |
Adds repoint lookup, comparison, versioning, and upload logic. |
imap_data_access/cli.py |
Routes instruments through daily or repoint-based downloads. |
Review details
Suppressed comments (4)
imap_data_access/webpoda.py:395
- The renamed parameter is not reflected in
tests/test_webpoda.py:177and:195, which still call this function withrepointing_file=...; both tests will now fail withTypeErrorbefore exercising the implementation. Update those tests to parse the fixture and passrepoint_data, along with coverage for the new comparison paths.
repoint_data: list,
imap_data_access/webpoda.py:492
- As in the daily path, this deletes the generated file even when
_upload_if_requestedhas caught an upload failure. Keep the local file unless upload success is explicitly communicated, so the recovered packets remain retryable.
for path in file_paths:
_upload_if_requested(path, upload_to_server)
# clean up files after upload to avoid filling up disk space
path.unlink()
imap_data_access/webpoda.py:790
imap_data_access.downloadcan return an already-existing local file without downloading it, so this may delete a production L0 file that the user had before running the checker. Only remove files whose temporary ownership is known; the safe behavior here is to remove the newly generated duplicate and leave the production path intact.
# Clean up the new file since it is a duplicate of the production file
new_path.unlink()
prod_l0_path.unlink()
imap_data_access/webpoda.py:452
- The documented return type is a list, and all other no-result paths return an empty list. Returning
Nonehere forces callers to handle an undocumented second container type; return the initialized empty list instead.
if not repoints:
logger.info(
f"No repoint IDs found for instrument [{instrument}] between "
f"{start_time} and {end_time}. Skipping."
)
logger.info("-" * 80)
return None
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tmplummer
left a comment
There was a problem hiding this comment.
I have an overall design question. These changes completely remove the use of ERT in all queries. What is the plan with the Dagster jobs? How will they be triggered and how will they be configured? i.e. what date range will be queried?
I don't have data to back it up, but IMO, the ENA L0 download has been working well and has some advantages. We have two approaches to download data:
Precision Approach:
- Trigger off of some product that we get from each contact. For ENAs, this has been repoint table.
- Query for packets with ERT in time range since last precision query.
- From ERT query, build list of dates/repoints to re-query over based on SCT.
Pros:
- We only download those dates/repoints that we got new data for.
- Triggered off of data downlink. This means that we will download data as soon as it is available.
Cons:
- Relies on POC ingest being completed before our queries run. This is non guaranteed when triggering on repoint table.
Brute Force Approach:
- Run based on cron type schedule
- Query packets with SCT that fall in last N days
- Check downloaded data against existing data and upload files that have changed.
Pros:
- If we miss data or POC is in process of ingesting data during one query, next time the job is run, we will catch that data.
Cons:
- We end up downloading lots of extra data. I don't know if this is an issue for the POC.
- Data is not retrieved as soon as it is available.
- Potential to retrieve data while POC is in the middle of an ingest which means extra reprocessing jobs because data from a single downlink will get split across multiple download jobs.
I am hesitant to abandon the Precision Approach all together because it seems like a more elegant approach to me. I do see value in the Brute Force Approach as a backup that gets run every so often. As I was writing the pros/cons, I was wondering if we could have the POC either deliver an additional "product" to the SDC or hit an API with detailed info about what they just ingested. For example, a JSON file with a list of all unique APIDs that were ingested and the ERT range they belong to as well as the unique SCT dates.
|
These were the assumptions I was working with for this work
|
|
Path forward from the meeting: Keep what's in imap-data-access as it is. Add these supports:
Then in dagster, create sensor to check historical data gaps:
|
There was a problem hiding this comment.
🟡 Changes recommended
Version-zero files can bypass comparison, and several date-range behaviors can select stale or out-of-range data.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Balanced
|
I made update based on outcome from the meeting. Then tested the new refactor changes with same Output: For in-situ, I double checked that it's still working. Output: Output: |
tmplummer
left a comment
There was a problem hiding this comment.
Mostly looks good. There are many print statements that I think should be logger calls. I'm also not sure if checking file size should be strictly less than check. New files being smaller shouldn't happen, but maybe is happening due to POC cleaning up duplicate packets?
tmplummer
left a comment
There was a problem hiding this comment.
LGTM, thanks for seeing this through.
e7a2f0c
into
IMAP-Science-Operations-Center:main
Change Summary
Related to #1617
Overview
This checks data gaps by comparing what exists in prod and what exists in WebPODA. This will be turned into Dagster asset in future PR. This is tested locally against prod and WebPODA.
File changes
Testing