Conversation
e319b42 to
9f4c5ae
Compare
Todo: reid-unique-count results with purely reid model after detection must match results before the change Add a functional test to verify that semantic metadata logic is working
| if name and name != 'detection' and ('reid' not in name and 'embedding' not in name): | ||
| categories[name] = {'label': tensor.get('label',''), | ||
| 'confidence': tensor.get('confidence', 100.0), | ||
| 'model_name': tensor.get('model_name', '')} |
There was a problem hiding this comment.
There is an edge case with this code such that we assume name that we get is unique, however running some experiments with the new models the names are not unique (they are all named classification_layer_name:logits).
When names collide, later tensors overwrite earlier ones in the dict, causing data loss.
We either have to:
- ensure uniqueness of the
namekey in our code (e.g. by adding a suffix in case of name clash) - ensure uniqueness of the
namekey from the DLSPS metadata output - use an array, not a dict
There was a problem hiding this comment.
array instead of dict will fail eventually. What we want is backward traceability of what created the metadata. Whether the uniqueness is ensured by DLSPS internally or we write it in our sscape_policies doesn't matter except for the long-term maintainability. I believe this is a universal problem rather than scenescape only. So my preference would be for handling inside DLSPS. @jakubsikorski could you file a JIRA ticket in the DLS project and add me and Dorau as watchers.
Thank you for being proactive and highlighting this.
4c06e34 to
06601e4
Compare
| networks: | ||
| scenescape-test: | ||
|
|
||
| secrets: |
There was a problem hiding this comment.
With secrets added, the vdms server gets stuck at initialization. The reid-unique-count test was passing prior to this change because the failure threshold was too generous. I will work with vdms team to resolve this issue.
There was a problem hiding this comment.
Pull request overview
This pull request implements a Semantic ReID Extension that adds 2-tier hybrid search capabilities to the Scene Controller's Re-ID system. The architecture combines TIER 1 metadata filtering (semantic attributes like age, gender) with TIER 2 vector similarity search for more accurate object re-identification.
Changes:
- Adds semantic metadata support with confidence-based constraint routing (>=0.8 for AND, <0.8 for OR)
- Implements VDMS adapter for storing/querying reid vectors with metadata
- Updates data structures to use dict format for reid (embedding_vector + model_name)
- Includes comprehensive unit tests (841 lines for VDMS adapter, 415 for UUID manager) and functional integration tests
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sscape_tests/vdms_adapter/test_vdms_adapter.py | Comprehensive unit tests for VDMS adapter with interface, initialization, entry addition, matching, and constraint building tests |
| tests/sscape_tests/uuid_manager/test_uuid_manager.py | Unit tests for UUID manager covering reid extraction, semantic metadata extraction, and tracker ID management |
| tests/functional/tc_reid_data_flow.py | End-to-end integration test validating 4 scenarios: no metadata, reid-only, semantic-only, and combined reid+semantic |
| tests/functional/tc_reid_semantic_unique_count.py | Functional test verifying unique detection counts with semantic classification enabled |
| controller/src/controller/vdms_adapter.py | Core VDMS database adapter implementing 2-tier hybrid search with metadata filtering and vector similarity |
| controller/src/controller/uuid_manager.py | UUID manager updates for semantic metadata extraction and feature gathering with timeout-based flushing |
| controller/src/controller/reid.py | ReID interface updates adding metadata parameter to addEntry and renaming findSimilarityScores to findMatches |
| controller/src/controller/moving_object.py | MovingObject changes to support reid dict format and metadata extraction from detection messages |
| controller/src/controller/scene.py | Scene controller updates to deserialize reid from metadata structure |
| controller/src/controller/detections_builder.py | Detection builder updates to output reid in metadata structure |
| controller/src/schema/metadata.schema.json | JSON schema updates adding semantic_metadata and semantic_metadata_attribute definitions |
| dlstreamer-pipeline-server/user_scripts/gvapython/sscape/sscape_policies.py | DLStreamer policy updates to extract reid and classification metadata into new format |
| controller/docs/user-guide/Extended-ReID.md | New documentation explaining 2-tier hybrid search implementation and confidence-based routing |
| tests/Makefile.sscape | Makefile updates adding uuid-manager-unit and vdms-adapter-unit test targets |
| tests/Makefile.functional | Functional test targets for reid-data-flow and reid-semantic-unique-count |
| tests/compose/vdms.yml | VDMS compose configuration with TLS removed (non-TLS mode) |
| tests/compose/scene_reid.yml | Scene compose configuration with VDMS client cert secrets removed |
| dlstreamer-pipeline-server/queuing-config-reid-semantic.json | DLStreamer pipeline configuration for reid with semantic classification |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tdorauintc
left a comment
There was a problem hiding this comment.
Not fully reviewed yet
| return 0 | ||
|
|
||
|
|
||
| def test_reid_data_flow_end_to_end(params, record_xml_attribute): |
There was a problem hiding this comment.
[may be done in another PR] I suggest to refactor the function and split into separate functions or a test class with a state and separate methods for test setup, clean-up and each scenario for better maintainability.
There was a problem hiding this comment.
Shouldn't we make the change (apply ReID on ROI from detection stage) in original dlstreamer-pipeline-server/{queuing|retail}-config-reid.json files and keep a single file for legacy reid and semantic-reid?
There was a problem hiding this comment.
The earlier comment is not valid any more, I realized that this file contains additional age-gender classification model. I would advice to add a similar config file for retail scene and document it in user guide.
| obj.rotation = obj_data.get('rotation') | ||
| obj.reidVector = obj_data.get('reid') | ||
| # Extract reid from metadata if present | ||
| metadata = obj_data.get('metadata', {}) |
There was a problem hiding this comment.
I personally prefer a separate reid_config.json rather than introducing tagging in the VA pipeline. Reason: the 2nd solution (reid config) is "I tell the component doing ReID how it should interpret its input", the 1st one is: "I tell the component doing VA how it should tell the ReID component how it should interpret my metadata", which also makes sense but has downside of introducing another coupling between VA and ReID implementation.
| @param record_xml_attribute Pytest fixture recording the test name. | ||
| @return exit_code Indicates test success or failure. | ||
| """ | ||
| TEST_NAME = "NEX-T10539" |
There was a problem hiding this comment.
tc_reid_unique_count.py (NEX-T10539) - implementation does not check the objective of the Zephyr test.
This test only checks counter integrity. It does not measure unique‑detection counts in two configurations (ReID disabled vs ReID enabled) and does not verify that counts are reduced by at least 50% when Re‑ID is enabled, as required by the Zephyr test steps.
There was a problem hiding this comment.
The fixtures mock_vdms and mock_log are not used by any test and should be removed. test_uuid_manager.py uses direct @patch calls.
There was a problem hiding this comment.
Again, neither of the fixtures is used in the test.
| name: {{ .Release.Name }}-tracker-config | ||
| - name: reid-config | ||
| configMap: | ||
| name: {{ .Release.Name }}-tracker-config |
There was a problem hiding this comment.
| name: {{ .Release.Name }}-tracker-config | |
| name: {{ .Release.Name }}-reid-config |
| log.info("Waiting for scene controller and VDMS to initialize...") | ||
| time.sleep(40) | ||
| log.info("Scene controller initialization wait complete") |
There was a problem hiding this comment.
[To be fixed in future PR]
This might be a root test flakiness, it's better to poll for components readiness (assuming we have it implemented).
| self.stale_feature_timer = threading.Timer(1.0, check_stale_features) | ||
| self.stale_feature_timer.daemon = True | ||
| self.stale_feature_timer.start() |
There was a problem hiding this comment.
You can avoid copy paste here by calling "parent" function.
| self.stale_feature_timer = threading.Timer(1.0, check_stale_features) | |
| self.stale_feature_timer.daemon = True | |
| self.stale_feature_timer.start() | |
| self._start_stale_feature_timer() |
| self.stale_feature_timer.daemon = True | ||
| self.stale_feature_timer.start() | ||
|
|
||
| self.stale_feature_timer = threading.Timer(1.0, check_stale_features) |
There was a problem hiding this comment.
Could we create const for this magic number?
Should it be configurable? If so, I suggest to postpone it to follow-up PR
|
|
||
| self.stale_feature_timer = threading.Timer(1.0, check_stale_features) | ||
| self.stale_feature_timer.daemon = True | ||
| self.stale_feature_timer.start() |
There was a problem hiding this comment.
We should gracefully stop the timer, the stop() method should be called wherever the UUIDManager is destroyed.
tdorauintc
left a comment
There was a problem hiding this comment.
Comments starting with [future] are not blocking the PR and may be addressed in another PR.
| - source: vdms-client-key | ||
| target: certs/scenescape-vdms-c.key | ||
| - source: vdms-client-cert | ||
| target: certs/scenescape-vdms-c.crt |
There was a problem hiding this comment.
To be removed. I get error: service "scene" refers to undefined secret vdms-client-key: invalid compose project when running out-of-box demo
| log.debug(f"[VDMS] findMatches returned {len(result)} result(s) from {len(reid_vectors)} vector(s)") | ||
| for idx, entities in enumerate(result): | ||
| if entities: | ||
| log.debug(f"[VDMS] Vector {idx}: found {len(entities)} matches") | ||
| # Log distance scores for debugging | ||
| for match_idx, entity in enumerate(entities[:3]): # Show first 3 matches | ||
| distance = entity.get('_distance', 'unknown') | ||
| uuid = entity.get('uuid', 'unknown') | ||
| rvid = entity.get('rvid', 'unknown') | ||
| log.debug(f"[VDMS] Match {match_idx}: uuid={uuid}, rvid={rvid}, distance={distance}") | ||
| return result | ||
| log.debug("[VDMS] findMatches returned None (no response from VDMS)") |
There was a problem hiding this comment.
[future] This loop is only for debug purposes. For performance reasons it would make sense to execute it conditionally, e.g.:
if log.logger.isEnabledFor(logging.DEBUG):
...
| # Case for incrementing the counter when there is no re-id vector | ||
| if sscape_object.reidVector is None and result is None: | ||
| if sscape_object.reid is None and result is None: | ||
| self.unique_id_count += 1 |
There was a problem hiding this comment.
I don't understand the logic that increments this counter in two cases: here and in pruneInactiveTracks function. Please make sure it is expected behavior.
[future] Incrementing a counter in a function named isNewTrackerID is not intuitive, and hence it may cause double counting if someone in future decides to run it another place in code. I would expect rather that a caller of this function increments the counter.
There was a problem hiding this comment.
The earlier comment is not valid any more, I realized that this file contains additional age-gender classification model. I would advice to add a similar config file for retail scene and document it in user guide.
|
One more comment: I did some tests and found that metadata attributes (e.g. age, gender) are not published in tracks, contrary to ReID embeddings which are outputted in |
📝 Description
This pull request implements a Semantic ReID Extension that adds 2-tier hybrid search capabilities to the Scene Controller's Re-ID system. The architecture combines TIER 1 metadata filtering (semantic attributes like age, gender) with TIER 2 vector similarity search for more accurate object re-identification.
✨ 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: