Consensus task creation #8939
Consensus task creation #8939
Conversation
zhiltsov-max
requested review from
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 WalkthroughThis pull request introduces a comprehensive feature for creating tasks with consensus jobs. The changes span multiple components of the CVAT system, including core, UI, backend, and database layers. The new functionality allows users to specify the number of consensus replicas when creating tasks, enabling more robust annotation workflows. The implementation adds support for consensus job types, introduces new fields for tracking parent-child job relationships, and updates various components to handle these new capabilities seamlessly across the application. Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 4
🧹 Nitpick comments (11)
cvat-ui/src/utils/validate-integer.ts (1)
24-34: Enhance error messages with value contextConsider including the actual value in error messages to help users understand why validation failed.
- return Promise.reject(new Error(`Value must be more than ${min}`)); + return Promise.reject(new Error(`Value ${intValue} must be more than ${min}`)); - return Promise.reject(new Error(`Value must be less than ${max}`)); + return Promise.reject(new Error(`Value ${intValue} must be less than ${max}`)); - return Promise.reject(new Error(`Value can not be equal to ${intValue}`)); + return Promise.reject(new Error(`Value ${intValue} is not allowed`));cvat/apps/engine/migrations/0087_job_parent_job_task_consensus_replicas_and_more.py (1)
27-30: Consider adding constraints to consensus_replicasThe field allows any integer value including negative numbers. Consider adding a positive integer validator or minimum value constraint.
migrations.AddField( model_name="task", name="consensus_replicas", - field=models.IntegerField(default=0), + field=models.PositiveIntegerField(default=0), ),cvat/apps/engine/default_settings.py (1)
95-97: Add docstring for MAX_CONSENSUS_REPLICAS settingAdd documentation explaining the purpose and impact of this setting, similar to other settings in the file.
MAX_CONSENSUS_REPLICAS = int(os.getenv("CVAT_MAX_CONSENSUS_REPLICAS", 11)) +""" +Maximum number of consensus replicas allowed per task. +This setting limits the number of independent annotations that can be created +for consensus verification on a single task. +""" if MAX_CONSENSUS_REPLICAS < 1: raise ImproperlyConfigured(f"MAX_CONSENSUS_REPLICAS must be >= 1, got {MAX_CONSENSUS_REPLICAS}")cvat-ui/src/components/jobs-page/job-card.tsx (1)
55-60: Use exhaustive type checking for job typesConsider using a switch statement with exhaustive type checking to ensure all JobType values are handled.
- let tag = null; - if (job.type === JobType.GROUND_TRUTH) { - tag = 'Ground truth'; - } else if (job.type === JobType.CONSENSUS) { - tag = 'Consensus'; - } + const getJobTypeTag = (type: JobType): string | null => { + switch (type) { + case JobType.GROUND_TRUTH: + return 'Ground truth'; + case JobType.CONSENSUS: + return 'Consensus'; + case JobType.ANNOTATION: + return null; + default: + const _exhaustiveCheck: never = type; + return _exhaustiveCheck; + } + }; + const tag = getJobTypeTag(job.type);cvat-ui/src/components/jobs-page/jobs-filter-configuration.ts (1)
99-111: Consider enhancing the job type filter configuration.The implementation could be improved by:
- Using the JobType enum values to maintain consistency
- Adding descriptions to help users understand each job type
+import { JobType } from 'cvat-core-wrapper'; type: { label: 'Job Type', type: 'select', operators: ['select_equals'], valueSources: ['value'], fieldSettings: { listValues: [ - { value: 'annotation', title: 'annotation' }, - { value: 'ground_truth', title: 'ground_truth' }, - { value: 'consensus', title: 'consensus' }, + { value: JobType.ANNOTATION, title: 'Annotation', description: 'Regular annotation job' }, + { value: JobType.GROUND_TRUTH, title: 'Ground Truth', description: 'Reference annotations for quality control' }, + { value: JobType.CONSENSUS, title: 'Consensus', description: 'Job requiring multiple annotators to reach consensus' }, ], }, },cvat-ui/src/reducers/jobs-reducer.ts (1)
24-24: Add TypeScript interface for regularJobViewUncollapse.Define the type for better type safety and documentation:
+interface RegularJobViewState { + [key: number]: boolean; +} const defaultState: JobsState = { - regularJobViewUncollapse: {}, + regularJobViewUncollapse: {} as RegularJobViewState,cvat-ui/src/actions/jobs-actions.ts (1)
168-174: Improve parameter naming and simplify the thunk implementation.The parameter name 'uncollapse' is confusing as it's used in the opposite way than expected.
-export const collapseRegularJob = (jobID: number, uncollapse: boolean): ThunkAction => async (dispatch) => { - if (uncollapse) { - dispatch(jobsActions.collapseRegularJob(jobID)); - } else { - dispatch(jobsActions.uncollapseRegularJob(jobID)); - } +export const setJobCollapsed = (jobID: number, collapsed: boolean): ThunkAction => async (dispatch) => { + dispatch(collapsed ? jobsActions.collapseRegularJob(jobID) : jobsActions.uncollapseRegularJob(jobID)); };cvat-ui/src/components/task-page/details.tsx (1)
92-114: Consider using flex layout for better alignment.The task name rendering looks good, but consider using flex layout for better alignment with other elements.
- <Row> - <Col> + <Row align="middle"> + <Col flex="auto">cvat-core/src/server-response-types.ts (2)
124-124: Add JSDoc comment for the consensus property.Consider adding documentation to explain the purpose and implications of enabling consensus.
+ /** Indicates whether consensus validation is enabled for this task */ consensus_enabled: boolean;
151-152: Add JSDoc comments and consider type refinement.Consider:
- Adding documentation for these properties
- Using a more specific type for consensus_replicas to ensure positive values
+ /** ID of the parent job for consensus validation */ parent_job_id: number | null; + /** Number of replicas required for consensus validation (must be positive) */ - consensus_replicas: number; + consensus_replicas: number & { __brand: 'PositiveInteger' };changelog.d/20250113_203431_mzhiltso_consensus_tasks.md (1)
1-4: Consider adding more details to the changelog entryWhile the entry correctly describes the main feature, it would be helpful to add more details about:
- What consensus jobs are and their purpose
- Key capabilities or limitations of the feature
- Any notable configuration options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
changelog.d/20250113_203431_mzhiltso_consensus_tasks.md(1 hunks)cvat-core/src/enums.ts(1 hunks)cvat-core/src/server-response-types.ts(2 hunks)cvat-core/src/session-implementation.ts(1 hunks)cvat-core/src/session.ts(9 hunks)cvat-ui/src/actions/jobs-actions.ts(3 hunks)cvat-ui/src/actions/tasks-actions.ts(2 hunks)cvat-ui/src/components/create-task-page/advanced-configuration-form.tsx(5 hunks)cvat-ui/src/components/create-task-page/create-task-content.tsx(1 hunks)cvat-ui/src/components/job-item/job-item.tsx(7 hunks)cvat-ui/src/components/job-item/styles.scss(1 hunks)cvat-ui/src/components/jobs-page/job-card.tsx(4 hunks)cvat-ui/src/components/jobs-page/jobs-filter-configuration.ts(1 hunks)cvat-ui/src/components/jobs-page/styles.scss(2 hunks)cvat-ui/src/components/task-page/details.tsx(5 hunks)cvat-ui/src/components/task-page/job-list.tsx(1 hunks)cvat-ui/src/reducers/index.ts(1 hunks)cvat-ui/src/reducers/jobs-reducer.ts(2 hunks)cvat-ui/src/utils/validate-integer.ts(1 hunks)cvat/apps/dataset_manager/bindings.py(4 hunks)cvat/apps/dataset_manager/task.py(1 hunks)cvat/apps/engine/backup.py(5 hunks)cvat/apps/engine/default_settings.py(1 hunks)cvat/apps/engine/migrations/0087_job_parent_job_task_consensus_replicas_and_more.py(1 hunks)cvat/apps/engine/models.py(4 hunks)cvat/apps/engine/serializers.py(6 hunks)cvat/apps/engine/task.py(1 hunks)cvat/apps/engine/views.py(1 hunks)cvat/schema.yml(8 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- cvat-ui/src/components/job-item/job-item.tsx
- cvat-ui/src/components/create-task-page/advanced-configuration-form.tsx
- cvat-ui/src/reducers/index.ts
- cvat-ui/src/components/create-task-page/create-task-content.tsx
🧰 Additional context used
🪛 GitHub Actions: isort
cvat/apps/engine/migrations/0087_job_parent_job_task_consensus_replicas_and_more.py
[error] 1-8: Imports are incorrectly sorted and/or formatted. The import statements need to be reordered according to isort rules.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: e2e_testing (features)
- GitHub Check: e2e_testing (issues_prs2)
- GitHub Check: e2e_testing (issues_prs)
- GitHub Check: e2e_testing (canvas3d_functionality_2)
- GitHub Check: e2e_testing (canvas3d_functionality)
- GitHub Check: e2e_testing (actions_projects_models)
- GitHub Check: e2e_testing (actions_users)
- GitHub Check: e2e_testing (actions_objects2)
- GitHub Check: e2e_testing (actions_objects)
- GitHub Check: e2e_testing (actions_tasks3)
- GitHub Check: rest_api_testing
- GitHub Check: e2e_testing (actions_tasks2)
- GitHub Check: unit_testing
- GitHub Check: e2e_testing (actions_tasks)
- GitHub Check: testing
🔇 Additional comments (36)
cvat-core/src/enums.ts (1)
40-40: LGTM!The addition of the CONSENSUS job type follows the existing enum pattern and maintains consistency with the rest of the codebase.
cvat-ui/src/actions/jobs-actions.ts (1)
30-31: LGTM!The action type names are clear and follow the existing naming pattern.
cvat-ui/src/components/task-page/job-list.tsx (2)
62-63: LGTM! Clear and descriptive comment.The comment effectively explains the purpose of the filtering logic.
63-63: Verify handling of job list updates.The filtering logic looks correct. However, we should ensure the job list updates properly when consensus-related changes occur.
✅ Verification successful
Consensus job updates are properly handled
The job list implementation correctly manages consensus-related changes through:
- Proper filtering of parent/child relationships
- Redux-managed state updates
- Hierarchical display in the UI with collapse/expand functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any event handlers or Redux actions for consensus status changes rg -A 5 "consensus.*status.*change|UPDATE_JOB_CONSENSUS"Length of output: 58
Script:
#!/bin/bash # Search for job-related Redux actions and state updates rg -g "*.ts*" "job.*Action|updateJob|setJob" -A 3 # Search for consensus-related code with broader terms rg -g "*.ts*" "consensus.*job|jobInstance.*consensus" -A 3 # Look for job list component updates ast-grep --pattern 'useEffect($$$, [$$$job$$$])'Length of output: 10972
cvat-ui/src/components/task-page/details.tsx (2)
63-63: LGTM! State initialization looks good.The consensusEnabled state is properly initialized from the task instance.
Also applies to: 75-75
145-145: LGTM! Clear consensus status indication.The consensus tag is appropriately placed and styled with a distinctive blue color.
cvat-ui/src/actions/tasks-actions.ts (1)
225-227: LGTM! Storage configuration looks good.The storage configuration is properly handled with appropriate default values.
cvat/apps/engine/models.py (4)
171-171: LGTM: JobType enum extended with CONSENSUSThe JobType enum is appropriately extended to support consensus jobs.
578-579: LGTM: Task model extended with consensus_replicas fieldThe consensus_replicas field is properly added with a default value of 0, and includes a descriptive docstring.
866-866: LGTM: Job model extended with parent_job fieldThe parent_job foreign key is correctly added with appropriate null/blank settings and a descriptive related_name.
Line range hint
881-883: Verify job type handling in _import_gt_jobsThe job type handling is updated to support consensus jobs, but we should verify the behavior for unknown job types.
✅ Verification successful
Job type handling is properly validated
The codebase validates job types through models.JobType enum and raises ValidationError for unknown types. No unhandled job types were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any unexpected job types in the codebase rg -A 3 "job.*type.*=.*['\"]" | grep -v "ANNOTATION\|GROUND_TRUTH\|CONSENSUS"Length of output: 77936
cvat/apps/engine/backup.py (2)
328-328: LGTM: Improved job retrieval in _get_db_jobsThe method now correctly retrieves all jobs for each segment instead of just the first one, which is necessary for consensus jobs.
441-463: Verify segment serialization logicThe segment serialization has been updated to handle multiple jobs, but we should verify that all job types are properly serialized.
Consider adding type hints to improve code clarity:
def serialize_segment(db_segment: models.Segment) -> list[dict]:✅ Verification successful
Segment serialization logic is properly implemented
The code correctly handles all possible segment types (RANGE, SPECIFIC_FRAMES) and validation modes (GT, GT_POOL) through appropriate serializers. The special cases for segment_size == 0 with RANGE type and GT_POOL validation mode are also properly handled.
Consider adding type hints as suggested:
def serialize_segment(db_segment: models.Segment) -> list[dict]:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any job types that might be missed in serialization rq -A 3 "serialize_segment.*db_segment" | grep "type" | grep -v "ANNOTATION\|GROUND_TRUTH\|CONSENSUS"Length of output: 194
Script:
#!/bin/bash # Find SegmentType and ValidationMode definitions echo "=== Searching for SegmentType definition ===" rg -A 5 "class SegmentType" echo -e "\n=== Searching for ValidationMode definition ===" rg -A 5 "class ValidationMode" echo -e "\n=== Searching for segment serialization patterns ===" rg -A 3 "serialize_segment"Length of output: 2040
cvat/apps/dataset_manager/task.py (1)
817-817: Verify TODO comment about consensus jobsThe TODO comment suggests that consensus jobs might need to be included in task exports in the future.
Would you like me to create an issue to track this TODO item for future implementation?
✅ Verification successful
Consensus job exclusion from task exports is intentional and correct
The current implementation correctly excludes consensus jobs from task exports to avoid data duplication, as these jobs share the same data segments with their parent annotation jobs. The TODO comment suggests reconsidering this design choice, but the current approach is well-reasoned and maintains data consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing task export implementations that might need updating rg -A 3 "export.*task" | grep -v "test"Length of output: 29762
Script:
#!/bin/bash # Check how consensus jobs are handled in the codebase echo "=== Checking consensus job handling ===" rg -B 3 -A 3 "consensus.*job" --glob '!*.{min.js,map,lock}' echo -e "\n=== Checking job filtering in task exports ===" rg -B 3 -A 3 "_get_db_jobs" --glob '!*.{min.js,map,lock}'Length of output: 7472
cvat-core/src/session.ts (4)
Line range hint
497-509: LGTM: Job class extended with consensus fieldsThe Job class is properly extended with parent_job_id and consensus_replicas fields.
537-538: LGTM: Job constructor initializationThe constructor correctly initializes the new consensus-related fields with appropriate defaults.
Also applies to: 553-554
906-907: LGTM: Job initialization from dataThe job initialization from data properly handles parent_job_id and consensus_replicas.
1015-1017: LGTM: Task class extended with consensusEnabledThe Task class is properly extended with the consensusEnabled property.
cvat-core/src/session-implementation.ts (1)
751-754: LGTM! Clean addition of consensus replicas supportThe conditional addition of consensus_replicas to taskSpec is implemented correctly.
cvat/apps/dataset_manager/bindings.py (2)
2181-2182: LGTM! Good refactoring of source validationThe introduction of a centralized sources set improves code maintainability and makes it easier to add new source types in the future.
2263-2263: LGTM! Consistent usage of the sources setThe source validation is correctly updated to use the new sources set across all relevant code paths.
Also applies to: 2277-2277, 2349-2349
cvat/apps/engine/serializers.py (4)
640-641: LGTM! Clear addition of consensus-related fieldsThe parent_job_id and consensus_replicas fields are added with appropriate read-only flags.
2237-2239: LGTM! Well-defined consensus_enabled fieldThe consensus_enabled field is added with proper source method reference.
2272-2302: LGTM! Thorough validation of consensus_replicasThe validation logic for consensus_replicas ensures:
- Value is either 0 or greater than 1
- Value doesn't exceed the maximum limit
- Default value is 0 when not specified
2256-2263: LGTM! Proper implementation of consensus_enabledThe consensus_enabled getter and representation override are implemented correctly to reflect the consensus state.
cvat/apps/engine/task.py (1)
227-232: LGTM! Efficient consensus job creationThe implementation:
- Correctly creates consensus jobs for each replica
- Efficiently reuses the same db_segment to avoid data duplication
- Includes clear comments explaining the design decision
cvat/apps/engine/views.py (1)
1952-1952: LGTM: Filter field addition for consensus jobsThe addition of
parent_job_idto the filter fields is appropriate for supporting the consensus task feature, allowing jobs to be filtered based on their parent-child relationships.cvat-ui/src/components/job-item/styles.scss (1)
74-87: LGTM: Well-structured styling additionsThe new styles for consensus jobs follow the existing patterns and maintain consistency with the codebase:
- Proper use of SCSS nesting
- Consistent use of spacing variables
- Clear class naming conventions
cvat-ui/src/components/jobs-page/styles.scss (1)
78-81: LGTM: Well-implemented job type indicator stylesThe new job type indicator styles are well-implemented:
- Consistent with existing hover patterns
- Smooth transitions for better UX
- Proper use of variables and spacing
Also applies to: 142-155
cvat/schema.yml (7)
1807-1807: LGTM! Filter fields updated for consensus supportThe addition of 'parent_job_id' to the available filter fields is well-integrated and enables proper filtering of consensus jobs.
1832-1836: LGTM! Parent job ID parameter properly definedThe parent_job_id query parameter is well-defined with appropriate type and description for filtering jobs.
1859-1860: LGTM! Sorting capabilities enhancedThe addition of 'parent_job_id' to the sorting fields enables proper organization of consensus jobs in the API responses.
8345-8351: LGTM! JobRead schema properly extendedThe JobRead schema has been correctly extended with:
- parent_job_id (nullable, read-only)
- consensus_replicas (read-only)
The properties are well-typed and appropriately marked as read-only.
8379-8384: LGTM! Job type enum updatedThe JobType enum has been properly extended with the 'consensus' type and includes appropriate documentation.
10966-10968: LGTM! TaskRead schema updatedThe consensus_enabled flag has been properly added as a read-only boolean property.
11061-11068: LGTM! TaskWriteRequest schema properly extendedThe consensus_replicas field is well-defined with:
- Appropriate type (integer)
- Proper validation (minimum: 0)
- Nullable with default value
- Clear documentation explaining its purpose
| if (Number.isNaN(intValue) || !Number.isInteger(intValue)) { | ||
| return Promise.reject(new Error('Value must be a positive integer')); | ||
| } |
There was a problem hiding this comment.
Fix misleading error message
The error message states "must be a positive integer" but the function doesn't actually enforce positivity. The value can be negative unless restricted by the min parameter.
- return Promise.reject(new Error('Value must be a positive integer'));
+ return Promise.reject(new Error('Value must be an integer'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Number.isNaN(intValue) || !Number.isInteger(intValue)) { | |
| return Promise.reject(new Error('Value must be a positive integer')); | |
| } | |
| if (Number.isNaN(intValue) || !Number.isInteger(intValue)) { | |
| return Promise.reject(new Error('Value must be an integer')); | |
| } |
cvat/apps/engine/migrations/0087_job_parent_job_task_consensus_replicas_and_more.py
Outdated
Show resolved
Hide resolved
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8939 +/- ##
===========================================
- Coverage 73.88% 73.86% -0.02%
===========================================
Files 416 417 +1
Lines 44413 44495 +82
Branches 3993 4006 +13
===========================================
+ Hits 32814 32868 +54
- Misses 11599 11627 +28
|
|
Probably need to extract common styles for Consensus and GT job tags. |
Motivation and context
A part of #8434
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes: Consensus Job Support
New Features
Improvements
User Interface