Conversation
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
I found 2 issue(s).
- [NIT]
ODC_API_BASEnow defined in three places (src/beeutil/_constants.py:2) - [NIT] Exception chaining lost in
exceptblocks (src/beeutil/embeddings.py:65, src/beeutil/recordings.py:53)
src/beeutil/embeddings.py
Outdated
| raise EmbeddingsError(f'Expected list from odc-api, got {type(items).__name__}') | ||
|
|
||
| valid = [] | ||
| for item in items: |
There was a problem hiding this comment.
If I understand this correctly, it's checking for sensor data. WDYT about if we move this to a method in the records module (and can parameterize it as an argument to do the checks in case there's a use-case that's not sensor data in the future)? That way we can leave embeddings.py be only concerned about embeddings operations
There was a problem hiding this comment.
Removed this logic. I think 1) it was checking for an edge case that's unlikely and 2) it shouldn't be the responsibility of this module/function.
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
I found 3 issue(s).
- [P2]
load_query_embeddingsreturns unvalidated data (src/beeutil/embeddings.py:253) - [P3]
cosine_similaritysilently produces wrong results for non-normalized vectors (src/beeutil/embeddings.py:155) - [INFO] Duplicate
ODC_API_BASEdefinition insecrets.py(src/beeutil/secrets.py:33)
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
jimmdd
left a comment
jimmdd
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
I found 1 issue(s).
- [P3] Param forwarding and timeout no longer tested (tests/test_embeddings.py:161)
| } | ||
| ] | ||
|
|
||
| with patch('beeutil.embeddings.requests.get', return_value=mock_resp) as mock_get: |
There was a problem hiding this comment.
[P3] Param forwarding and timeout no longer tested
The old test_list_embeddings_returns_valid_items verified that since/until are correctly forwarded as query params and that timeout=10 is set. The new version only asserts on the returned timestamps. Combined with the removal of test_list_embeddings_no_params, test_list_embeddings_since_only, test_list_embeddings_until_only, and test_poll_and_match_forwards_since_param, there is now no test verifying the HTTP call contract (correct params, timeout). A regression silently dropping the timeout would cause production hangs. - [NIT] Extra blank lines left behind — tests/test_embeddings.py:52-53, 140-141, 213-214, 328-330, 366 Several spots have double/triple blank lines where tests were deleted. Minor cleanup.
jimmdd
left a comment
jimmdd
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
jimmdd
left a comment
jimmdd
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
# Conflicts: # requirements.txt
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
# Conflicts: # src/beeutil/__init__.py
tylerlu-hivemapper
force-pushed
the
cap-94-embeddings-module
branch
from
214e606 to
2754d52
Compare
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
hiveclawgit
left a comment
hiveclawgit
left a comment
There was a problem hiding this comment.
Re-review (changes since last review):
✅ Looks good. I did not identify a discrete regression in this diff.
5 participants
Summary
Two new
beeutilmodules for plugin developers to query scene embeddings and find matches.beeutil.embeddingsfetch_and_match(since, query_embeddings)— fetch new embeddings, compare, return matcheslist_embeddings(),cosine_similarity(),find_matches()— low-level primitivesload_query_embeddings()— stubbed, blocked on CAP-103 (endpoint TBD)beeutil.recordingsget_videos_by_timerange(start_ms, end_ms)— find video files by time rangeAlso:
_constants.pyfor shared ODC base URL, 26 tests.Key decisions
{label, embedding, threshold}— consistent across backend, odc-api, SDKnp.dot)list_embeddings()is a thin wrapper — returns whatever odc-api gives us, no SDK-side field filteringfetch_and_match()even with no matchesload_query_embeddings()stubbed — endpoint TBD pending team alignment on where query embeddings live on device (dashcam user config vs plugin KV store vs separate collection)