Add per-zone max_age overrides for tracker lifecycle#76
Add per-zone max_age overrides for tracker lifecycle#762024itb047samata wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTracker.update() now centers each bbox to get zones, sorts zones by a fixed ZONE_PRIORITY before emitting/saving, and refactors the single-frame lost-track lookup into a multiline next(...) call; memory imports were reformatted and a new get_active_track_ids method was inserted in MemoryStore. ChangesTrack lifecycle and ReID state management
Sequence Diagram(s)sequenceDiagram
participant DeepSort
participant Tracker
participant ZoneService
participant EmissionService
participant MemoryStore
DeepSort->>Tracker: confirmed track (bbox, id, features)
Tracker->>ZoneService: get_zones_for_point(cx, cy)
ZoneService-->>Tracker: zone list
Tracker->>Tracker: sort zones by ZONE_PRIORITY
Tracker->>EmissionService: emit TrackState.BORN (zones, dwell)
Tracker->>MemoryStore: persist/refresh active track entry
Note over Tracker,DeepSort: on disappearance (frames_since == 1)
Tracker->>DeepSort: next(...) lookup for matching track
DeepSort-->>Tracker: track/features or None
Tracker->>MemoryStore: store embedding in _lost_embeddings
Tracker->>EmissionService: emit TrackState.LOST
Tracker->>Tracker: when age >= max_age -> emit TrackState.DEAD and cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/tracking/tracker.py (2)
295-309:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDedent cleanup and
return TrackedFrame(...)out of the LOST branch.With the current indentation,
update()returns after the first disappeared track and falls off the end entirely on frames where nothing is lost.Suggested fix
- expired_ids = [ - tid for tid, data in self._lost_embeddings.items() - if self._frame_id - data["last_seen"] > self.max_age - ] - - for tid in expired_ids: - del self._lost_embeddings[tid] - return TrackedFrame( + expired_ids = [ + lost_tid for lost_tid, data in self._lost_embeddings.items() + if self._frame_id - data["last_seen"] > self.max_age + ] + + for lost_tid in expired_ids: + del self._lost_embeddings[lost_tid] + + return TrackedFrame( frame_id = self._frame_id, camera_id = self.camera_id, tracks = tracked_objects, timestamp_ms = time.time() * 1000, fps = self.fps, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/tracking/tracker.py` around lines 295 - 309, The return of TrackedFrame is accidentally inside the "LOST" branch causing update() to exit early; move the return out of that block so it always executes. Specifically, in update() adjust the indentation so that the cleanup of self._lost_embeddings (building expired_ids and deleting them) completes, then the function returns the TrackedFrame constructed with frame_id=self._frame_id, camera_id=self.camera_id, tracks=tracked_objects, timestamp_ms=time.time()*1000, fps=self.fps outside the LOST branch, ensuring expired_ids/max_age logic runs but does not cause an early return.
341-354:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMove
_cosine_similarityinside theTrackerclass.It is currently defined at module scope (line 341), so calling
self._cosine_similarity(...)at line 140 will raiseAttributeErrorat runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/tracking/tracker.py` around lines 341 - 354, The helper function _cosine_similarity is currently defined at module scope but is referenced as an instance method (self._cosine_similarity) from the Tracker class; move its definition inside the Tracker class body (as a private instance method) so it becomes Tracker._cosine_similarity and can be called via self._cosine_similarity(a, b). Ensure the signature and logic remain the same (parameters a: np.ndarray, b: np.ndarray, return float) and update any imports/indentation accordingly so it is indented as a method of class Tracker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/tracking/tracker.py`:
- Around line 261-281: The code incorrectly uses only prev_obj.zones_present[0]
to pick a max_age_override; instead, look up all zones referenced in
prev_obj.zones_present against DEFAULT_ZONES, collect their max_age_override
values, and compute effective_max_age by an explicit precedence rule (prefer a
zone.priority attribute if present—select the zone with highest priority—or if
no priority field exists, choose the most restrictive override e.g., the
smallest non-None max_age_override); replace the single-zone lookup block around
prev_obj.zones_present, DEFAULT_ZONES, zone.max_age_override and
effective_max_age with this aggregation+precedence logic so overrides are
consistently applied regardless of list order.
- Around line 252-290: After emitting TrackState.DEAD you must remove the track
id from the track registry so it doesn't repeatedly re-enter the LOST/DEAD
branch; locate the block where frames_since > effective_max_age triggers
self._emit_lifecycle(TrackState.DEAD, tid, ...) and after that call remove the
track from self._active_tracks (and any related per-track state if applicable)
keyed by tid so the dead track is no longer considered on subsequent frames
(ensure you reference tid and _active_tracks and keep the _emit_lifecycle call
intact).
---
Outside diff comments:
In `@services/tracking/tracker.py`:
- Around line 295-309: The return of TrackedFrame is accidentally inside the
"LOST" branch causing update() to exit early; move the return out of that block
so it always executes. Specifically, in update() adjust the indentation so that
the cleanup of self._lost_embeddings (building expired_ids and deleting them)
completes, then the function returns the TrackedFrame constructed with
frame_id=self._frame_id, camera_id=self.camera_id, tracks=tracked_objects,
timestamp_ms=time.time()*1000, fps=self.fps outside the LOST branch, ensuring
expired_ids/max_age logic runs but does not cause an early return.
- Around line 341-354: The helper function _cosine_similarity is currently
defined at module scope but is referenced as an instance method
(self._cosine_similarity) from the Tracker class; move its definition inside the
Tracker class body (as a private instance method) so it becomes
Tracker._cosine_similarity and can be called via self._cosine_similarity(a, b).
Ensure the signature and logic remain the same (parameters a: np.ndarray, b:
np.ndarray, return float) and update any imports/indentation accordingly so it
is indented as a method of class Tracker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db568d0d-a304-4bab-9ac3-233af32f10a4
📒 Files selected for processing (2)
services/detection/zones.pyservices/tracking/tracker.py
| if prev_obj.zones_present: | ||
|
|
||
| zone_name = prev_obj.zones_present[0] | ||
|
|
||
| from services.detection.zones import DEFAULT_ZONES | ||
|
|
||
| zone = next( | ||
| ( | ||
| z for z in DEFAULT_ZONES | ||
| if z.name == zone_name | ||
| ), | ||
| None, | ||
| ) | ||
|
|
||
| if ( | ||
| zone | ||
| and zone.max_age_override is not None | ||
| ): | ||
| effective_max_age = ( | ||
| zone.max_age_override | ||
| ) |
There was a problem hiding this comment.
Don't derive the override from zones_present[0].
keypad_area is nested inside restricted_door, and get_zones_for_point() preserves DEFAULT_ZONES order. That means a track inside the keypad resolves to restricted_door first, so the 15-frame override never applies. Compute effective_max_age from all matching zones with an explicit precedence rule instead of relying on list order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/tracking/tracker.py` around lines 261 - 281, The code incorrectly
uses only prev_obj.zones_present[0] to pick a max_age_override; instead, look up
all zones referenced in prev_obj.zones_present against DEFAULT_ZONES, collect
their max_age_override values, and compute effective_max_age by an explicit
precedence rule (prefer a zone.priority attribute if present—select the zone
with highest priority—or if no priority field exists, choose the most
restrictive override e.g., the smallest non-None max_age_override); replace the
single-zone lookup block around prev_obj.zones_present, DEFAULT_ZONES,
zone.max_age_override and effective_max_age with this aggregation+precedence
logic so overrides are consistently applied regardless of list order.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/tracking/tracker.py (1)
255-264:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPer-zone
max_age_overridelogic is missing and zones schema needs extension.The code at line 255 only checks the global
self._tracker.max_agewithout any zone-specific override logic. To implement the PR objective of "dynamic, zone-specific expiration logic," you need to:
- Add
max_age_overridefield to the zone schema inconfig/zones.yaml- Update
zone_loader.pyto load and validate this optional field- Modify
tracker.pyline 255 to look up the zone-specific override fromprev_obj.zones_present[0]and fall back to the globalmax_ageif absentCurrently, zones are loaded with only:
name,polygon,alert_on_entry,color_hex. The zone-specific timeout feature is not implemented despite the merged feature branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/tracking/tracker.py` around lines 255 - 264, The tracker currently uses only the global timeout self._tracker.max_age when expiring tracks; implement per-zone overrides by (1) adding an optional max_age_override field to the zones schema in config/zones.yaml, (2) updating zone_loader.py to parse and validate max_age_override into each Zone object (keeping it optional and defaulting to None), and (3) changing the expiration logic in tracker.py (around the block using frames_since and self._tracker.max_age and referencing prev_obj.zones_present) to determine timeout = zone.max_age_override if present for prev_obj.zones_present[0] else self._tracker.max_age before comparing frames_since, then use that timeout for emitting DEAD lifecycle and deleting from _active_tracks/_active_embeddings; keep existing calls to _emit_lifecycle and logging intact.
🧹 Nitpick comments (1)
services/tracking/tracker.py (1)
189-193: ⚡ Quick winRemove duplicate dwell computation.
Lines 186–187 already compute
dwell_framesanddwell_secsidentically; the repeated assignment at 189–193 is dead code.♻️ Proposed fix
prev = self._active_tracks.get(tid) dwell_frames = (prev.dwell_time_frames + 1) if prev else 1 dwell_secs = dwell_frames / self.fps - dwell_frames = ( - prev.dwell_time_frames + 1 - ) if prev else 1 - - dwell_secs = dwell_frames / self.fps - prev_traj = prev.trajectory if prev else []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/tracking/tracker.py` around lines 189 - 193, The block duplicates the dwell computation: remove the redundant reassignment of dwell_frames and dwell_secs (the second occurrence that uses prev and self.fps) so only the first computation remains; locate the duplicate in tracker.py where dwell_frames and dwell_secs are assigned (references: variable names dwell_frames, dwell_secs, prev and attribute self.fps, within the tracking update logic/function) and delete the later repeated lines to avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/tracking/tracker.py`:
- Around line 212-215: In the loop where you create and register new
TrackedObject instances (you assign self._active_tracks[tid] = obj and
current_ids.add(tid)), also append each obj to the tracked_objects list so that
the resulting TrackedFrame.tracks is populated; specifically, add
tracked_objects.append(obj) immediately after setting self._active_tracks[tid] =
obj (using the existing variables tid, obj, current_ids, tracked_objects) so
downstream consumers receive the created tracks.
---
Outside diff comments:
In `@services/tracking/tracker.py`:
- Around line 255-264: The tracker currently uses only the global timeout
self._tracker.max_age when expiring tracks; implement per-zone overrides by (1)
adding an optional max_age_override field to the zones schema in
config/zones.yaml, (2) updating zone_loader.py to parse and validate
max_age_override into each Zone object (keeping it optional and defaulting to
None), and (3) changing the expiration logic in tracker.py (around the block
using frames_since and self._tracker.max_age and referencing
prev_obj.zones_present) to determine timeout = zone.max_age_override if present
for prev_obj.zones_present[0] else self._tracker.max_age before comparing
frames_since, then use that timeout for emitting DEAD lifecycle and deleting
from _active_tracks/_active_embeddings; keep existing calls to _emit_lifecycle
and logging intact.
---
Nitpick comments:
In `@services/tracking/tracker.py`:
- Around line 189-193: The block duplicates the dwell computation: remove the
redundant reassignment of dwell_frames and dwell_secs (the second occurrence
that uses prev and self.fps) so only the first computation remains; locate the
duplicate in tracker.py where dwell_frames and dwell_secs are assigned
(references: variable names dwell_frames, dwell_secs, prev and attribute
self.fps, within the tracking update logic/function) and delete the later
repeated lines to avoid dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c05c7ae-f7e7-4704-8a49-3cad143c93ff
📒 Files selected for processing (1)
services/tracking/tracker.py
|
fix as per suggestions |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/tracking/tracker.py (1)
260-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-zone
max_age_overrideis not applied in LOST→DEAD transition.Expiry still uses only
self._tracker.max_age(Line 266), so zone-specific retention from issue#65is bypassed. Computeeffective_max_agefromprev_obj.zones_presentby reading each zone’smax_age_override, then fall back to globalmax_agewhen none exists.Suggested minimal patch
@@ - if frames_since >= self._tracker.max_age: + zone_overrides = [] + for zone_name in prev_obj.zones_present: + zone = next((z for z in DEFAULT_ZONES if z.name == zone_name), None) + if zone is not None and zone.max_age_override is not None: + zone_overrides.append(zone.max_age_override) + + effective_max_age = ( + min(zone_overrides) if zone_overrides else self._tracker.max_age + ) + + if frames_since >= effective_max_age: self._emit_lifecycle( TrackState.DEAD, tid, prev_obj.zones_present, prev_obj.dwell_time_seconds, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/tracking/tracker.py` around lines 260 - 267, The LOST→DEAD expiry uses self._tracker.max_age directly; change it to compute an effective_max_age from prev_obj.zones_present by reading each zone's max_age_override (e.g., collect overrides for zones in prev_obj.zones_present, use the maximum override if any exist, otherwise fall back to self._tracker.max_age) and then replace the frames_since >= self._tracker.max_age check with frames_since >= effective_max_age before calling _emit_lifecycle/TrackState.DEAD; update logic around frames_since, _emit_lifecycle, prev_obj.zones_present, and max_age_override accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/tracking/tracker.py`:
- Around line 260-267: The LOST→DEAD expiry uses self._tracker.max_age directly;
change it to compute an effective_max_age from prev_obj.zones_present by reading
each zone's max_age_override (e.g., collect overrides for zones in
prev_obj.zones_present, use the maximum override if any exist, otherwise fall
back to self._tracker.max_age) and then replace the frames_since >=
self._tracker.max_age check with frames_since >= effective_max_age before
calling _emit_lifecycle/TrackState.DEAD; update logic around frames_since,
_emit_lifecycle, prev_obj.zones_present, and max_age_override accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b94ae30d-514b-48b2-8746-6ac4094aab47
📒 Files selected for processing (1)
services/tracking/tracker.py
|
@Devnil434 I did all the possible changes but could'nt figure out why it is failing in this test? Please mark it gssoc approved and difficulty in this PR and the other 3 PRs that is already merged #48 #53 #59 . Thank You! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/memory/memory.py`:
- Around line 39-42: The import list in services/memory/memory.py dropped
ActionHint but store_event() still references ActionHint.ZONE_ENTRY; restore
ActionHint in the from libs.schemas.memory import (...) line so ActionHint is
available to the module and lint/runtime errors are resolved (search for the
import and the store_event function to update the import tuple to include
ActionHint).
- Around line 350-353: A newly inserted get_active_track_ids definition was
placed in the middle of get_sequence, truncating get_sequence after raw_list,
duplicating get_active_track_ids later, and calling self._active_key(camera_id)
even though _active_key() takes no arguments; to fix, remove the misplaced
duplicate method so get_sequence retains its full body (including use of
raw_list), keep a single get_active_track_ids implementation positioned below
get_zone_entry_count, and update any _active_key(...) calls to call
_active_key() with no arguments; reference get_sequence, get_active_track_ids,
get_zone_entry_count, raw_list, and _active_key when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a24a8150-a277-4a6c-9e51-2af226e8ca11
📒 Files selected for processing (2)
services/memory/memory.pyservices/tracking/tracker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/tracking/tracker.py
| from libs.schemas.memory import ( | ||
| TrackEvent, | ||
| TrackSequence, | ||
| ) |
There was a problem hiding this comment.
Restore ActionHint in this import.
store_event() still compares event.action_hint to ActionHint.ZONE_ENTRY at Line 329, so dropping the import makes the module fail lint and raises on that path at runtime.
🐛 Minimal fix
from libs.schemas.memory import (
+ ActionHint,
TrackEvent,
TrackSequence,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from libs.schemas.memory import ( | |
| TrackEvent, | |
| TrackSequence, | |
| ) | |
| from libs.schemas.memory import ( | |
| ActionHint, | |
| TrackEvent, | |
| TrackSequence, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/memory/memory.py` around lines 39 - 42, The import list in
services/memory/memory.py dropped ActionHint but store_event() still references
ActionHint.ZONE_ENTRY; restore ActionHint in the from libs.schemas.memory import
(...) line so ActionHint is available to the module and lint/runtime errors are
resolved (search for the import and the store_event function to update the
import tuple to include ActionHint).
| def get_active_track_ids(self, camera_id: str) -> set[int]: | ||
| members = self._r.smembers(self._active_key(camera_id)) | ||
| result: set[int] = set() | ||
| for m in members: |
There was a problem hiding this comment.
This insertion breaks get_sequence() and duplicates get_active_track_ids().
Placing a new method here ends get_sequence() right after raw_list, pulls the rest of its body into the wrong scope, and then gets shadowed again by the existing get_active_track_ids() at Lines 379-382. The new call to self._active_key(camera_id) is also invalid because _active_key() takes no argument.
🐛 Suggested correction
- def get_active_track_ids(self, camera_id: str) -> set[int]:
- members = self._r.smembers(self._active_key(camera_id))
- result: set[int] = set()
- for m in members:
+ events: list[TrackEvent] = []
+ for raw in raw_list:Keep the single get_active_track_ids() implementation below get_zone_entry_count().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_active_track_ids(self, camera_id: str) -> set[int]: | |
| members = self._r.smembers(self._active_key(camera_id)) | |
| result: set[int] = set() | |
| for m in members: | |
| events: list[TrackEvent] = [] | |
| for raw in raw_list: |
🧰 Tools
🪛 GitHub Actions: Phase 3 — Memory Tests / 0_test.txt
[error] 352-352: ruff check (F841): Local variable result is assigned to but never used.
🪛 GitHub Actions: Phase 3 — Memory Tests / test
[error] 352-352: Ruff (F841): Local variable result is assigned to but never used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/memory/memory.py` around lines 350 - 353, A newly inserted
get_active_track_ids definition was placed in the middle of get_sequence,
truncating get_sequence after raw_list, duplicating get_active_track_ids later,
and calling self._active_key(camera_id) even though _active_key() takes no
arguments; to fix, remove the misplaced duplicate method so get_sequence retains
its full body (including use of raw_list), keep a single get_active_track_ids
implementation positioned below get_zone_entry_count, and update any
_active_key(...) calls to call _active_key() with no arguments; reference
get_sequence, get_active_track_ids, get_zone_entry_count, raw_list, and
_active_key when making the changes.
2 participants
Summary
Implemented configurable per-zone
max_ageoverrides for tracker persistence behavior.Changes
max_age_overridefield to zone definitionsmax_ageBehavior
closes [Issue] — Add configurable
max_ageper zone #65Summary by CodeRabbit