feat(sequences): re-run cone matching when relabeling back to wildfire_smoke#585
feat(sequences): re-run cone matching when relabeling back to wildfire_smoke#585MateoLostanlen wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 92.30% 92.31% +0.01%
==========================================
Files 56 56
Lines 2585 2603 +18
==========================================
+ Hits 2386 2403 +17
- Misses 199 200 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
It's an interesting PR, thx! I can do a thorough review by Monday if it's ok for you. I want to make sure I understand what happens with the triangulation and how alerts/sequences/detections concepts are linked :) |
Resolve label_sequence conflict by combining both changes: keep main's sibling-check for the non-wildfire path (skip detach/recreate when the sequence is alone in its alert, avoiding alert-id churn) and add this branch's revert-to-wildfire path (detach + re-run cone matching anchored on the sequence's own last_seen_at). Keep both unit tests.
|
@MateoLostanlen I'll review by EOD |
Acruve15
left a comment
There was a problem hiding this comment.
Hey @MateoLostanlen, I added some comments, the most important one on how to handle orphan alerts :)
Can be helpful, here is the current flow for context:
sequenceDiagram
autonumber
participant U as User
participant L as label_sequence
participant CM as cone matching
participant A as AlertSequence
Note over U,A: T0: Alert#100 triangulates {seq_B, seq_C}
U->>L: label seq_B OTHER_SMOKE
L->>A: detach seq_B from Alert#100
L->>A: create lonely Alert#101 = {seq_B}
Note over A: Alert#100 = {seq_C}, Alert#101 = {seq_B}
Note over U,A: T1 (later): user reconsiders
U->>L: label seq_B WILDFIRE_SMOKE
rect rgb(255, 235, 235)
Note over L,A: pre-PR: no-op, seq_B stuck in Alert#101 forever
end
rect rgb(235, 250, 235)
L->>A: detach seq_B from Alert#101 (deleted, empty)
L->>CM: anchor on seq_B.last_seen_at
alt overlap found
CM-->>A: link seq_B to Alert#100
Note over A: Alert#100 = {seq_B, seq_C} again
else no overlap (orphan risk)
CM-->>A: returns None, no link created
Note over A: seq_B has zero alert rows
end
end
Longer term: I was thinking we could soft-delete AlertSequence rows (with unlinked_at timestamps) would give us audit history for free and is useful when debugging "why did this alert lose triangulation?"?
| camera = cast(Camera, await cameras.get(sequence.camera_id, strict=True)) | ||
| # Anchor the candidate window on the sequence's own time so old relabels still merge | ||
| await attach_sequence_to_alert( | ||
| updated, camera, cameras, sequences, alerts, reference_time=sequence.last_seen_at |
There was a problem hiding this comment.
For consistency?
| updated, camera, cameras, sequences, alerts, reference_time=sequence.last_seen_at | |
| updated, camera, cameras, sequences, alerts, reference_time=updated.last_seen_at |
| await _detach_sequence_from_alerts(sequence_id, session, alerts) | ||
| camera = cast(Camera, await cameras.get(sequence.camera_id, strict=True)) | ||
| # Anchor the candidate window on the sequence's own time so old relabels still merge | ||
| await attach_sequence_to_alert( |
There was a problem hiding this comment.
if this returns None (eg no neighbour in the window or cones don't overlap?) , the sequence has zero alert links. I suggest adding a fallback to make sure we don't have orphan alters.
alert_id = await attach_sequence_to_alert(
updated, camera, cameras, sequences, alerts, reference_time=sequence.last_seen_at
)
if alert_id is None:
new_alert = await alerts.create(
AlertCreate(
organization_id=camera.organization_id,
started_at=sequence.started_at,
last_seen_at=sequence.last_seen_at,
lat=None,
lon=None,
)
)
session.add(AlertSequence(alert_id=new_alert.id, sequence_id=sequence_id))
await session.commit()If orphaning is the intended behaviour, a one-line comment is enough for me.
other_smoke), it's detached from its triangulating alert and a fresh single-sequence alert is created. Reverting the label towildfire_smokepreviously left the sequence stuck in that lonely alert, breaking triangulation. This PR makes the revert path drop the lonely alert and re-run cone matching so the sequence merges back with its overlapping neighbours.last_seen_at(notutcnow()) so relabels done hours later still find the original neighbours withinSEQUENCE_RELAXATION_SECONDS._attach_sequence_to_alert→attach_sequence_to_alertand extract_detach_sequence_from_alertsto deduplicate the detach/refresh block.