-
Notifications
You must be signed in to change notification settings - Fork 42
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
[PLA-639][External] Allow properties to be imported for annotations without IDs #803
Conversation
darwin/importer/importer.py
Outdated
else: | ||
annotation_id = str(uuid.uuid4()) | ||
annotation.id = annotation_id | ||
serial_obj["id"] = annotation_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives every annotation an ID if one isn't present, even those that we're not importing properties to. Because of this, I'm not extremely happy with it as a solution, but it is by far the simplest solution I could think of. IDs have to be added here before _import_properties()
& _update_payload_with_properties()
I don't think inefficiency this will have a significant impact. However, I'm open to feedback on alternative approaches!
assert ( | ||
output["annotations"][0]["annotation_class_id"] | ||
== assertion["annotations"][0]["annotation_class_id"] | ||
) | ||
assert output["annotations"][0]["data"] == assertion["annotations"][0]["data"] | ||
assert ( | ||
output["annotations"][0]["actors"] == assertion["annotations"][0]["actors"] | ||
) | ||
assert ( | ||
output["annotations"][0]["context_keys"] | ||
== assertion["annotations"][0]["context_keys"] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the importer now assigns a random UUID, comparing the entire annotations
field for each no longer works. Instead, we need to compare the individual fields inside annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🙏
Co-authored-by: saurbhc <sc@saurabhchopra.co.uk>
Problem
Currently, annotations with properties can only be imported if the annotation has an ID in the JSON file. Annotation IDs are not required for any other import functionality, so we should remove this requirement
Solution
When importing annotations, if an annotation does not have an ID, give it one with
uuid.uuid4()
. This works because we don't actually respect the ID that's assigned during import (to avoid clashes), it just needs to be a unique value per annotationChangelog
Removed the requirement for annotation IDs to be present in imported JSON to successfully import properties