Skip to content

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

Closed
clinton-encord wants to merge 11 commits intomasterfrom
clinton/ed-2306/rollback-dynamic-attributes
Closed

fix: ED-2306 Make dynamic attributes unaware of spaces#1084
clinton-encord wants to merge 11 commits intomasterfrom
clinton/ed-2306/rollback-dynamic-attributes

Conversation

@clinton-encord
Copy link
Contributor

@clinton-encord clinton-encord commented Mar 5, 2026

Introduction and Explanation

  1. Dynamic attributes now longer work on a per-space basis.
  2. We use the original object's dynamic answer manager to handle dynamic attributes
  3. This means if an object exists on two spaces, and we set the dynamic attribute on frame 5 for either space, that attribute will apply to frame 5 for BOTH spaces

Backwards compatibitlity:

  • Before, we were adding a special "spaceId" to the object_actions to make it space-aware
  • Now that doesn't break anything, because that will still show up on the UI, just the UI will assume it applies to all spaces

JIRA

Link ticket(s)

Documentation

There should be enough internal documentation for a product owner to write customer-facing documentation or a separate PR linked if writing the customer documentation directly. Link all that are relevant below.

  • Internal: notion link
  • Customer docs PR: link
  • OpenAPI/SDK
    • Generated docs: link to example if possible
    • Command to generate: here

Tests

Some unit tests!

Known issues

If there are any known issues with the solution, make a statement about what they are and why they are Ok to leave unsolved for now. Make tickets for the known issues linked to the original ticket linked above

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Unit test report (Python 3.9.24, Pydantic 2.12.3)

510 tests   510 ✅  13s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 75d0f98.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Unit test report (Python 3.9.24, Pydantic 1.10.22)

510 tests   510 ✅  12s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 75d0f98.

♻️ This comment has been updated with latest results.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the handling of dynamic attributes to make them more self-contained within the ObjectInstance. The change streamlines the API for dynamic attributes, allowing ObjectInstance methods to directly manage them without requiring specific VideoSpace context. This also involves an update to the internal data model, shifting from spaceId to trackHash for classifications, and includes corresponding updates to the test suite to validate the new behavior.

Highlights

  • Dynamic Attribute Management Refactor: The core logic for handling dynamic attributes has been refactored to centralize management within the ObjectInstance rather than being spread across VideoSpace or LabelRowV2 methods. This simplifies the API and internal consistency.
  • API Simplification for Dynamic Attributes: Methods like get_answer and set_answer on ObjectInstance can now directly handle dynamic attributes, removing previous restrictions that required delegation to VideoSpace specific methods.
  • Internal Data Structure Update: The internal representation of classification answers has been updated to use trackHash instead of spaceId, aligning with a more object-centric approach for dynamic attributes.
  • Code Removal and Delegation: Several redundant methods related to setting dynamic answers on multiframe_space have been removed, as their functionality is now delegated to the ObjectInstance.
  • Test Suite Updates: Existing test cases have been updated to reflect the new dynamic attribute handling, and a new test has been added to ensure proper serialization and deserialization of video group labels with dynamic attributes.
Changelog
  • encord/objects/ontology_labels_impl.py
    • Refactored dynamic attribute collection to use self for _dynamic_answers_to_encord_dict.
    • Improved handling of existing object hashes to prevent duplicate action entries.
    • Reworked _add_action_answers to delegate dynamic attribute setting to the ObjectInstance found within any space.
  • encord/objects/ontology_object_instance.py
    • Removed _operation_not_allowed_for_objects_on_space checks for dynamic attributes in get_answer and set_answer, allowing ObjectInstance to directly manage them.
  • encord/objects/spaces/multiframe_space/multiframe_space.py
    • Delegated dynamic attribute deletion to ObjectInstance's _dynamic_answer_manager.
    • Removed redundant methods (_set_answer_from_list, _set_answer_from_grouped_list, _set_answer_from_dict) as dynamic attribute setting is now handled by ObjectInstance.
    • Updated set_dynamic_answer, remove_dynamic_answer, and get_dynamic_answer to call corresponding methods on the ObjectInstance directly, simplifying the VideoSpace's role.
    • Modified set_dynamic_answer to validate object existence on the space before setting answers.
  • tests/objects/data/data_group/two_videos.py
    • Renamed metadata variable DATA_GROUP_METADATA to DATA_GROUP_WITH_TWO_VIDEOS_METADATA for clarity.
    • Updated test data structure to include spaces for classifications and replaced spaceId with trackHash in classification answers.
  • tests/objects/spaces/test_multi_frame_space/test_dynamic_attributes.py
    • Updated test cases to use the renamed metadata variable.
    • Modified a test to directly call ObjectInstance.set_answer for dynamic attributes, reflecting the new API.
  • tests/objects/spaces/test_serde.py
    • Added a new test case (test_read_and_export_video_group_with_dynamic_attributes) to verify the serialization and deserialization of video group labels with dynamic attributes, ensuring compatibility with the updated structure.
Activity
  • No specific human activity (comments, reviews, or progress updates) has been recorded for this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the handling of dynamic attributes to make them 'space-unaware'. The core change involves moving the management of dynamic attributes from Space classes to the ObjectInstance itself. This is a good architectural improvement that centralizes the logic and simplifies the Space implementations. The changes are well-supported by updates to test data and the addition of a new end-to-end serialization test. I've left a couple of minor suggestions for code cleanup.

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

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.

# Act
new_object_instance.set_answer(frames=[0], attribute=key_point_dynamic_text_attribute, answer=answer_on_frame_0)

# # Act
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This commented-out line appears to be a leftover from refactoring. It should be removed to improve test readability.

Comment on lines +54 to +55
#
# # Assert
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

These commented-out lines appear to be leftovers from refactoring and can be removed to improve test readability.

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

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.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

SDK integration test report

  1 files  ±0    1 suites  ±0   13m 8s ⏱️ - 9m 21s
289 tests ±0  280 ✅ +1  4 💤 ±0  4 ❌  - 1  1 🔥 ±0 
290 runs  ±0  280 ✅ +1  4 💤 ±0  4 ❌  - 1  2 🔥 ±0 

For more details on these failures and errors, see this check.

Results for commit 75d0f98. ± Comparison against base commit 552a77d.

♻️ This comment has been updated with latest results.

Comment on lines 336 to 345
def set_dynamic_answer(
self,
object_instance: ObjectInstance,
frames: Frames,
answer: Union[str, NumericAnswerValue, Option, Sequence[Option]],
attribute: Optional[Attribute] = None,
) -> None:
"""Set dynamic attribute answers for an object instance on specific frames.

This method is only for dynamic attributes. For static attributes, use `ObjectInstance.set_answer`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually would prefer to just raise an error here now, until we properly implement space-aware dynamic attributes, but afraid it will break people's existing scripts...

@clinton-encord clinton-encord changed the title fix: ED-2306 Make dynamic attributes unaware fix: ED-2306 Make dynamic attributes unaware of spaces Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant