-
Notifications
You must be signed in to change notification settings - Fork 583
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
Provide custom task name for CVAT #2353
Provide custom task name for CVAT #2353
Conversation
@ehofesmann just have a couple of notes about this implementation:
|
fiftyone/utils/cvat.py
Outdated
_dataset_name = samples_batch._dataset.name.replace( | ||
" ", "_" | ||
) | ||
latest_anno_key = _get_latest_anno_key(samples) |
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 is a clever workaround to get the anno_key
, but now that config.task_name
is supported, I think it would be best to just rely on the user putting their anno_key
in the task name if that's what they want.
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.
It is unfortunate that we didn't include anno_key
in either the upload_annotations()
method or in the AnnotationBackendConfig
, because it's definitely reasonable to want to use it to generate default values like this 🤦
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.
I guess it's enough for me that I can provide a custom task name to differentiate between tasks for the same dataset.
Do you think that will require a lot of/breaking changes to pass anno_key
to upload_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.
In cases where there are more than one task in the run, what do you think about treating tasks_name
as a base name and appending 1
, 2
, 3
, etc to it?
Would not that require getting the count of annotation keys as well? |
No I'm suggesting adding a separate enhancement to this PR: a single annotation run may create multiple tasks (eg 1 per video for video collections, or if the |
Oh yeah that can be done. How about the default task name? Do we include an annotation key or not? |
I don't think we should include annotation key in the default task name because there's no official way to get that information into |
c0968c7
to
12208fb
Compare
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.
LGTM, thanks for the contribution!
What changes are proposed in this pull request?
Closes #1753
dataset.annotate("anno_key", backend="cvat", task_name="Custom task name", ...)
FiftyOne_{dataset_name}_{annotation_key}
How is this patch tested? If it is not, please explain why.
added few lines to existing cvat unit-tests.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
A custom task name can be provided with CVAT annotation backend. Default CVAT task name now includes an annotation key.
What areas of FiftyOne does this PR affect?
fiftyone
Python library changes