-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add MetadataAnnotations class for user-facing handling of annotations #503
Conversation
Add autoloading of 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.
All very cool features, added some comments and questions.
Thanks!
setup.py
Outdated
"dagshub-annotation-converter @ " | ||
+ "git+https://github.com/DagsHub/" | ||
+ "dagshub-annotation-converter@refactor/static-annotations#egg=dagshub-annotation-converter", |
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.
- Remember
if "load_into_memory" in kwargs: | ||
del kwargs["load_into_memory"] | ||
self.get_blob_fields(*self.annotation_fields, load_into_memory=True, **kwargs) |
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 bit is unclear to me. You discard load_into_memory
and force it to be true
. Is there a common scenario where you pass load_into_memory=False
explicitly to get_annotations
, and expect this to happen?
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.
No, however I pass the kwargs to the downloading functions, and loading annotations requires them to be already in memory, so load_into_memory=False
will break things.
Actually I should probably handle this particular edge case better in get_blob_fields
and either skip loading if load_into_memory=False
(explicitly respecting the user, even if it doesn't make sense), or force-opening.
- Get rid of the yolo_context argument in add_yolo_annotation, instead using a categories dictionary
# Conflicts: # requirements-dev.txt
Scope of this PR:
MetadataAnnotations
class. Objects of this class will get created for any annotation field in the QueryResult, so you can use them like this:ds.all()
andds.head()
load_documents
andload_annotations
kwargs in these functions