Skip to content
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

[WIP] Mk/validation api small fixes #8816

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cvat/apps/engine/backup.py
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ def serialize_data():
data['custom_segments'] = True

if (
(validation_layout := getattr(self._db_data, 'validation_layout', None)) and
(validation_layout := self._db_data.nullable_validation_layout) and
validation_layout.mode == models.ValidationMode.GT_POOL
):
validation_params_serializer = ValidationParamsSerializer({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 4.2.15 on 2024-12-11 17:13

import cvat.apps.engine.models
from django.db import migrations
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
("engine", "0086_profile_has_analytics_access"),
]

operations = [
migrations.AlterField(
model_name="validationlayout",
name="task_data",
field=cvat.apps.engine.models.NullableOneToOneField(
on_delete=django.db.models.deletion.CASCADE,
related_name="validation_layout",
to="engine.data",
),
),
]
28 changes: 26 additions & 2 deletions cvat/apps/engine/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from django.db import IntegrityError, models, transaction
from django.db.models import Q, TextChoices
from django.db.models.fields import FloatField
from django.db.models.fields.reverse_related import OneToOneRel
from django.utils.translation import gettext_lazy as _
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import extend_schema_field
Expand Down Expand Up @@ -259,10 +260,32 @@ class ValidationFrame(models.Model):
)
path = models.CharField(max_length=1024, default='')


class NullableOneToOneField(models.OneToOneField):
NULLABLE_PREFIX = "nullable"

@classmethod
def _make_nullable_property(cls, property_name: str):
return f"{cls.NULLABLE_PREFIX}_{property_name}"

def contribute_to_related_class(self, cls: models.Model, related: OneToOneRel):
super().contribute_to_related_class(cls, related)

property_name = related.get_accessor_name()
nullable_property_name = self._make_nullable_property(property_name)

if not hasattr(cls, nullable_property_name):
def _fget(instance: models.Model):
return getattr(instance, property_name, None)

# todo: cls._meta.concrete_model?
setattr(cls, nullable_property_name, property(_fget))


class ValidationLayout(models.Model):
"Represents validation configuration in a task"

task_data = models.OneToOneField(
task_data = NullableOneToOneField(
'Data', on_delete=models.CASCADE, related_name="validation_layout"
)

Expand Down Expand Up @@ -299,6 +322,7 @@ class Data(models.Model):
video: MaybeUndefined[Video]
related_files: models.manager.RelatedManager[RelatedFile]
validation_layout: MaybeUndefined[ValidationLayout]
nullable_validation_layout: ValidationLayout | None

client_files: models.manager.RelatedManager[ClientFile]
server_files: models.manager.RelatedManager[ServerFile]
Expand Down Expand Up @@ -388,7 +412,7 @@ def update_validation_layout(

@property
def validation_mode(self) -> Optional[ValidationMode]:
return getattr(getattr(self, 'validation_layout', None), 'mode', None)
return getattr(self.nullable_validation_layout, 'mode', None)


class Video(models.Model):
Expand Down
27 changes: 17 additions & 10 deletions cvat/apps/engine/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -992,10 +992,15 @@ def update(self, instance: models.Job, validated_data: dict[str, Any]) -> models
db_task = db_segment.task
db_data = db_task.data

if not (
hasattr(db_job.segment.task.data, 'validation_layout') and
db_job.segment.task.data.validation_layout.mode == models.ValidationMode.GT_POOL
):
db_task_validation_layout = db_data.nullable_validation_layout

if not db_task_validation_layout:
raise serializers.ValidationError(
"Task has no validation layout configured. "
"Validation must be initialized during a task creation"
)

if db_task_validation_layout.mode != models.ValidationMode.GT_POOL:
raise serializers.ValidationError(
"Honeypots can only be modified if the task "
f"validation mode is '{models.ValidationMode.GT_POOL}'"
Expand Down Expand Up @@ -1258,7 +1263,7 @@ class JobValidationLayoutReadSerializer(serializers.Serializer):
)

def to_representation(self, instance: models.Job):
validation_layout = getattr(instance.segment.task.data, 'validation_layout', None)
validation_layout = instance.segment.task.data.nullable_validation_layout
if not validation_layout:
return {}

Expand Down Expand Up @@ -1336,11 +1341,13 @@ def validate(self, attrs):

@transaction.atomic
def update(self, instance: models.Task, validated_data: dict[str, Any]) -> models.Task:
validation_layout: models.ValidationLayout | None = (
getattr(instance.data, 'validation_layout', None)
)
validation_layout: models.ValidationLayout | None = instance.data.nullable_validation_layout

if not validation_layout:
raise serializers.ValidationError("Validation is not configured in the task")
raise serializers.ValidationError(
"Task has no validation layout configured. "
"Validation must be initialized during a task creation"
)

if 'disabled_frames' in validated_data:
requested_disabled_frames = validated_data['disabled_frames']
Expand Down Expand Up @@ -2427,7 +2434,7 @@ def update(self, instance: models.Data, validated_data: dict[str, Any]) -> model
)
)

validation_layout = getattr(instance, 'validation_layout', None)
validation_layout = instance.nullable_validation_layout
if validation_layout and validation_layout.mode == models.ValidationMode.GT_POOL:
gt_frame_set = set(validation_layout.frames)
changed_deleted_frames = requested_deleted_frames_set.difference(instance.deleted_frames)
Expand Down
2 changes: 1 addition & 1 deletion cvat/apps/engine/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ def _to_rel_frame(abs_frame: int) -> int:
))

# TODO: refactor
if hasattr(db_data, 'validation_layout'):
if db_data.nullable_validation_layout:
if db_data.validation_layout.mode == models.ValidationMode.GT:
def _to_abs_frame(rel_frame: int) -> int:
return rel_frame * db_data.get_frame_step() + db_data.start_frame
Expand Down
36 changes: 19 additions & 17 deletions cvat/apps/engine/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1746,13 +1746,15 @@ def preview(self, request, pk):
summary="Allows getting current validation configuration",
responses={
'200': OpenApiResponse(TaskValidationLayoutReadSerializer),
'204': OpenApiResponse(description="No validation layout is associated with a task"),
})
@extend_schema(
methods=["PATCH"],
summary="Allows updating current validation configuration",
request=TaskValidationLayoutWriteSerializer,
responses={
'200': OpenApiResponse(TaskValidationLayoutReadSerializer),
'400': OpenApiResponse(description="Bad request"),
},
examples=[
OpenApiExample("set honeypots to random validation frames", {
Expand All @@ -1774,22 +1776,17 @@ def preview(self, request, pk):
def validation_layout(self, request, pk):
db_task = self.get_object() # call check_object_permissions as well

validation_layout = getattr(db_task.data, 'validation_layout', None)

if request.method == "PATCH":
if not validation_layout:
return ValidationError(
"Task has no validation setup configured. "
"Validation must be initialized during task creation"
)

# TODO: probably it should take validation_layout instead of task
request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data)
request_serializer.is_valid(raise_exception=True)
validation_layout = request_serializer.save().data.validation_layout
updated_task = request_serializer.save()
validation_layout = updated_task.data.validation_layout
else:
Comment on lines +1780 to +1785
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address the TODO comment and pass validation_layout to the serializer

The TODO comment suggests that the serializer should take validation_layout instead of db_task. Passing the correct instance ensures proper serialization and updates. Please update the code to pass validation_layout as the instance. If validation_layout is None, handle its creation accordingly.

Apply this diff to fix the issue:

if request.method == "PATCH":
-    # TODO: probably it should take validation_layout instead of task
-    request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data)
+    validation_layout = db_task.data.validation_layout
+    if validation_layout is None:
+        validation_layout = ValidationLayout(data=db_task.data)
+    request_serializer = TaskValidationLayoutWriteSerializer(validation_layout, data=request.data)
    request_serializer.is_valid(raise_exception=True)
-    updated_task = request_serializer.save()
-    validation_layout = updated_task.data.validation_layout
+    validation_layout = request_serializer.save()
else:
    validation_layout = db_task.data.validation_layout
📝 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.

Suggested change
# TODO: probably it should take validation_layout instead of task
request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data)
request_serializer.is_valid(raise_exception=True)
validation_layout = request_serializer.save().data.validation_layout
updated_task = request_serializer.save()
validation_layout = updated_task.data.validation_layout
else:
validation_layout = db_task.data.validation_layout
if validation_layout is None:
validation_layout = ValidationLayout(data=db_task.data)
request_serializer = TaskValidationLayoutWriteSerializer(validation_layout, data=request.data)
request_serializer.is_valid(raise_exception=True)
validation_layout = request_serializer.save()
else:

validation_layout = db_task.data.nullable_validation_layout

if not validation_layout:
response_serializer = TaskValidationLayoutReadSerializer(SimpleNamespace(mode=None))
return Response(response_serializer.data, status=status.HTTP_200_OK)
if validation_layout is None:
return Response(status=status.HTTP_204_NO_CONTENT)

response_serializer = TaskValidationLayoutReadSerializer(validation_layout)
return Response(response_serializer.data, status=status.HTTP_200_OK)
Expand Down Expand Up @@ -1920,13 +1917,11 @@ def perform_create(self, serializer):
serializer.instance = self.get_queryset().get(pk=serializer.instance.pk)

@transaction.atomic
def perform_destroy(self, instance):
def perform_destroy(self, instance: Job):
if instance.type != JobType.GROUND_TRUTH:
raise ValidationError("Only ground truth jobs can be removed")

validation_layout: Optional[models.ValidationLayout] = getattr(
instance.segment.task.data, 'validation_layout', None
)
validation_layout: Optional[models.ValidationLayout] = instance.segment.task.data.nullable_validation_layout
if (validation_layout and validation_layout.mode == models.ValidationMode.GT_POOL):
raise ValidationError(
'GT jobs cannot be removed when task validation mode is "{}"'.format(
Expand Down Expand Up @@ -2352,13 +2347,15 @@ def preview(self, request, pk):
summary="Allows getting current validation configuration",
responses={
'200': OpenApiResponse(JobValidationLayoutReadSerializer),
'204': OpenApiResponse(description="No validation layout is associated with a job"),
})
@extend_schema(
methods=["PATCH"],
summary="Allows updating current validation configuration",
request=JobValidationLayoutWriteSerializer,
responses={
'200': OpenApiResponse(JobValidationLayoutReadSerializer),
'400': OpenApiResponse(description="Bad request"),
},
examples=[
OpenApiExample("set honeypots to random validation frames", {
Expand Down Expand Up @@ -2394,7 +2391,12 @@ def validation_layout(self, request, pk):
db_job = request_serializer.save()

response_serializer = JobValidationLayoutReadSerializer(db_job)
return Response(response_serializer.data, status=status.HTTP_200_OK)
content = response_serializer.data

if content:
return Response(response_serializer.data, status=status.HTTP_200_OK)

return Response(status=status.HTTP_204_NO_CONTENT)

@extend_schema(tags=['issues'])
@extend_schema_view(
Expand Down
8 changes: 8 additions & 0 deletions cvat/schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2664,6 +2664,8 @@ paths:
schema:
$ref: '#/components/schemas/JobValidationLayoutRead'
description: ''
'204':
description: No validation layout is associated with a job
patch:
operationId: jobs_partial_update_validation_layout
summary: Allows updating current validation configuration
Expand Down Expand Up @@ -2707,6 +2709,8 @@ paths:
schema:
$ref: '#/components/schemas/JobValidationLayoutRead'
description: ''
'400':
description: Bad request
/api/labels:
get:
operationId: labels_list
Expand Down Expand Up @@ -6147,6 +6151,8 @@ paths:
schema:
$ref: '#/components/schemas/TaskValidationLayoutRead'
description: ''
'204':
description: No validation layout is associated with a task
patch:
operationId: tasks_partial_update_validation_layout
summary: Allows updating current validation configuration
Expand Down Expand Up @@ -6201,6 +6207,8 @@ paths:
schema:
$ref: '#/components/schemas/TaskValidationLayoutRead'
description: ''
'400':
description: Bad request
/api/tasks/backup/:
post:
operationId: tasks_create_backup
Expand Down
Loading