fix: indentation error in tracker.py causing SyntaxError - #200
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTracker update flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🤖 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 199-240: Move the LOST/DEAD lifecycle scan, expired ReID cleanup,
and TrackedFrame return out of the lost-embedding candidate loop so they execute
once per update() call, including when _lost_embeddings is empty. In the
disappeared-track loop, initialize track for each iteration before checking its
features, or restrict the feature access to the frames_since == 1 branch,
preventing undefined or stale values while preserving embedding capture only for
newly lost tracks.
- Around line 127-156: Move the ReID logic beginning with the `hasattr(t,
"features")` check and the `_lost_embeddings` loop into the enclosing `for t in
raw_tracks` loop. Keep the lost-track scan conditional on a valid
`new_embedding`, so tracks without features skip similarity matching instead of
referencing an unassigned variable; preserve the existing match, restoration,
deletion, and logging behavior.
- Around line 157-198: Dedent the per-track processing block beginning with ltwh
conversion and ending with tracked_objects.append(obj) so it runs for every
confirmed track inside the raw-tracks loop, not inside the ReID similarity
condition or after its unconditional break. Preserve the existing BORN, dwell,
trajectory, TrackedObject, current_ids, and tracked_objects behavior; optionally
replace trajectory list concatenation with the Ruff-recommended form.
🪄 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: 1e38f38b-3b9a-4626-8e03-32029f383e66
📒 Files selected for processing (1)
services/tracking/tracker.py
Comment on lines
+157
to
+198
| ltwh = t.to_ltwh() | ||
| x1 = float(ltwh[0]) | ||
| y1 = float(ltwh[1]) | ||
| x2 = x1 + float(ltwh[2]) | ||
| y2 = y1 + float(ltwh[3]) | ||
| cx, cy = (x1 + x2) / 2, (y1 + y2) / 2 | ||
|
|
||
| zones = [z.name for z in get_zones_for_point(cx, cy)] | ||
|
|
||
| # ── Lifecycle: BORN ─────────────────────────────────────────── | ||
| if tid not in self._known_ids: | ||
| self._known_ids.add(tid) | ||
| self._emit_lifecycle(TrackState.BORN, tid, zones, 0.0) | ||
| logger.info(f"Track BORN: #{tid} in zones={zones}") | ||
|
|
||
| # ── Dwell time ──────────────────────────────────────────────── | ||
| prev = self._active_tracks.get(tid) | ||
| dwell_frames = (prev.dwell_time_frames + 1) if prev else 1 | ||
| dwell_secs = dwell_frames / self.fps | ||
|
|
||
| # ── Trajectory ──────────────────────────────────────────────── | ||
| prev_traj = prev.trajectory if prev else [] | ||
| new_point = TrajectoryPoint(x=cx, y=cy, frame_id=self._frame_id) | ||
| trajectory = (prev_traj + [new_point])[-self.MAX_TRAJECTORY_LEN:] | ||
|
|
||
| obj = TrackedObject( | ||
| track_id = tid, | ||
| label = "person", | ||
| bbox = [x1, y1, x2, y2], | ||
| confidence = float(t.det_conf or 0.0), | ||
| center = (cx, cy), | ||
| dwell_time_frames = dwell_frames, | ||
| dwell_time_seconds = round(dwell_secs, 2), | ||
| state = TrackState.ACTIVE, | ||
| trajectory = trajectory, | ||
| zones_present = zones, | ||
| last_seen_frame = self._frame_id, | ||
| ) | ||
| if frames_since > self._tracker.max_age: | ||
| self._active_tracks[tid] = obj | ||
| current_ids.add(tid) | ||
| tracked_objects.append(obj) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Per-track processing only runs on a ReID match — normal tracks never produce a TrackedObject.
Lines 157-197 (bbox conversion, zone lookup, BORN emission, dwell time, trajectory, TrackedObject build/append) sit at the same indentation as break (line 155), i.e. nested inside if similarity > self.REID_SIMILARITY_THRESHOLD:. This means:
- This block never runs for the common case of a normal confirmed track with no ReID recovery —
tracked_objects/current_idsstay empty except when a ReID match happens. - It physically follows an unconditional
breakin the same suite, so it is unreachable dead code regardless.
This directly contradicts the AI summary/line-range description, which claims this block was fixed to run "after the ReID scan concludes, rather than being incorrectly nested." The code still shows the exact nesting bug described as fixed.
🐛 Proposed fix: dedent to run for every confirmed track
- break
+ break
- ltwh = t.to_ltwh()
- x1 = float(ltwh[0])
- y1 = float(ltwh[1])
- x2 = x1 + float(ltwh[2])
- y2 = y1 + float(ltwh[3])
- cx, cy = (x1 + x2) / 2, (y1 + y2) / 2
+ ltwh = t.to_ltwh()
+ x1 = float(ltwh[0])
+ y1 = float(ltwh[1])
+ x2 = x1 + float(ltwh[2])
+ y2 = y1 + float(ltwh[3])
+ cx, cy = (x1 + x2) / 2, (y1 + y2) / 2
...
- self._active_tracks[tid] = obj
- current_ids.add(tid)
- tracked_objects.append(obj)
+ self._active_tracks[tid] = obj
+ current_ids.add(tid)
+ tracked_objects.append(obj)(dedent lines 157-197 by one level, so they are siblings of the if hasattr(...) ReID block, still inside for t in raw_tracks:)
Optionally, per the Ruff hint, line 180 can use [*prev_traj, new_point] instead of list concatenation while you're touching this block.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 180-180: Consider [*prev_traj, new_point] instead of concatenation
Replace with [*prev_traj, new_point]
(RUF005)
🤖 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 157 - 198, Dedent the per-track
processing block beginning with ltwh conversion and ending with
tracked_objects.append(obj) so it runs for every confirmed track inside the
raw-tracks loop, not inside the ReID similarity condition or after its
unconditional break. Preserve the existing BORN, dwell, trajectory,
TrackedObject, current_ids, and tracked_objects behavior; optionally replace
trajectory list concatenation with the Ruff-recommended form.
Comment on lines
+199
to
+240
| # ── Lifecycle: LOST for tracks that disappeared ──────────────────── | ||
| for tid, prev_obj in list(self._active_tracks.items()): | ||
| if tid not in current_ids: | ||
| frames_since = self._frame_id - prev_obj.last_seen_frame | ||
| if frames_since == 1: | ||
| track = next((t for t in raw_tracks if int(t.track_id) == tid), None) | ||
|
|
||
| if track is not None and hasattr(track, "features") and track.features: | ||
| self._lost_embeddings[tid] = { | ||
| "embedding": track.features[-1], | ||
| "last_seen": self._frame_id, | ||
| } | ||
|
|
||
| self._emit_lifecycle( | ||
| TrackState.DEAD, tid, | ||
| TrackState.LOST, tid, | ||
| prev_obj.zones_present, | ||
| prev_obj.dwell_time_seconds, | ||
| ) | ||
| del self._active_tracks[tid] | ||
| logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s") | ||
| # ── Cleanup expired ReID embeddings ────────────────── | ||
| 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( | ||
| frame_id = self._frame_id, | ||
| camera_id = self.camera_id, | ||
| tracks = tracked_objects, | ||
| timestamp_ms = time.time() * 1000, | ||
| fps = self.fps, | ||
| ) | ||
| if frames_since > self._tracker.max_age: | ||
| self._emit_lifecycle( | ||
| TrackState.DEAD, tid, | ||
| prev_obj.zones_present, | ||
| prev_obj.dwell_time_seconds, | ||
| ) | ||
| del self._active_tracks[tid] | ||
| logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s") | ||
| # ── Cleanup expired ReID embeddings ────────────────── | ||
| 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( | ||
| frame_id = self._frame_id, | ||
| camera_id = self.camera_id, | ||
| tracks = tracked_objects, | ||
| timestamp_ms = time.time() * 1000, | ||
| fps = self.fps, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
LOST/DEAD handling, cleanup, and return are nested inside the ReID candidate loop — implicit None return / premature return.
Lines 199-240 sit at the same indentation as line 133 (age = ...), i.e. nested inside for lost_id, data in list(self._lost_embeddings.items()): (131). Consequences:
- If
self._lost_embeddingsis empty (typical case), this whole block never executes, soupdate()falls through without hitting anyreturn— implicitly returningNoneinstead of the declaredTrackedFrame, which will break any caller doingframe.tracks/frame.frame_id. - If non-empty,
update()returns after examining only the first lost_id candidate, short-circuiting the LOST/DEAD scan, cleanup, and frame construction so they run at most once per_lost_embeddingsentry instead of once perupdate()call.
Separately, at lines 203-206, track is assigned only inside if frames_since == 1: (203-204) but read by if track is not None and hasattr(track, "features")... at 206, which is a sibling of that if, not nested inside it — this will raise NameError the first time this path is hit, or silently reuse a stale track from a previous loop iteration afterward.
This contradicts the line-range description's claim that "LOST/DEAD for disappeared tracks" and the "return wiring" behavior are preserved as before.
🐛 Proposed fix: dedent to run once per `update()` call, and fix `track` scoping
- # ── Lifecycle: LOST for tracks that disappeared ────────────────────
- for tid, prev_obj in list(self._active_tracks.items()):
- if tid not in current_ids:
- frames_since = self._frame_id - prev_obj.last_seen_frame
- if frames_since == 1:
- track = next((t for t in raw_tracks if int(t.track_id) == tid), None)
-
- if track is not None and hasattr(track, "features") and track.features:
- self._lost_embeddings[tid] = {
- "embedding": track.features[-1],
- "last_seen": self._frame_id,
- }
-
- self._emit_lifecycle(
- TrackState.LOST, tid,
- prev_obj.zones_present,
- prev_obj.dwell_time_seconds,
- )
- if frames_since > self._tracker.max_age:
- self._emit_lifecycle(
- TrackState.DEAD, tid,
- prev_obj.zones_present,
- prev_obj.dwell_time_seconds,
- )
- del self._active_tracks[tid]
- logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s")
- # ── Cleanup expired ReID embeddings ──────────────────
- 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(
- frame_id = self._frame_id,
- camera_id = self.camera_id,
- tracks = tracked_objects,
- timestamp_ms = time.time() * 1000,
- fps = self.fps,
- )
+ # ── Lifecycle: LOST for tracks that disappeared ────────────────────
+ for tid, prev_obj in list(self._active_tracks.items()):
+ if tid not in current_ids:
+ frames_since = self._frame_id - prev_obj.last_seen_frame
+ if frames_since == 1:
+ track = next((t for t in raw_tracks if int(t.track_id) == tid), None)
+ if track is not None and hasattr(track, "features") and track.features:
+ self._lost_embeddings[tid] = {
+ "embedding": track.features[-1],
+ "last_seen": self._frame_id,
+ }
+
+ self._emit_lifecycle(
+ TrackState.LOST, tid,
+ prev_obj.zones_present,
+ prev_obj.dwell_time_seconds,
+ )
+ if frames_since > self._tracker.max_age:
+ self._emit_lifecycle(
+ TrackState.DEAD, tid,
+ prev_obj.zones_present,
+ prev_obj.dwell_time_seconds,
+ )
+ del self._active_tracks[tid]
+ logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s")
+ # ── Cleanup expired ReID embeddings ──────────────────
+ 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(
+ frame_id = self._frame_id,
+ camera_id = self.camera_id,
+ tracks = tracked_objects,
+ timestamp_ms = time.time() * 1000,
+ fps = self.fps,
+ )📝 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.
Suggested change
| # ── Lifecycle: LOST for tracks that disappeared ──────────────────── | |
| for tid, prev_obj in list(self._active_tracks.items()): | |
| if tid not in current_ids: | |
| frames_since = self._frame_id - prev_obj.last_seen_frame | |
| if frames_since == 1: | |
| track = next((t for t in raw_tracks if int(t.track_id) == tid), None) | |
| if track is not None and hasattr(track, "features") and track.features: | |
| self._lost_embeddings[tid] = { | |
| "embedding": track.features[-1], | |
| "last_seen": self._frame_id, | |
| } | |
| self._emit_lifecycle( | |
| TrackState.DEAD, tid, | |
| TrackState.LOST, tid, | |
| prev_obj.zones_present, | |
| prev_obj.dwell_time_seconds, | |
| ) | |
| del self._active_tracks[tid] | |
| logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s") | |
| # ── Cleanup expired ReID embeddings ────────────────── | |
| 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( | |
| frame_id = self._frame_id, | |
| camera_id = self.camera_id, | |
| tracks = tracked_objects, | |
| timestamp_ms = time.time() * 1000, | |
| fps = self.fps, | |
| ) | |
| if frames_since > self._tracker.max_age: | |
| self._emit_lifecycle( | |
| TrackState.DEAD, tid, | |
| prev_obj.zones_present, | |
| prev_obj.dwell_time_seconds, | |
| ) | |
| del self._active_tracks[tid] | |
| logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s") | |
| # ── Cleanup expired ReID embeddings ────────────────── | |
| 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( | |
| frame_id = self._frame_id, | |
| camera_id = self.camera_id, | |
| tracks = tracked_objects, | |
| timestamp_ms = time.time() * 1000, | |
| fps = self.fps, | |
| ) | |
| # ── Lifecycle: LOST for tracks that disappeared ──────────────────── | |
| for tid, prev_obj in list(self._active_tracks.items()): | |
| if tid not in current_ids: | |
| frames_since = self._frame_id - prev_obj.last_seen_frame | |
| if frames_since == 1: | |
| track = next((t for t in raw_tracks if int(t.track_id) == tid), None) | |
| if track is not None and hasattr(track, "features") and track.features: | |
| self._lost_embeddings[tid] = { | |
| "embedding": track.features[-1], | |
| "last_seen": self._frame_id, | |
| } | |
| self._emit_lifecycle( | |
| TrackState.LOST, tid, | |
| prev_obj.zones_present, | |
| prev_obj.dwell_time_seconds, | |
| ) | |
| if frames_since > self._tracker.max_age: | |
| self._emit_lifecycle( | |
| TrackState.DEAD, tid, | |
| prev_obj.zones_present, | |
| prev_obj.dwell_time_seconds, | |
| ) | |
| del self._active_tracks[tid] | |
| logger.info(f"Track DEAD: #{tid} after {prev_obj.dwell_time_seconds:.1f}s") | |
| # ── Cleanup expired ReID embeddings ────────────────── | |
| 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( | |
| 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 199 - 240, Move the LOST/DEAD
lifecycle scan, expired ReID cleanup, and TrackedFrame return out of the
lost-embedding candidate loop so they execute once per update() call, including
when _lost_embeddings is empty. In the disappeared-track loop, initialize track
for each iteration before checking its features, or restrict the feature access
to the frames_since == 1 branch, preventing undefined or stale values while
preserving embedding capture only for newly lost tracks.
Contributor
Author
|
🔔 Friendly ping @Devnil434 — this real fix has passing CI and references an approved GSSoC issue. Ready for review/merge. Thanks! |
1 similar comment
Contributor
Author
|
🔔 Friendly ping @Devnil434 — this real fix has passing CI and references an approved GSSoC issue. Ready for review/merge. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Real bug: services/tracking/tracker.py:127-240 has broken indentation causing 'return outside function' SyntaxError
Summary by CodeRabbit