Creating Consensus Job #7974
Creating Consensus Job #7974
Conversation
…rce_job_id under Job
Viditagarwal7479
requested review from
zhiltsov-max,
bsekachev,
SpecLad and
Marishka17
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce two new properties, Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant UI as CVAT UI
participant Backend as CVAT Backend
participant DB as Database
User->>UI: Create Task
UI->>Backend: Send Task Data (including consensusJobPerSegment)
Backend->>DB: Save Task Data
DB-->>Backend: Confirmation
Backend-->>UI: Task Created
UI-->>User: Task Creation Confirmation
User->>UI: View Task Details
UI->>Backend: Fetch Task Details
Backend->>DB: Retrieve Task Data
DB-->>Backend: Task Data (including consensusJobPerSegment)
Backend-->>UI: Task Details
UI-->>User: Display Task Details
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (22)
- cvat-core/src/server-response-types.ts (2 hunks)
- cvat-core/src/session-implementation.ts (1 hunks)
- cvat-core/src/session.ts (6 hunks)
- cvat-sdk/cvat_sdk/core/proxies/tasks.py (2 hunks)
- cvat-ui/src/actions/tasks-actions.ts (2 hunks)
- cvat-ui/src/components/create-task-page/advanced-configuration-form.tsx (4 hunks)
- cvat/apps/dataset_manager/bindings.py (2 hunks)
- cvat/apps/dataset_manager/project.py (1 hunks)
- cvat/apps/dataset_manager/task.py (1 hunks)
- cvat/apps/engine/admin.py (1 hunks)
- cvat/apps/engine/backup.py (1 hunks)
- cvat/apps/engine/migrations/0079_job_source_job_id_task_consensus_job_per_segment.py (1 hunks)
- cvat/apps/engine/migrations/0080_alter_job_source_job_id.py (1 hunks)
- cvat/apps/engine/migrations/0081_alter_job_source_job_id.py (1 hunks)
- cvat/apps/engine/migrations/0082_alter_task_consensus_job_per_segment.py (1 hunks)
- cvat/apps/engine/migrations/0083_alter_task_consensus_job_per_segment.py (1 hunks)
- cvat/apps/engine/migrations/0084_remove_task_consensus_job_per_segment_and_more.py (1 hunks)
- cvat/apps/engine/migrations/0085_alter_data_consensus_job_per_segment.py (1 hunks)
- cvat/apps/engine/migrations/0086_alter_data_consensus_job_per_segment.py (1 hunks)
- cvat/apps/engine/models.py (2 hunks)
- cvat/apps/engine/serializers.py (9 hunks)
- cvat/apps/engine/task.py (1 hunks)
Files skipped from review due to trivial changes (9)
- cvat/apps/dataset_manager/project.py
- cvat/apps/engine/backup.py
- cvat/apps/engine/migrations/0079_job_source_job_id_task_consensus_job_per_segment.py
- cvat/apps/engine/migrations/0080_alter_job_source_job_id.py
- cvat/apps/engine/migrations/0081_alter_job_source_job_id.py
- cvat/apps/engine/migrations/0082_alter_task_consensus_job_per_segment.py
- cvat/apps/engine/migrations/0083_alter_task_consensus_job_per_segment.py
- cvat/apps/engine/migrations/0085_alter_data_consensus_job_per_segment.py
- cvat/apps/engine/migrations/0086_alter_data_consensus_job_per_segment.py
Additional comments not posted (27)
cvat/apps/engine/migrations/0084_remove_task_consensus_job_per_segment_and_more.py (2)
13-16: The removal of theconsensus_job_per_segmentfield from thetaskmodel is correctly implemented.
17-21: The addition of theconsensus_job_per_segmentfield to thedatamodel with a default value and allowing blank entries is correctly implemented.cvat/apps/engine/admin.py (1)
62-62: The addition ofconsensus_job_per_segmentto theDataAdminfields and marking it as read-only is correctly implemented.cvat-ui/src/actions/tasks-actions.ts (2)
216-216: Initialization ofconsensus_job_per_segmentwith a default value of0in the task creation description is correctly implemented.
255-257: The conditional setting ofconsensus_job_per_segmentbased on user input is correctly implemented.cvat-core/src/server-response-types.ts (2)
123-123: The addition ofconsensus_job_per_segmentto theSerializedTaskinterface is correctly implemented.
150-150: The addition ofsource_job_idto theSerializedJobinterface, allowing bothnumberandnull, is correctly implemented.cvat-sdk/cvat_sdk/core/proxies/tasks.py (1)
Line range hint
81-99: The addition ofconsensus_job_per_segmentwith a default value of0in theupload_datamethod, and its inclusion in thekeeplist for data filtering, is correctly implemented.cvat-ui/src/components/create-task-page/advanced-configuration-form.tsx (2)
52-52: The addition of the optionalconsensusJobPerSegmentfield to theAdvancedConfigurationinterface is correctly implemented.
330-337: The implementation of therenderConsensusJobPerSegmentmethod, including validation for non-negative integers and its placement in the form, is correctly implemented.cvat-core/src/session-implementation.ts (1)
486-486: The addition ofconsensus_job_per_segmentto the task specification is correctly implemented.Verification successful
The addition of
consensus_job_per_segmentis correctly handled in various parts of the system, includingtasks-actions.ts,session.ts,session-implementation.ts, andserver-response-types.ts.
cvat-ui/src/actions/tasks-actions.ts:
- Line:
consensus_job_per_segment: 0,- Line:
description.consensus_job_per_segment = data.advanced.consensusJobPerSegment;cvat-core/src/session.ts:
- Line:
consensus_job_per_segment: undefined,- Line:
get: () => data.consensus_job_per_segment,cvat-core/src/session-implementation.ts:
- Line:
consensus_job_per_segment: this.consensusJobPerSegment,cvat-core/src/server-response-types.ts:
- Line:
consensus_job_per_segment: number;Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `consensus_job_per_segment` is correctly handled in all related parts of the system. # Test: Search for the usage of `consensus_job_per_segment` in the TypeScript files. rg --type typescript $'consensus_job_per_segment'Length of output: 83
Script:
#!/bin/bash # Description: Verify that `consensus_job_per_segment` is correctly handled in all related parts of the system. # Test: Search for the usage of `consensus_job_per_segment` in all TypeScript files. rg 'consensus_job_per_segment' --glob '*.ts'Length of output: 599
cvat/apps/dataset_manager/task.py (1)
733-733: Ensure proper filtering of jobs based onsource_job_id.This change correctly implements the filtering of annotation jobs to exclude those that are sourced from other jobs, aligning with the new functionality for consensus jobs.
cvat/apps/engine/models.py (2)
235-235: Ensure the default value forconsensus_job_per_segmentaligns with system requirements.Verification successful
The default value for
consensus_job_per_segmentis consistently set to 0 across the codebase.
cvat/apps/engine/models.py:consensus_job_per_segment = models.IntegerField(default=0, blank=True)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the default value of 0 for consensus_job_per_segment is consistent across all related files. # Test: Search for the default value across all Python files. Expect: Consistent default value of 0. rg --type python 'consensus_job_per_segment' --files-with-matches | xargs rg 'default=0'Length of output: 5318
681-681: Check if allowingnullforsource_job_idaligns with the data integrity requirements of the system.Verification successful
Allowing
nullforsource_job_idaligns with the data integrity requirements of the system.The search results indicate that
source_job_idis used in various parts of the codebase, including models, serializers, and migrations. Specifically, the following points are noteworthy:
- In
cvat/apps/engine/models.py,source_job_idis defined asnull=True, blank=True, default=None.- In
cvat/apps/engine/serializers.py,source_job_idis aReadOnlyFieldwithallow_null=True.- In
cvat/apps/dataset_manager/task.py, there is an instance wheresource_job_idis explicitly set toNone.These points suggest that the system is designed to handle
nullvalues forsource_job_id, and this is consistent across the codebase.
- cvat/apps/engine/models.py
- cvat/apps/engine/serializers.py
- cvat/apps/dataset_manager/task.py
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that allowing null values for source_job_id is consistent and necessary across the system. # Test: Search for the usage of source_job_id in Python files. Expect: Proper handling of null values. rg --type python 'source_job_id' --files-with-matches | xargs rg 'if.*source_job_id'Length of output: 115
Script:
#!/bin/bash # Description: Verify that allowing null values for source_job_id is consistent and necessary across the system. # Test: Search for the usage of source_job_id in Python files. Expect: Proper handling of null values. rg --type py 'source_job_id' --files-with-matches | xargs rg 'if.*source_job_id'Length of output: 80
Script:
#!/bin/bash # Description: Gather more information about the usage and constraints of source_job_id. # Search for all occurrences of source_job_id in the codebase. rg --type py 'source_job_id' # Check for any related database constraints or migrations. fd -e sql -e py | xargs rg 'source_job_id'Length of output: 2725
cvat-core/src/session.ts (5)
457-457: Addition ofsource_job_idto theJobclass.This change aligns with the PR's objective to track the source job of a task, facilitating the creation of consensus jobs.
592-594: Getter forsource_job_idin theJobclass.Proper encapsulation of
source_job_idwith a getter ensures that the property is accessed in a controlled manner, which is a good practice in object-oriented programming.
674-674: Addition ofconsensusJobPerSegmentto theTaskclass.This addition supports the new functionality of handling consensus jobs per segment, as described in the PR summary.
729-729: Initialization ofconsensus_job_per_segmentin theTaskconstructor.Initializing this property in the constructor ensures that it is set up correctly when an instance of
Taskis created.
915-917: Getter forconsensusJobPerSegmentin theTaskclass.The implementation of a getter for
consensusJobPerSegmentfollows best practices for data encapsulation and provides a clean API for accessing this property.cvat/apps/dataset_manager/bindings.py (2)
678-678: Ensure proper handling ofsource_job_idin the metadata.Verification successful
Ensure proper handling of
source_job_idin the metadata.
cvat/apps/engine/serializers.py:source_job_idis defined as aReadOnlyField.cvat/apps/engine/models.py:source_job_idis defined as aPositiveIntegerField.cvat/apps/engine/task.py:source_job_idis used in theJobmodel.cvat/apps/dataset_manager/task.py:source_job_idis used in a query filter.cvat/apps/dataset_manager/bindings.py:source_job_idis included in the metadata.These usages indicate that
source_job_idis properly handled in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `source_job_id` is properly handled in all relevant places. # Test: Search for the usage of `source_job_id`. Expect: Proper handling in all relevant places. rg --type python $'source_job_id'Length of output: 63
Script:
#!/bin/bash # Description: Verify that `source_job_id` is properly handled in all relevant places. # Test: Search for the usage of `source_job_id`. Expect: Proper handling in all relevant places. rg 'source_job_id' --glob '*.py'Length of output: 1359
783-783: Ensureconsensus_job_per_segmentis properly integrated and used where necessary.Verification successful
Ensure
consensus_job_per_segmentis properly integrated and used where necessary.The search results indicate that
consensus_job_per_segmentis indeed integrated and used in various parts of the codebase, including serializers, models, migrations, and other modules.
- Serializers:
cvat/apps/engine/serializers.py- Task Processing:
cvat/apps/engine/task.py- Backup:
cvat/apps/engine/backup.py- Admin:
cvat/apps/engine/admin.py- Models:
cvat/apps/engine/models.py- Migrations: Multiple migration files
- Project Management:
cvat/apps/dataset_manager/project.py- Bindings:
cvat/apps/dataset_manager/bindings.py- UI Actions:
cvat-ui/src/actions/tasks-actions.ts- Core Session:
cvat-core/src/session.ts,cvat-core/src/session-implementation.ts- Server Response Types:
cvat-core/src/server-response-types.ts- SDK Proxies:
cvat-sdk/cvat_sdk/core/proxies/tasks.pyGiven the extensive integration across different modules, it appears that
consensus_job_per_segmentis properly integrated and used where necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `consensus_job_per_segment` is properly integrated and used where necessary. # Test: Search for the usage of `consensus_job_per_segment`. Expect: Proper integration and usage in all relevant places. rg --type python $'consensus_job_per_segment'Length of output: 75
Script:
#!/bin/bash # Description: Verify that `consensus_job_per_segment` is properly integrated and used where necessary. # Test: Search for the usage of `consensus_job_per_segment`. Expect: Proper integration and usage in all relevant places. rg 'consensus_job_per_segment'Length of output: 4002
cvat/apps/engine/serializers.py (6)
596-596: Addition ofsource_job_idfield inJobReadSerializer.This field addition aligns with the PR's objective to support job sourcing functionalities.
968-971: Addition ofconsensus_job_per_segmentfield inDataSerializer.This field supports the new functionality for consensus jobs per segment, which is crucial for the task segmentation feature.
1104-1104: Addition ofconsensus_job_per_segmentfield inTaskReadSerializer.Proper integration of the new field to support consensus job configurations at the task level.
1129-1129: Addition ofconsensus_job_per_segmentfield inTaskWriteSerializer.This addition allows setting the consensus job per segment during task creation or update, which is essential for the new functionality.
1195-1195: Handling ofconsensus_job_per_segmentin the update method ofTaskWriteSerializer.Proper handling of the new field during the update operations, ensuring that the consensus job settings are preserved or updated correctly.
Line range hint
1456-1470: Addition ofconsensus_job_per_segmentfield inDataMetaReadSerializer.The field is correctly added to support reading consensus job configurations, which is necessary for proper data management and operations.
…ouldn't be updated once Task is created
|
In the above commit itself, I have added the constraint on the value of consensus jobs, [0, 10] - {1}. |
…ta model to Task model
…core_threshold which are set through extra configuration when creating task
…can be set through API call also when creating task initially
…multiple annotator mode
List of features added in this PR
|
|
On going through its detailed analysis, I also don't seem to find any redundant code block that can be removed. |
cvat/apps/engine/migrations/0079_job_source_job_id_task_consensus_job_per_segment.py
Outdated
Show resolved
Hide resolved
| @@ -677,6 +679,7 @@ class Job(TimestampedModel): | |||
|
|
|||
| type = models.CharField(max_length=32, choices=JobType.choices(), | |||
There was a problem hiding this comment.
Please add a new job type in the enum.
There was a problem hiding this comment.
But then we would also have to check that that there isn't any logic based on job type due to which the consensus job doesn't work properly.
Like some functionalities which it exhibited since it was inheriting the job type, would now not work, and break?
There was a problem hiding this comment.
Despite that, shouldn't the job type structure look like this?
- Job Types
- Ground Truth
- Annotation
- Normal Job
- Consensus Job
And inevitably the information about whether it's a normal or consensus job is present in parameter parent_job_id...?
There was a problem hiding this comment.
Despite that, shouldn't the job type structure look like this? [...]
I don't see a point in making such a nested structure at this point. The consensus jobs are not the same as annotation jobs, they have different handling logic in annotation import and export. Probably, they should not be included in the task quality report. It's better to just keep the type list flat, unless there is a clear use case for such a structure.
But then we would also have to check that that there isn't any logic based on job type due to which the consensus job doesn't work properly.
Like some functionalities which it exhibited since it was inheriting the job type, would now not work, and break?
You would need to check this in any case, as these jobs aren't the same as "normal" jobs.
…ving them from TaskData to Task
… while moving them from TaskData to Task
consensus_job_per_segment and agreement_score_threshold into one migration file
…editing the agreement_score_threshold
Update develop after v2.14.4
…onsensus_job_per_segment`
…rom task data in cvat-sdk
| @@ -92,7 +92,7 @@ function validateURL(_: RuleObject, value: string): Promise<void> { | |||
| return Promise.resolve(); | |||
| } | |||
|
|
|||
| const isInteger = ({ min, max }: { min?: number; max?: number }) => ( | |||
| const isInteger = ({ min, max, toBeSkipped }: { min?: number; max?: number; toBeSkipped?: number }) => ( | |||
There was a problem hiding this comment.
Consider renaming the isInteger function or adding another validator that checks for specific values. toBeSkipped can be renamed to excluded for better readability.
There was a problem hiding this comment.
With this, the functionality is added with minimal modifications, and the function gets reused in consensus-configuration. The existing function also checks for the range along with whether it's an integer, as the function's name suggests...
There was a problem hiding this comment.
This is clear, but now the function does something that is not explicitly reflected in its name. It can be confusing and unexpected to other people editing this code.
| @@ -1119,12 +1122,13 @@ class TaskWriteSerializer(WriteOnceMixin, serializers.ModelSerializer): | |||
| project_id = serializers.IntegerField(required=False, allow_null=True) | |||
| target_storage = StorageSerializer(required=False, allow_null=True) | |||
| source_storage = StorageSerializer(required=False, allow_null=True) | |||
| consensus_jobs_per_segment = serializers.IntegerField(required=False) | |||
There was a problem hiding this comment.
Typically, it's nice to set the default value to 0 or none for such optional parameters. You can see how we use allow_null for other parameters here. This simplifies calling of such APIs in various SDKs and API clients.
| @@ -1891,6 +1891,11 @@ paths: | |||
| enum: | |||
There was a problem hiding this comment.
Probably, this file needs to be updated to reflect the latest changes. I can see some added parameters are missing.
| const { task: taskInstance, onUpdateTask } = this.props; | ||
| const taskName = name + (consensusJobsPerSegment > 0 ? ' (Consensus Based Annotation)' : ''); |
There was a problem hiding this comment.
I suggest that the task name is not modified, and an icon or a tag is shown instead. Take for example the "Ground Truth" label on GT jobs.
Fixes Creating
consensus jobsfornormal jobsin #7973List of features present in this PR
Checklist
developbranch- [ ] I have created a changelog fragment- [ ] I have updated the documentation accordingly- [ ] I have added tests to cover my changes- [ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
consensus job per segmentconfiguration option for tasks.source job IDfor jobs.Enhancements
consensus job per segment.Bug Fixes
Migration
consensus job per segmentandsource job ID.