feat: implement REPEATED_APPROACH action classification with cooldown#79
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements repeated-zone-approach detection with a 10s per-track cooldown, moves the memory store to an in-memory implementation (cap 100 events per track), wires pipeline registries for per-track state, and adds tests covering repeated-approach scenarios. ChangesRepeated-Approach Detection Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (1)
tests/test_memory.py (1)
262-264: ⚡ Quick winAdd an exact-boundary cooldown assertion (10,000 ms elapsed).
The current cooldown test skips the equality edge. Add a check at exactly
last_trigger + 10000(e.g.,12000.0after2000.0) so cooldown semantics are explicit and regression-proof.🤖 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 `@tests/test_memory.py` around lines 262 - 264, Add an exact-boundary cooldown assertion: call classify_action with the same args used for the other checks (classify_action(obj, None, registry, counts, cooldown, 12000.0)) and assert it equals ActionHint.REPEATED_APPROACH so the behavior at last_trigger + 10000 ms is explicitly tested; place this assertion just before the existing 15000.0 check and reference classify_action and ActionHint in the update.
🤖 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/action_classifier.py`:
- Line 62: Update the cooldown condition in the check that uses last_time and
current_time_ms (the if with "last_time == 0.0 or (current_time_ms - last_time)
> 10000.0") to use an inclusive boundary so events at exactly 10,000 ms can
re-trigger; replace the strict greater-than with a greater-than-or-equal-to
comparison on (current_time_ms - last_time) to make the 10s cooldown inclusive.
In `@services/memory/memory.py`:
- Around line 251-252: The store currently keys events only by event.track_id
which causes cross-camera collisions; change the structure to partition by
camera_id first (e.g., self._store.setdefault(camera_id, {}) then per-camera
dict of track_id -> list) and update all places that read/write the store: in
the append path (where seq = self._store.setdefault(event.track_id, []);
seq.append(event)) use event.camera_id when creating/looking up the per-camera
bucket and then the event.track_id inside it, and modify read APIs like
get_active_track_ids(camera_id) and any other lookups (the code around the
blocks referencing self._store at the locations for lines ~259-266 and ~275-276)
to use the per-camera bucket so lookups become self._store.get(camera_id,
{})[track_id] (or iterate that inner dict) instead of global track_id keys;
ensure any function signatures that accept camera_id use it when accessing the
store.
- Around line 278-279: The expire_track method currently only removes entries
from self._store, leaving per-track state in self._zones (and related structures
like zones_visited) which causes stale state and memory leaks; update
expire_track to also remove any per-track entries from self._zones and any other
per-track collections (e.g., zones_visited) by popping/deleting the track_id
keys from those dicts/sets (refer to the expire_track function, self._store,
self._zones, and zones_visited symbols) so all per-track state is cleared when a
track expires.
- Around line 262-263: The code currently uses a truthy check for last_n so
last_n=0 is treated like falsy and returns the whole events list; update the
check to handle zero explicitly: replace the truthy branch with an explicit None
check and a zero-case, e.g. if last_n is None: leave events as-is; elif last_n >
0: events = events[-last_n:]; else: events = [] — apply this change where the
variable events is sliced based on the last_n parameter in
services/memory/memory.py.
---
Nitpick comments:
In `@tests/test_memory.py`:
- Around line 262-264: Add an exact-boundary cooldown assertion: call
classify_action with the same args used for the other checks
(classify_action(obj, None, registry, counts, cooldown, 12000.0)) and assert it
equals ActionHint.REPEATED_APPROACH so the behavior at last_trigger + 10000 ms
is explicitly tested; place this assertion just before the existing 15000.0
check and reference classify_action and ActionHint in the update.
🪄 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: d6f35941-3d2e-4e06-8183-c70e59e6b394
📒 Files selected for processing (5)
.gitignoreservices/memory/action_classifier.pyservices/memory/memory.pyservices/memory/pipeline.pytests/test_memory.py
| if counts[zone] >= 2: | ||
| if last_repeated_approach is not None: | ||
| last_time = last_repeated_approach.get(obj.track_id, 0.0) | ||
| if last_time == 0.0 or (current_time_ms - last_time) > 10000.0: # 10 second cooldown |
There was a problem hiding this comment.
Use an inclusive cooldown boundary (>=) for exact 10s re-trigger.
Line 62 currently uses > 10000.0, so an event at exactly 10,000 ms is still suppressed. That makes the effective cooldown slightly longer than configured.
Proposed fix
- if last_time == 0.0 or (current_time_ms - last_time) > 10000.0: # 10 second cooldown
+ if last_time == 0.0 or (current_time_ms - last_time) >= 10000.0: # 10 second cooldown📝 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.
| if last_time == 0.0 or (current_time_ms - last_time) > 10000.0: # 10 second cooldown | |
| if last_time == 0.0 or (current_time_ms - last_time) >= 10000.0: # 10 second cooldown |
🤖 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/action_classifier.py` at line 62, Update the cooldown
condition in the check that uses last_time and current_time_ms (the if with
"last_time == 0.0 or (current_time_ms - last_time) > 10000.0") to use an
inclusive boundary so events at exactly 10,000 ms can re-trigger; replace the
strict greater-than with a greater-than-or-equal-to comparison on
(current_time_ms - last_time) to make the 10s cooldown inclusive.
| if last_n: | ||
| events = events[-last_n:] |
There was a problem hiding this comment.
Handle last_n=0 explicitly in sequence slicing.
Line 262 uses a truthy check, so last_n=0 returns the full sequence instead of an empty one.
Small fix
- if last_n:
+ if last_n is not None:
events = events[-last_n:]📝 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.
| if last_n: | |
| events = events[-last_n:] | |
| if last_n is not None and last_n > 0: | |
| events = events[-last_n:] | |
| elif last_n == 0: | |
| events = [] |
| if last_n: | |
| events = events[-last_n:] | |
| if last_n is not None: | |
| events = events[-last_n:] if last_n > 0 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/memory/memory.py` around lines 262 - 263, The code currently uses a
truthy check for last_n so last_n=0 is treated like falsy and returns the whole
events list; update the check to handle zero explicitly: replace the truthy
branch with an explicit None check and a zero-case, e.g. if last_n is None:
leave events as-is; elif last_n > 0: events = events[-last_n:]; else: events =
[] — apply this change where the variable events is sliced based on the last_n
parameter in services/memory/memory.py.
tirth1356
force-pushed
the
feature/repeated-approach
branch
from
f8c31d3 to
a03f8d9
Compare
|
merge conflicts resolved and updates done |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 250-255: store_event is truncating the per-track ring buffer
(MAX_EVENTS_PER_TRACK) which causes get_zone_entry_count and similar methods to
undercount once older zone_entry events are dropped; fix by maintaining a
separate persistent counter(s) for cumulative event types (e.g., zone_entry) per
track and update them in store_event: when appending an event increment the
appropriate counter for event.track_id (and for other methods mentioned around
lines 267-273), and when trimming the seq compute how many removed events of
each counted type were dropped and add those to the persistent counters before
replacing self._store[event.track_id] with the truncated list; keep references
to MAX_EVENTS_PER_TRACK, self._store, store_event and get_zone_entry_count to
locate and apply the change.
🪄 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: 05c31ec9-3e6a-4d70-9130-bb28b7735e21
📒 Files selected for processing (5)
.gitignoreservices/memory/action_classifier.pyservices/memory/memory.pyservices/memory/pipeline.pytests/test_memory.py
🚧 Files skipped from review as they are similar to previous changes (3)
- services/memory/pipeline.py
- tests/test_memory.py
- services/memory/action_classifier.py
| def store_event(self, event): | ||
| seq = self._store.setdefault(event.track_id, []) | ||
| seq.append(event) | ||
| # Handle ring buffer | ||
| if len(seq) > MAX_EVENTS_PER_TRACK: | ||
| self._store[event.track_id] = seq[-MAX_EVENTS_PER_TRACK:] |
There was a problem hiding this comment.
get_zone_entry_count() starts undercounting after the 100-event cap.
The ring buffer drops older events at Line 254, but this method recomputes counts from the truncated buffer. Once a long-lived track rolls over, earlier zone_entry events disappear and the returned count shrinks, which can suppress repeated-entry logic or skew analytics.
Suggested direction
class MemoryStore:
def __init__(self, redis_client=None):
self._r = redis_client
self._store = {}
self._zones = {}
+ self._zone_entry_counts = {}
def store_event(self, event):
seq = self._store.setdefault(event.track_id, [])
seq.append(event)
# Handle ring buffer
if len(seq) > MAX_EVENTS_PER_TRACK:
self._store[event.track_id] = seq[-MAX_EVENTS_PER_TRACK:]
if event.zone:
self._zones.setdefault(event.track_id, set()).add(event.zone)
+ if event.zone and getattr(event.action_hint, "value", None) == "zone_entry":
+ counts = self._zone_entry_counts.setdefault(event.track_id, {})
+ counts[event.zone] = counts.get(event.zone, 0) + 1
def get_zone_entry_count(self, track_id, zone):
- events = self._store.get(track_id, [])
- count = 0
- for e in events:
- if e.zone == zone and e.action_hint.value == "zone_entry":
- count += 1
- return count
+ return self._zone_entry_counts.get(track_id, {}).get(zone, 0)Also applies to: 267-273
🤖 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 250 - 255, store_event is truncating
the per-track ring buffer (MAX_EVENTS_PER_TRACK) which causes
get_zone_entry_count and similar methods to undercount once older zone_entry
events are dropped; fix by maintaining a separate persistent counter(s) for
cumulative event types (e.g., zone_entry) per track and update them in
store_event: when appending an event increment the appropriate counter for
event.track_id (and for other methods mentioned around lines 267-273), and when
trimming the seq compute how many removed events of each counted type were
dropped and add those to the persistent counters before replacing
self._store[event.track_id] with the truncated list; keep references to
MAX_EVENTS_PER_TRACK, self._store, store_event and get_zone_entry_count to
locate and apply the change.
2 participants
#63 fixed
This PR implements the
ActionHint.REPEATED_APPROACHclassification as requested in the issue. It successfully identifies suspicious behavior when a tracked person repeatedly enters and leaves a restricted area.🚀 Key Changes
_zone_entry_countsand_last_repeated_approach) inpipeline.pyto seamlessly track zone entry counts across frames without injecting massive dependencies.classify_action()inaction_classifier.pyto correctly evaluate fresh zone entries compared to the previous frame's bounding box.MemoryStoredefinitions inservices/memory/memory.pyallowing tests to execute properly in the current environment structure.test_memory.py:test_repeated_approach_second_entrytest_repeated_approach_no_spamtest_repeated_approach_cooldown✅ Acceptance Criteria Met
📷 Screenshots / Logs
Summary by CodeRabbit
New Features
Tests