-
Notifications
You must be signed in to change notification settings - Fork 510
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
Support loading annotations for large CVAT tasks with many jobs #4392
Conversation
WalkthroughThe recent updates enhance the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (19)
tests/intensive/cvat_tests.py (7)
Line range hint
13-13
: Remove unused importcollections.defaultdict
.- from collections import defaultdict
Line range hint
14-14
: Remove unused importnumpy
.- import numpy as np
Line range hint
380-381
: Local variablesapi
andtask_id
are assigned but never used in thetest_multiple_fields
method.Consider removing or using these variables if they were intended for future use.
Line range hint
694-694
: Local variableperson_labels
is assigned but never used in thetest_example_restricting_label_edits
method.Consider removing or using this variable if it was intended for future use.
Line range hint
816-816
: Local variablestatus
is assigned but never used in thetest_deleted_tasks
method.Consider removing or using this variable if it was intended for future use.
Line range hint
892-892
: Ambiguous variable namel
in thetest_project_exists
method.Consider renaming
l
to a more descriptive name to improve code readability.
Line range hint
1327-1327
: Local variableresults
is assigned but never used in thetest_frame_start_stop_step
method.Consider removing or using this variable if it was intended for future use.
fiftyone/utils/cvat.py (12)
Line range hint
3439-3439
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
3474-3474
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
3988-3988
: Remove the unused local variableresponse
.- response = self.get(self.task_data_meta_url(task_id)).json()
Line range hint
4151-4151
: Remove the unused local variableresponse
.- response = self.get(self.task_data_meta_url(task_id)).json()
Line range hint
6075-6075
: Remove the unused local variableclasses
.- classes = label_info["classes"]
Line range hint
6720-6720
: Remove the unused local variableformatted_track
.- formatted_track = self._finalize_track(track, frame_count, only_keyframes)
Line range hint
6856-6856
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
6933-6933
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
7409-7409
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
7418-7418
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
7423-7423
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Line range hint
7829-7829
: Use a specific exception type instead of a bareexcept
.- except: + except Exception as e:
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- fiftyone/utils/cvat.py (4 hunks)
- tests/intensive/cvat_tests.py (1 hunks)
Additional Context Used
Ruff (20)
fiftyone/utils/cvat.py (12)
3439-3439: Do not use bare
except
3474-3474: Do not use bare
except
3988-3988: Local variable
response
is assigned to but never used
4151-4151: Local variable
response
is assigned to but never used
6075-6075: Local variable
classes
is assigned to but never used
6720-6720: Local variable
formatted_track
is assigned to but never used
6856-6856: Do not use bare
except
6933-6933: Do not use bare
except
7409-7409: Do not use bare
except
7418-7418: Do not use bare
except
7423-7423: Do not use bare
except
7829-7829: Do not use bare
except
tests/intensive/cvat_tests.py (8)
13-13:
collections.defaultdict
imported but unused
14-14:
numpy
imported but unused
380-380: Local variable
api
is assigned to but never used
381-381: Local variable
task_id
is assigned to but never used
694-694: Local variable
person_labels
is assigned to but never used
816-816: Local variable
status
is assigned to but never used
892-892: Ambiguous variable name:
l
1327-1327: Local variable
results
is assigned to but never used
Additional comments not posted (1)
tests/intensive/cvat_tests.py (1)
309-309
: Ensure the newsegment_size
parameter is appropriately tested.Verification successful
The verification process has confirmed that the
segment_size
parameter is indeed tested within thetest_detection_labelling
method in thecvat_tests.py
file. The output from the script shows thatsegment_size
is set to 1 in two instances, and there is an assertion checking that thesegment_size
in thetask_json
matches the expected value of 1. This indicates that the parameter is being appropriately tested as per the requirements mentioned in the review comment.Final Response:
The
segment_size
parameter is appropriately tested in thetest_detection_labelling
method as evidenced by the assertion checking its value. No further action is required regarding this parameter's testing.
- Location:
tests/intensive/cvat_tests.py
- Assertion:
self.assertEqual(task_json["segment_size"], 1)
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new `segment_size` parameter is tested in the `test_detection_labelling` method. # Test: Search for the usage of `segment_size` in test assertions. Expect: At least one assertion involving `segment_size`. rg --type py 'segment_size' 'tests/intensive/cvat_tests.py'Length of output: 176
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (12)
fiftyone/utils/cvat.py (12)
Line range hint
3440-3440
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
3475-3475
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
3989-3989
: Remove the unused local variableresponse
.- response = self.get(self.task_data_meta_url(task_id)).json()
Line range hint
4152-4152
: Remove the unused local variableresponse
.- response = self.put(self.task_annotation_url(task_id), json=anno_json).json()
Line range hint
6078-6078
: Remove the unused local variableclasses
.- classes = label_info["classes"]
Line range hint
6723-6723
: Remove the unused local variableformatted_track
.- formatted_track = self._finalize_track(track, frame_count, only_keyframes)
Line range hint
6859-6859
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
6936-6936
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
7412-7412
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
7421-7421
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
7426-7426
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Line range hint
7832-7832
: Replace the bareexcept
with a more specific exception type.- except: + except Exception as e:
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/utils/cvat.py (7 hunks)
Additional Context Used
Ruff (12)
fiftyone/utils/cvat.py (12)
3440-3440: Do not use bare
except
3475-3475: Do not use bare
except
3989-3989: Local variable
response
is assigned to but never used
4152-4152: Local variable
response
is assigned to but never used
6078-6078: Local variable
classes
is assigned to but never used
6723-6723: Local variable
formatted_track
is assigned to but never used
6859-6859: Do not use bare
except
6936-6936: Do not use bare
except
7412-7412: Do not use bare
except
7421-7421: Do not use bare
except
7426-7426: Do not use bare
except
7832-7832: Do not use bare
except
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.
Implementation LGTM 💪
@ehofesmann if you retarget this at release/v0.24.0
we can include in the release this week 🤓
Thanks @brimoor ! Just getting back to this now, I assume I missed the window on this. I do still need to get it into teams too. It's OK if it doesn't make it into v0.24.0. @benjaminpkane I see you changed the base back to |
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 👍
What changes are proposed in this pull request?
Optimized loading annotations from the CVAT backend. Annotations are now loaded from individual jobs instead of entire tasks which allows for importing annotations from much larger task sizes. There is one task in the internal CVAT deployment with 10k samples, in 200 jobs of 50 samples. Previously, trying to load this task would make a single request to the CVAT server to load all annotations from the task at once, this crashes the CVAT server. Now, annotations from each job are loaded sequentially which resolves this problem.
How is this patch tested? If it is not, please explain why.
Unit tests pass:
Also task 159 on the internal CVAT test deployment containing bdd100k-validation now imports properly. It is recommended you have bdd100k validation images available locally on disk as it makes this easier:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Optimized loading annotations from the CVAT backend. Annotations are now loaded from individual jobs instead of entire tasks which allows for importing annotations from much larger task sizes.
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit