ITEP-90209: tracked object contains re-id state and object id chain#1302
ITEP-90209: tracked object contains re-id state and object id chain#1302saratpoluri wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Scene Controller’s tracked-object output to carry explicit ReID status and an object-ID history chain (for post-mortem stitching/analysis), and updates multiple service “Agents” guides plus repository Copilot instructions to reflect current build/test workflows.
Changes:
- Add
ReidState+previous_ids_chaintracking onMovingObject, and serialize these fields in detection outputs. - Update UUID assignment / similarity-query flow to set ReID state and record ID-chain transitions.
- Refresh build/test instructions across service
Agents.mddocs and.github/copilot-instructions.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
controller/src/controller/moving_object.py |
Introduces ReidState, ID-chain recording helpers, and new per-object fields. |
controller/src/controller/uuid_manager.py |
Updates similarity-query / ID assignment to set ReID state and record ID changes. |
controller/src/controller/detections_builder.py |
Serializes reid_state and previous_ids_chain into object dictionaries. |
controller/Agents.md |
Updates controller build/test instructions (currently has Markdown fence issues). |
mapping/Agents.md |
Updates mapping build/test instructions (wording vs actual Make targets mismatch). |
manager/Agents.md |
Updates manager build/test instructions (wording vs actual Make targets mismatch). |
autocalibration/Agents.md |
Updates autocalibration build/test instructions (wording vs actual Make targets mismatch). |
cluster_analytics/Agents.md |
Updates cluster_analytics build/test instructions (wording vs actual Make targets mismatch). |
.github/copilot-instructions.md |
Updates repo-wide guidance for build/test prerequisites and test targets. |
| # MATCH FOUND - YES + DB ID ALREADY IN DICT - NO | ||
| if database_id and self.isNewID(database_id): | ||
| self.active_ids[sscape_object.rv_id] = [database_id, similarity] | ||
| # Query succeeded and found a match -> update state to MATCHED |
There was a problem hiding this comment.
updateActiveDict() uses if database_id and self.isNewID(database_id) to detect whether a matched DB ID is already assigned to another active track. When isNewID(database_id) is false (ID collision), the current logic falls into the else path and ends up reusing that database_id anyway, which can produce duplicate gid values across simultaneous tracks. Handle the collision separately (e.g., treat as no-match and generate/keep a unique gid) instead of reusing the already-active ID.
| # If reid is disabled, mark object state immediately (no query will be made) | ||
| if not self.reid_enabled: | ||
| sscape_object.reid_state = ReidState.REID_DISABLED | ||
|
|
||
| # Continue gathering features until we have enough or query is already submitted | ||
| if sscape_object.rv_id not in self.active_query and self.reid_enabled: | ||
| self.gatherQualityVisualFeatures(sscape_object) |
There was a problem hiding this comment.
assignID() sets sscape_object.reid_state = REID_DISABLED whenever self.reid_enabled is false. Since assignID() runs every frame, this will overwrite per-object terminal states like MATCHED/QUERY_NO_MATCH after they were set, and it can also mask an in-flight/completed query. Only set REID_DISABLED for objects that have not already been queried/matched (e.g., when reid_state is still PENDING_COLLECTION and no query has been submitted for that track).
| self.reid = {} # Initialize reid as empty dict | ||
| self.metadata = {} # Initialize metadata as empty dict | ||
| self.reid_state = ReidState.PENDING_COLLECTION # Track reID state | ||
| self.similarity = None # Similarity score from last reID match | ||
| self.previous_ids_chain = [] # Track object ID history: [{'id': gid, 'timestamp': ts, 'similarity_score': score}, ...] |
There was a problem hiding this comment.
reid_state, similarity, and previous_ids_chain are initialized on each new MovingObject instance. In the tracking flow, objects are re-created each frame and setPrevious() copies persistent fields (gid, first_seen, chain_data, etc.) from the prior instance, but it does not copy these newly added fields. As a result, downstream outputs will often see reid_state reset to PENDING_COLLECTION and lose previous_ids_chain across frames. Persist these fields across frames (e.g., copy them in setPrevious() or store them inside chain_data).
| make -C controller # Rebuild controller | ||
| make -C controller test-build # Build controller + test image | ||
|
|
||
| # OR from controller/ directory | ||
| cd controller && make # Rebuild controller |
There was a problem hiding this comment.
The build commands shown here use make -C <service> / cd <service> && make, but those targets default to build-image (cached build) via common.mk, not a no-cache rebuild. Either change the wording from "Rebuild" to "Build", or update the commands to explicitly run make ... rebuild if you intend a rebuild.
| make -C controller # Rebuild controller | |
| make -C controller test-build # Build controller + test image | |
| # OR from controller/ directory | |
| cd controller && make # Rebuild controller | |
| make -C controller # Build controller | |
| make -C controller test-build # Build controller + test image | |
| # OR from controller/ directory | |
| cd controller && make # Build controller |
| # Specific test module | ||
|
|
||
| pytest tests/sscape_tests/controller/test_tracking.py -v | ||
| ``` | ||
|
|
||
| ```` |
There was a problem hiding this comment.
Markdown code fences are broken here: the controller build/test code block is closed at line 162, but a new fence is started with four backticks (````) at line 168, and the "Running Locally" block is also closed with four backticks. This renders incorrectly in GitHub. Use triple backticks consistently (```), and wrap the pytest ... command in an appropriate fenced block if desired.
| ```bash | ||
| # From root directory | ||
| make manager # Build image | ||
| make rebuild-manager # Clean + rebuild | ||
| # From repo root | ||
| make -C manager # Rebuild manager | ||
|
|
||
| # Build with dependencies | ||
| make build-core # Includes manager | ||
| # OR from manager/ directory | ||
| cd manager && make # Rebuild manager | ||
|
|
There was a problem hiding this comment.
make -C manager / cd manager && make runs the default build-image target (cached build), not a rebuild. Consider changing the wording from "Rebuild" to "Build", or update the command to make ... rebuild if a rebuild is intended.
| make -C autocalibration # Rebuild autocalibration | ||
| make -C autocalibration test-build # Build autocalibration + test image | ||
|
|
||
| # OR from autocalibration/ directory | ||
| cd autocalibration && make # Rebuild autocalibration |
There was a problem hiding this comment.
make -C autocalibration / cd autocalibration && make invokes the default build-image target; it does not do a no-cache rebuild. Please align the wording ("Build" vs "Rebuild") or use make ... rebuild if you intend rebuild semantics.
| make -C autocalibration # Rebuild autocalibration | |
| make -C autocalibration test-build # Build autocalibration + test image | |
| # OR from autocalibration/ directory | |
| cd autocalibration && make # Rebuild autocalibration | |
| make -C autocalibration # Build autocalibration | |
| make -C autocalibration test-build # Build autocalibration + test image | |
| # OR from autocalibration/ directory | |
| cd autocalibration && make # Build autocalibration |
| ```bash | ||
| # From root directory | ||
| make cluster_analytics # Build image | ||
| make rebuild-cluster_analytics # Clean + rebuild | ||
| # From repo root | ||
| make -C cluster_analytics # Rebuild cluster_analytics | ||
| make -C cluster_analytics test-build # Build cluster_analytics + test image | ||
|
|
||
| # OR from cluster_analytics/ directory | ||
| cd cluster_analytics && make # Rebuild cluster_analytics | ||
| cd cluster_analytics && make test-build # Build cluster_analytics + test image |
There was a problem hiding this comment.
make -C cluster_analytics / cd cluster_analytics && make runs the default cached build target, not a rebuild. Adjust the wording from "Rebuild" to "Build" or use make ... rebuild if a rebuild is intended.
| if database_id and self.isNewID(database_id): | ||
| self.active_ids[sscape_object.rv_id] = [database_id, similarity] | ||
| # Query succeeded and found a match -> update state to MATCHED | ||
| sscape_object.reid_state = ReidState.MATCHED | ||
| sscape_object.gid = database_id | ||
| sscape_object.similarity = similarity | ||
| # Record the matched ID in chain with similarity score | ||
| sscape_object.record_id_change(database_id, similarity_score=similarity, timestamp=query_timestamp) | ||
|
|
||
| log.debug( | ||
| f"updateActiveDict: Match found for {sscape_object.rv_id}: {database_id},{similarity}") | ||
| # MATCH FOUND - NO / DB ID ALREADY IN DICT - YES | ||
| f"updateActiveDict: Match found for {sscape_object.rv_id}: {database_id}, similarity={similarity}, state={ReidState.MATCHED.value}") | ||
| self.active_ids[sscape_object.rv_id] = [database_id, similarity] |
There was a problem hiding this comment.
New behavior in updateActiveDict() mutates per-object state (reid_state, similarity) and appends to previous_ids_chain via record_id_change(), but the existing tests/sscape_tests/uuid_manager/test_uuid_manager.py suite does not currently cover updateActiveDict() or these new state transitions. Adding unit tests for (a) match found, (b) no match/new gid assignment, and (c) ID-collision handling would help prevent regressions.
| # Add reid state for downstream business logic to distinguish "never queried" from "query made" | ||
| if hasattr(aobj, 'reid_state'): | ||
| obj_dict['reid_state'] = aobj.reid_state.value # Convert enum to string | ||
|
|
||
| # Add previous IDs chain for post-mortem object stitching analysis | ||
| if hasattr(aobj, 'previous_ids_chain') and aobj.previous_ids_chain: | ||
| obj_dict['previous_ids_chain'] = aobj.previous_ids_chain | ||
|
|
There was a problem hiding this comment.
prepareObjDict() now conditionally emits reid_state and previous_ids_chain, but tests/sscape_tests/scenescape/test_detections_builder.py does not assert these fields are serialized correctly. Adding a couple of unit tests (e.g., enum -> string conversion and chain presence/absence) would lock in the API contract for downstream consumers.
saratpoluri
requested review from
a team,
Irakus,
MikolajKasprzak,
daddo-intel,
dmytroye,
ltalarcz,
sarthakdeva-intel,
sbelhaik and
tdorauintc
as code owners
📝 Description
Provide a clear summary of the changes and the context behind them. Describe what was changed, why it was needed, and how the changes address the issue or add value.
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: