Climate-REF / Climate-REF/climate-ref

reingest: use latest_successful (pre-rank) so recovery targets the last good run

Open
#782 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.