[PLA-639][External] Allow properties to be imported for annotations without IDs #803
Conversation
This file contains 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
JBWilkie
commented
Mar 19, 2024
darwin/importer/importer.py
Outdated
Comment on lines
1312
to
1315
| else: | ||
| annotation_id = str(uuid.uuid4()) | ||
| annotation.id = annotation_id | ||
| serial_obj["id"] = annotation_id |
There was a problem hiding this comment.
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!
JBWilkie
commented
Mar 19, 2024
Comment on lines
+575
to
+586
| 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.
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
Co-authored-by: saurbhc <sc@saurabhchopra.co.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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