Climate-REF / Climate-REF/climate-ref
reingest: use latest_successful (pre-rank) so recovery targets the last good run
- Dominant language
- Python
- Stars
- 26
- Forks
- 14
- Avg merge
- 1d 10h
- Merged PRs (30d)
- 36
Description
## Summary
`reingest.py` should reingest each group's latest *successful* execution, not skip groups whose newest run happens to have failed. Now that `get_execution_group_and_latest_filtered` exposes a pre-rank `latest_successful` knob, the recovery path can ask the right question.
## Background
There are two distinct "latest execution" questions, now both available on `get_execution_group_and_latest_filtered`:
- `successful` (post-rank): "is the *latest* run successful?" — keeps a group only if its newest execution succeeded.
- `latest_successful` (pre-rank): "what is the latest *successful* run?" — ranks over successful executions only, so a group whose newest run failed but succeeded earlier is still returned, pointing at that earlier success.
## The reingest case
`get_reingest_targets` (`packages/climate-ref/src/climate_ref/executor/reingest.py:400`) currently passes `successful=None if include_failed else True`, i.e. the **post-rank** filter. For a group whose newest run failed after an earlier success, this drops the group entirely — even though a good bundle exists to reingest from. A recovery/reingest caller almost certainly wants the **pre-rank** `latest_successful=True` instead, so it recovers the last good run rather than skipping the group.
## Proposed change
Switch the `include_failed=False` path from `successful=True` to `latest_successful=True`:
```python
results = get_execution_group_and_latest_filtered(
database.session,
diagnostic_filters=diagnostic_filters,
provider_filters=provider_filters,
latest_successful=None if include_failed else True,
include_superseded=True,
)
```
## Why it wasn't done in the same change
This alters recovery/reingest behavior (which groups get reprocessed and from which execution), so it deserves its own PR with a focused test rather than riding along with the read-only primitive refactor that introduced `latest_successful`. The downstream logic in `get_reingest_targets` already re-checks `oldest.successful` and selects `eg.executions[0]`, so the interaction with the pre-rank population needs a deliberate test (e.g. a group with success -> fail -> and confirm it now reingests the good run).
## Acceptance criteria
- [ ] `include_failed=False` reingest targets a group whose latest run failed but which has an earlier successful run.
- [ ] `include_failed=True` behavior is unchanged.
- [ ] Regression test covering the success -> fail sequence.
Contributor guide
Research direction
Start in packages/climate-ref/src/climate_ref/executor/reingest.py at get_reingest_targets and read the get_execution_group_and_latest_filtered options it calls. Inspect the existing reingest tests and add coverage for a success → fail execution sequence. Done means include_failed=False recovers the earlier successful run, while include_failed=True remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 75/100