Skip to content

fix: ED-2306 Make dynamic attributes unaware of spaces #1084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
clinton-encord wants to merge 11 commits into from
Closed

fix: ED-2306 Make dynamic attributes unaware of spaces #1084

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5c27b2b
Dynamic attributes for videos now operate on a per frame basis
clinton-encord Mar 5, 2026
25ce831
Fix tests
clinton-encord Mar 5, 2026
9f1a62d
Remove unused dynamic answer managers
clinton-encord Mar 5, 2026
d64a464
Do not remove dynamic attributes when object is removed from frame
clinton-encord Mar 5, 2026
4989d0b
Fix test
clinton-encord Mar 5, 2026
3bb7c36
Formatting
clinton-encord Mar 5, 2026
ea5bdb0
Fix more tests
clinton-encord Mar 5, 2026
b5fc876
Some small refactors
clinton-encord Mar 5, 2026
445abc4
Fix more tests
clinton-encord Mar 5, 2026
84f0ac9
Fix more tests
clinton-encord Mar 5, 2026
75d0f98
Formatting error
clinton-encord Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions encord/objects/ontology_labels_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2201,12 +2201,13 @@ def _to_object_actions(self) -> Dict[str, ObjectAction]:
for obj in space._objects_map.values():
# Currently, dynamic attributes only available for VideoSpace
if isinstance(space, VideoSpace):
all_static_answers = space._dynamic_answers_to_encord_dict(obj)
all_static_answers = self._dynamic_answers_to_encord_dict(obj)
if len(all_static_answers) == 0:
continue

if obj.object_hash in ret:
ret[obj.object_hash]["actions"].extend(list(all_static_answers))
# The same object might still exist across object hashes
continue
else:
ret[obj.object_hash] = {
"actions": list(all_static_answers),
Expand Down Expand Up @@ -3056,19 +3057,17 @@ def _add_action_answers(self, label_row_dict: dict):
for answer in label_row_dict["object_actions"].values():
object_hash = answer["objectHash"]
object_instance = self._objects_map.get(object_hash)

answer_list = answer["actions"]
if object_instance is not None:
answer_list = answer["actions"]
object_instance.set_answer_from_list(answer_list)
else:
# Not great that we're looping through spaces, but usually not that many spaces on a label row
answer_list = answer["actions"]
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This assignment to answer_list is redundant as it was already assigned on line 3060. You can remove this line for better code clarity.

for answer_dict in answer_list:
space_id = answer_dict.get("spaceId")
if space_id is None:
raise LabelRowError("Object action does not contain spaceId")

space = self.get_space(id=space_id, type_="video")
space._set_answer_from_list(object_hash, answers_list=[answer_dict])
for space in self._space_map.values():
object_on_space = space._objects_map.get(object_hash)
if object_on_space is not None:
object_on_space.set_answer_from_list(answers_list=answer_list)
break

def _create_new_object_instance(self, frame_object_label: FrameObject, frame: int) -> ObjectInstance:
ontology = self._ontology.structure
Expand Down
7 changes: 0 additions & 7 deletions encord/objects/ontology_object_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,6 @@ def get_answer(
)

if attribute.dynamic:
self._operation_not_allowed_for_objects_on_space(
extended_message="For getting dynamic attributes for objects on a space, use VideoSpace.get_answer_on_frames."
)
return self._dynamic_answer_manager.get_answer(attribute, filter_answer, filter_frame)

static_answer = self._static_answer_map[attribute.feature_node_hash]
Expand Down Expand Up @@ -298,10 +295,6 @@ def set_answer(
)
elif frames is not None and attribute.dynamic is False:
raise LabelRowError("Setting frames is only possible for dynamic attributes.")
elif attribute.dynamic:
self._operation_not_allowed_for_objects_on_space(
extended_message="For setting dynamic attributes for objects on a space, use VideoSpace.set_answer_on_frames."
)

if attribute.dynamic:
self._dynamic_answer_manager.set_answer(answer, attribute, frames)
Expand Down
169 changes: 14 additions & 155 deletions encord/objects/spaces/multiframe_space/multiframe_space.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ def __init__(
# Global classifications are NOT tracked here
self._classifications_ontology_to_ranges: defaultdict[Classification, RangeManager] = defaultdict(RangeManager)

self._object_hash_to_dynamic_answer_manager: dict[str, DynamicAnswerManager] = dict()

# Need to check if this is 1-indexed
self._number_of_frames: int = number_of_frames

Expand Down Expand Up @@ -184,11 +182,6 @@ def _put_object_instance(
self._are_frames_valid(frame_list)
self._objects_map[object_instance.object_hash] = object_instance

if object_instance.object_hash not in self._object_hash_to_dynamic_answer_manager:
self._object_hash_to_dynamic_answer_manager[object_instance.object_hash] = DynamicAnswerManager(
object_instance
)

object_instance._add_to_space(self)

check_coordinate_type(coordinates, object_instance._ontology_object, self._label_row)
Expand Down Expand Up @@ -289,7 +282,6 @@ def put_object_instance(
def _remove_object_instance(self, object_instance: ObjectInstance) -> None:
object_hash = object_instance.object_hash
object_instance._remove_from_space(self.space_id)
self._object_hash_to_dynamic_answer_manager.pop(object_instance.object_hash)
self._object_hash_to_range_manager.pop(object_hash)

frames_to_remove: list[int] = []
Expand All @@ -313,9 +305,6 @@ def _remove_object_instance_from_frames(
) -> List[int]:
frame_list = frames_class_to_frames_list(frames)

# Remove all dynamic answers from these frames
self._remove_all_answers_from_frames(object_instance, frame_list)

# Tracks frames that are actually removed. User might have passed in frames that object doesn't even exist on.
frames_removed: list[int] = []

Expand All @@ -331,113 +320,9 @@ def _remove_object_instance_from_frames(
range_manager_for_object_hash.remove_ranges(temp_range_manager.get_ranges())
if len(range_manager_for_object_hash.get_ranges()) == 0:
self._objects_map.pop(object_instance.object_hash)
self._object_hash_to_dynamic_answer_manager.pop(object_instance.object_hash)

return frames_removed

def _remove_all_answers_from_frames(self, object_instance: ObjectInstance, frames: List[int]) -> None:
"""Remove all dynamic answers from the specified frames for an object instance.

Args:
object_instance: The object instance to remove answers from.
frames: List of frame numbers to remove answers from.
"""
dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager.get(object_instance.object_hash)
Copy link
Contributor Author

@clinton-encord clinton-encord Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this was initially used in ontology_labels_impl.py when doing add_action_answers, but we now use the ObjectInstance methods to do that instead.

if dynamic_answer_manager is None:
# No dynamic answers to remove
return

# Get all dynamic attributes for this object
dynamic_attributes = [attr for attr in object_instance._ontology_object.attributes if attr.dynamic]

# Remove answers for each dynamic attribute on the specified frames
for attribute in dynamic_attributes:
dynamic_answer_manager.delete_answer(attribute, frames=frames)

# This implementation is copied mostly from ObjectInstance.set_answer_from_list
def _set_answer_from_list(self, object_hash: str, answers_list: List[Dict[str, Any]]):
grouped_answers = defaultdict(list)
object_instance = self._objects_map[object_hash]
dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager[object_instance.object_hash]

for answer_dict in answers_list:
attribute = _get_attribute_by_hash(answer_dict["featureHash"], object_instance._ontology_object.attributes)
if attribute is None:
raise LabelRowError(
"One of the attributes does not exist in the ontology. Cannot create a valid LabelRow."
)
if not object_instance._is_attribute_valid_child_of_object_instance(attribute):
raise LabelRowError(
"One of the attributes set for a classification is not a valid child of the classification. "
"Cannot create a valid LabelRow."
)

grouped_answers[attribute.feature_node_hash].append(answer_dict)

for feature_hash, answers_list in grouped_answers.items():
attribute = _get_attribute_by_hash(feature_hash, object_instance._ontology_object.attributes)
assert attribute # we already checked that attribute is not null above. So just silencing this for now
self._set_answer_from_grouped_list(dynamic_answer_manager, attribute, answers_list)

def _set_answer_from_grouped_list(
self, dynamic_answer_manager: DynamicAnswerManager, attribute: Attribute, answers_list: List[Dict[str, Any]]
) -> None:
if isinstance(attribute, ChecklistAttribute):
if not attribute.dynamic:
raise LabelRowError("This method should not be called for non-dynamic attributes.")
else:
all_feature_hashes: Set[str] = set()
ranges = []
for answer_dict in answers_list:
feature_hashes: Set[str] = {answer["featureHash"] for answer in answer_dict["answers"]}
all_feature_hashes.update(feature_hashes)
for frame_range in ranges_list_to_ranges(answer_dict["range"]):
ranges.append((frame_range, feature_hashes))

options_cache = {
feature_hash: attribute.get_child_by_hash(feature_hash, type_=Option)
for feature_hash in all_feature_hashes
}

for frame_range, feature_hashes in ObjectInstance._merge_answers_to_non_overlapping_ranges(ranges):
options = [options_cache[feature_hash] for feature_hash in feature_hashes]
dynamic_answer_manager.set_answer(options, attribute, [frame_range])
else:
for answer in answers_list:
self._set_answer_from_dict(dynamic_answer_manager, answer, attribute)

def _set_answer_from_dict(
self, dynamic_answer_manager: DynamicAnswerManager, answer_dict: Dict[str, Any], attribute: Attribute
) -> None:
if not attribute.dynamic:
raise LabelRowError("This method should not be called for non-dynamic attributes.")

ranges = ranges_list_to_ranges(answer_dict["range"])

if isinstance(attribute, TextAttribute):
dynamic_answer_manager.set_answer(answer_dict["answers"], attribute, ranges)
elif isinstance(attribute, RadioAttribute):
if len(answer_dict["answers"]) == 1:
feature_hash = answer_dict["answers"][0]["featureHash"]
option = attribute.get_child_by_hash(feature_hash, type_=Option)
dynamic_answer_manager.set_answer(option, attribute, ranges)
elif isinstance(attribute, ChecklistAttribute):
options = []
for answer in answer_dict["answers"]:
feature_hash = answer["featureHash"]
option = attribute.get_child_by_hash(feature_hash, type_=Option)
options.append(option)
dynamic_answer_manager.set_answer(options, attribute, ranges)
elif isinstance(attribute, NumericAttribute):
value: float = answer_dict["answers"]

if not isinstance(value, float) and not isinstance(value, int):
raise LabelRowError(f"The answer for a numeric attribute must be a float or an int. Found {value}.")

dynamic_answer_manager.set_answer(value, attribute, ranges)
else:
raise NotImplementedError(f"The attribute type {type(attribute)} is not supported.")

def set_dynamic_answer(
self,
object_instance: ObjectInstance,
Expand All @@ -464,21 +349,21 @@ def set_dynamic_answer(
or if the object doesn't exist on the space yet.
"""
self._label_row._check_labelling_is_initalised()
if attribute is None:
attribute = _infer_attribute_from_answer(object_instance._ontology_object.attributes, answer)
if not object_instance._is_attribute_valid_child_of_object_instance(attribute):
raise LabelRowError("The attribute is not a valid child of the object.")
elif not attribute.dynamic and not object_instance._is_selectable_child_attribute(attribute):
raise LabelRowError(
"Setting a nested attribute is only possible if all parent attributes have been selected."
)
elif attribute.dynamic is False:

if object_instance.object_hash not in self._objects_map:
raise LabelRowError(
"This method should only be used for dynamic attributes. For static attributes, use `ObjectInstance.set_answer`."
"Object does not yet exist on this space. Place the object on this space with `Space.place_object`."
)

if attribute is None:
attribute = _infer_attribute_from_answer(object_instance._ontology_object.attributes, answer)

if not attribute.dynamic:
raise LabelRowError("This method should not be called for non-dynamic attributes.")

frames_list = frames_class_to_frames_list(frames)

# Check that frames do exist on this object on this space
valid_frames = []
for frame in frames_list:
annotation_data = self._get_frame_object_annotation_data(
Expand All @@ -487,13 +372,7 @@ def set_dynamic_answer(
if annotation_data is not None:
valid_frames.append(frame)

dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager.get(object_instance.object_hash)
if dynamic_answer_manager is None:
raise LabelRowError(
"Object does not yet exist on this space. Place the object on this space with `Space.place_object`."
)

dynamic_answer_manager.set_answer(answer, attribute, frames=valid_frames)
object_instance.set_answer(answer, attribute, frames=valid_frames)

def remove_dynamic_answer(
self,
Expand All @@ -518,13 +397,7 @@ def remove_dynamic_answer(
if not attribute.dynamic:
raise LabelRowError("This method should not be called for non-dynamic attributes.")

dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager.get(object_instance.object_hash)
if dynamic_answer_manager is None:
raise LabelRowError(
"Object does not yet exist on this space. Place the object on this space with `Space.place_object`."
)

dynamic_answer_manager.delete_answer(attribute, frames=frame, filter_answer=filter_answer)
object_instance._dynamic_answer_manager.delete_answer(attribute, frames=frame, filter_answer=filter_answer)

def get_dynamic_answer(
self,
Expand All @@ -550,14 +423,13 @@ def get_dynamic_answer(
LabelRowError: If the attribute is not dynamic or if the object doesn't exist on the space.
"""
self._label_row._check_labelling_is_initalised()
dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager.get(object_instance.object_hash)
if dynamic_answer_manager is None:
if object_instance.object_hash not in self._objects_map:
raise LabelRowError("This object does not exist on this space.")

if not attribute.dynamic:
raise LabelRowError("This method should only be used for dynamic attributes.")

return dynamic_answer_manager.get_answer(attribute, filter_answer, filter_frames=frames)
return object_instance._dynamic_answer_manager.get_answer(attribute, filter_answer, filter_frames=frames)

def put_classification_instance(
self,
Expand Down Expand Up @@ -933,19 +805,6 @@ def _get_frame_classification_annotation_data(
else:
return classification_to_frame_annotation_data.get(classification_hash)

def _dynamic_answers_to_encord_dict(self, object_instance: ObjectInstance) -> List[DynamicAttributeObject]:
ret = []
dynamic_answer_manager = self._object_hash_to_dynamic_answer_manager[object_instance.object_hash]

if dynamic_answer_manager is None:
raise LabelRowError("No dynamic answers found for this object instance on this space.")

for answer, ranges in dynamic_answer_manager.get_all_answers():
d_opt = answer.to_encord_dict(ranges, space_id=self.space_id)
if d_opt is not None:
ret.append(cast(DynamicAttributeObject, d_opt))
return ret

def _to_encord_object(
self,
object_instance: ObjectInstance,
Expand Down
12 changes: 9 additions & 3 deletions tests/objects/data/data_group/two_videos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
}


DATA_GROUP_METADATA = LabelRowMetadata(
DATA_GROUP_WITH_TWO_VIDEOS_METADATA = LabelRowMetadata(
label_hash="",
branch_name="main",
created_at=datetime.datetime.now(),
Expand Down Expand Up @@ -121,6 +121,12 @@
"classification1": {
"classificationHash": "classification1",
"featureHash": "jPOcEsbw",
"spaces": {
"video-1-uuid": {
"range": [[0, 0]],
"type": "frame",
},
},
"classifications": [
{
"name": "Text classification",
Expand All @@ -145,7 +151,7 @@
"featureHash": "OTkxMjU1",
"shouldPropagate": False,
"manualAnnotation": True,
"spaceId": "video-1-uuid",
"trackHash": "fbb97dda-1e66-48f9-b749-af2f83dab9fc",
},
{
"name": "First name",
Expand All @@ -156,7 +162,7 @@
"featureHash": "OTkxMjU1",
"shouldPropagate": False,
"manualAnnotation": True,
"spaceId": "video-2-uuid",
"trackHash": "fbb97dda-1e66-48f9-b749-af2f83dab9fc",
},
],
},
Expand Down
Loading
Loading