Add TemporalSpan model#1601
Open
horatiualmasan wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds temporal span storage and annotation fields, wires temporal spans through create/update/delete/query paths, updates dataset copy/delete behavior, and adds resolver tests. ChangesTemporal span feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lightly_studio/tests/resolvers/test_create_many_temporal_span.py (1)
124-161: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMissing boundary test for
start_time_s == end_time_s.The parametrize list covers less-than violation (
3.0, 2.0) but not the equality boundary case, which is often where off-by-one validation bugs hide.🤖 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 `@lightly_studio/tests/resolvers/test_create_many_temporal_span.py` around lines 124 - 161, The invalid temporal span test in test_create_many__invalid_temporal_span is missing the equality boundary case for start_time_s and end_time_s. Update the parametrize list to include a case where both values are equal and assert the same ValueError path, so annotation_resolver.create_many is covered for the non-strict boundary as well as the existing less-than and missing-value cases.lightly_studio/src/lightly_studio/models/temporal_span.py (1)
19-34: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider a DB-level check constraint for temporal ordering.
Nothing here enforces
end_time_s >= start_time_sor non-negative values at the schema level; if upstream validation (_validate_optional_temporal_span, referenced in the create_many resolver layer) is ever bypassed, invalid rows could still be persisted.🤖 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 `@lightly_studio/src/lightly_studio/models/temporal_span.py` around lines 19 - 34, Add a database-level constraint to TemporalSpanTable so invalid spans cannot be persisted even if upstream validation is bypassed. Update the TemporalSpanTable model to enforce non-negative start_time_s/end_time_s and require end_time_s to be greater than or equal to start_time_s, using the model’s table/schema constraints rather than relying only on _validate_optional_temporal_span in the resolver layer.lightly_studio/src/lightly_studio/resolvers/annotation_resolver/create_many.py (1)
122-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment now understates scope.
The comment
# Bulk save object detection annotationsat Line 134 now precedes threebulk_save_objectscalls (object detection, segmentation, temporal spans), not just object detection.✏️ Suggested tweak
- # Bulk save object detection annotations + # Bulk save type-specific annotation details session.bulk_save_objects(object_detection_annotations) session.bulk_save_objects(segmentation_annotations) session.bulk_save_objects(temporal_spans)🤖 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 `@lightly_studio/src/lightly_studio/resolvers/annotation_resolver/create_many.py` around lines 122 - 137, The bulk-save comment is stale and only describes object detection, but the following calls in create_many.py now save object detection, segmentation, and temporal spans. Update the comment near the bulk_save_objects calls in create_many() to accurately describe all three annotation collections being persisted, so the label matches the full scope of the code.
🤖 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.
Nitpick comments:
In `@lightly_studio/src/lightly_studio/models/temporal_span.py`:
- Around line 19-34: Add a database-level constraint to TemporalSpanTable so
invalid spans cannot be persisted even if upstream validation is bypassed.
Update the TemporalSpanTable model to enforce non-negative
start_time_s/end_time_s and require end_time_s to be greater than or equal to
start_time_s, using the model’s table/schema constraints rather than relying
only on _validate_optional_temporal_span in the resolver layer.
In
`@lightly_studio/src/lightly_studio/resolvers/annotation_resolver/create_many.py`:
- Around line 122-137: The bulk-save comment is stale and only describes object
detection, but the following calls in create_many.py now save object detection,
segmentation, and temporal spans. Update the comment near the bulk_save_objects
calls in create_many() to accurately describe all three annotation collections
being persisted, so the label matches the full scope of the code.
In `@lightly_studio/tests/resolvers/test_create_many_temporal_span.py`:
- Around line 124-161: The invalid temporal span test in
test_create_many__invalid_temporal_span is missing the equality boundary case
for start_time_s and end_time_s. Update the parametrize list to include a case
where both values are equal and assert the same ValueError path, so
annotation_resolver.create_many is covered for the non-strict boundary as well
as the existing less-than and missing-value cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c13eb43-d24b-405d-a4a6-ecf9b9a16045
📒 Files selected for processing (13)
lightly_studio/src/lightly_studio/api/db_tables.pylightly_studio/src/lightly_studio/migrations/versions/1783435200_c3d4e5f6a7b8_add_temporal_span.pylightly_studio/src/lightly_studio/models/annotation/annotation_base.pylightly_studio/src/lightly_studio/models/temporal_span.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/annotation_helper.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/create_many.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/delete_annotation.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/delete_annotations.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/get_all_by_collection_id_and_parent_sample_ids.pylightly_studio/src/lightly_studio/resolvers/dataset_resolver/deep_copy.pylightly_studio/src/lightly_studio/resolvers/dataset_resolver/delete_dataset.pylightly_studio/src/lightly_studio/resolvers/dataset_resolver/table_coverage_utils.pylightly_studio/tests/resolvers/test_create_many_temporal_span.py
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
1 participant
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.
What has changed and why?
Adds a generic temporal_span table to store optional start/end times (seconds) on any sample-backed entity, and wires it into annotations as a first step toward video event annotations (e.g. ActivityNet).
How has it been tested?
new tests
Did you update CHANGELOG.md?
Summary by CodeRabbit
start_time_s/end_time_sinputs.