Conversation
FSAFTik
requested review from
kkeroo,
klemen1999 and
tersekmatija
as code owners
FSAFTik
requested review from
PetrNovota and
Copilot
and removed request for
kkeroo,
klemen1999 and
tersekmatija
There was a problem hiding this comment.
Pull request overview
This PR refactors the SnapsUploader node and SnapData message to better align with the DepthAI Hub Events API. The key change is migrating from individual frame/detections parameters to using dai.FileGroup for more flexible data handling.
Changes:
- Refactored
SnapDatato usedai.FileGroupinstead of separateframe,file_name, anddetectionsparameters - Updated
SnapsUploaderto use camelCase method names (setToken,setCacheDir,setCacheIfCannotSend,setLogResponse) and removedset_urlmethod - Updated all tests to work with the new
SnapDatastructure and added test coverage for newSnapsUploadermethods
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| depthai_nodes/message/snap_data.py | Simplified constructor to accept file_group parameter instead of individual frame/detections, updated documentation |
| depthai_nodes/node/snaps_uploader.py | Renamed methods to camelCase, removed set_url, added new configuration methods (setCacheDir, setCacheIfCannotSend, setLogResponse), simplified process logic |
| tests/unittests/test_messages/test_snap_data.py | Updated all test cases to use dai.FileGroup, removed mutability tests for removed fields, added new tests for multiple files in group |
| tests/unittests/test_nodes/test_host_nodes/test_snaps_uploader_node.py | Restructured tests, added coverage for new methods, removed tests for deleted set_url functionality, improved test assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def set_token(self, token: str): | ||
| os.environ.setdefault("DEPTHAI_HUB_API_KEY", token) | ||
| def setToken(self, token: str): | ||
| os.environ["DEPTHAI_HUB_API_KEY"] = token |
There was a problem hiding this comment.
The setToken method unconditionally overwrites the environment variable. If the token is already set, this will silently replace it. Consider either using setdefault (as before) or adding a parameter to control the overwrite behavior, or logging when an existing token is being replaced.
| def setCacheDir(self, cacheDir: str): | ||
| """Set the cache directory for storing cached data. | ||
|
|
||
| By default, the cache directory is set to /internal/private |
There was a problem hiding this comment.
Documentation formatting inconsistency: this comment doesn't end with a period, while the comments in setCacheIfCannotSend (line 35) and setLogResponse (line 44) do. Maintain consistent punctuation in docstrings.
depthai_nodes/node/snaps_uploader.py
Outdated
| By default, logResponse is set to false. Logs are visible in depthAI logs with | ||
| INFO level. |
There was a problem hiding this comment.
Corrected capitalization of 'DepthAI' to match the standard product name formatting (should be 'DepthAI' not 'depthAI').
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| uploader._em.setCacheDir.assert_called_once_with("/custom/cache/path") | ||
| mock_snaps_uploader_logger.info.assert_called_with( | ||
| "Set cache directory to: /custom/cache/path" | ||
| ) |
There was a problem hiding this comment.
The test verifies that setCacheDir calls the underlying _em.setCacheDir and logs appropriately, but doesn't test the behavior with invalid paths (e.g., None, empty string). Consider adding edge case tests for input validation.
Purpose
Update of SnapsUploader Node and SnapData Message
Specification
Update to the Snaps Uploader node to expose all the snaps related methods of the dai.EventsManager + removal of unrelated methods. Changed structure of SnapData to use dai.FileGroup
Dependencies & Potential Impact
After merge and release, apps and examples who use SnapData will need to migrate to new SnapData structure. Also set_url method of SnapsUploader is removed, check for usage
Deployment Plan
Change oak-examples examples who use SnapData or SnapsUploader to be compatible with new versions
Testing & Validation
Tested manually with data-collection application + with unit tests