Skip to content
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.

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

[DAR-3919][External] Warn when importing annotations with multiple instance IDs #929

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

JBWilkie
Copy link
Collaborator

@JBWilkie JBWilkie commented Sep 23, 2024

Problem

With the release of static instance IDs, if uploading an annotation with multiple instance IDs - Only the value of the instance ID on the first keyframe is used for import

This may confuse users who expect static instance IDs to operate like dynamic instance IDs, which is the current behaviour

Solution

If the static instance IDs feature flag is present & enabled, OR if the flag doesn't exist, we perform the following check: For each frame of every video annotation check sub-annotations. If there is >1 unique instance ID value in any annotation, record this

After checking all annotations, print a warning for each affected file as follows:

The following files have annotation(s) with multiple instance ID values. Instance IDs are static, so only the first instance ID of each annotation will be imported::
- File: {local_annotation_file_path_1} has {n_1} annotation(s) with multiple instance IDs
- File: {local_annotation_file_path_2} has {n_2} annotation(s) with multiple instance IDs

We check if the flag is present so that the code will still work when the flag is deprecated in the future. From this point onward, we will always perform the check

Changelog

Added a warning when importing annotations with multiple instance ID values where only 1 value will be imported due to static instance IDs

Copy link

linear bot commented Sep 23, 2024

# Check if the flag exists. When the flag is deprecated in the future we will always perform this check
static_instance_id_feature_flag_exists = any(
feature.name == "STATIC_INSTANCE_ID"
for feature in dataset.client.features.get(dataset.team, [])
Copy link
Contributor

@dorfmanrobert dorfmanrobert Sep 24, 2024

Choose a reason for hiding this comment

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

why do we need this static_instance_id_feature_flag_exists and associated check? it exists, so seems this is unneeded

Copy link
Collaborator Author

@JBWilkie JBWilkie Sep 25, 2024

Choose a reason for hiding this comment

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

We only want to run the validation that this PR introduces (_warn_for_annotations_with_multiple_instance_ids) if:

  • 1: The STATIC_INSTANCE_ID feature flag exists and it's value for the target workspace is True, or:
  • 2: The flag doesn't exist. At this point, the feature will be globally enabled so we will always want to run the check. We'll be able to remove this section of code (apart from the function call to _warn_for_annotations_with_multiple_instance_ids, but the flag will be removed before we can update darwin-py

Copy link
Contributor

Choose a reason for hiding this comment

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

oooh makes sense thanks!

@JBWilkie JBWilkie merged commit b1f3e21 into master Sep 25, 2024
24 checks passed
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.

2 participants