Skip to content

Commit

Permalink
Don't save all fields of a model when bumping the update date (#7450)
Browse files Browse the repository at this point in the history
Currently, there are a lot of places where changing one object requires
updating the `updated_date` field on other objects. Usually, the `.save`
method is used for this purpose, which results in a generic SQL `UPDATE`
query that replaces the values in all columns.

Introduce and use a specialized method that only changes the date. This
provides the following advantages:

* Prevents the date updates from overwriting updates to other fields
that may happen at the same time.

* Possibly makes the updates more efficient due to fewer columns being
updated (I have no data on this, though).

* Makes it easier to recognize the purpose of the query when analyzing
logs/live statistics.

* Makes the purpose of the code more obvious.

Additionally, update `__save_job_handler`, where we don't want to save
_just_ the date, but we still only need to save two fields.
  • Loading branch information
SpecLad authored Feb 16, 2024
1 parent 972a218 commit 9c048f2
Show file tree
Hide file tree
Showing 8 changed files with 49 additions and 66 deletions.
7 changes: 2 additions & 5 deletions cvat/apps/dataset_manager/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

from django.db import transaction
from django.db.models.query import Prefetch
from django.utils import timezone
from rest_framework.exceptions import ValidationError

from cvat.apps.engine import models, serializers
Expand Down Expand Up @@ -389,9 +388,8 @@ def _save_tags_to_db(self, tags):
self.ir_data.tags = tags

def _set_updated_date(self):
db_task = self.db_job.segment.task
db_task.updated_date = timezone.now()
db_task.save()
self.db_job.segment.task.touch()
self.db_job.touch()

def _save_to_db(self, data):
self.reset()
Expand All @@ -404,7 +402,6 @@ def _save_to_db(self, data):
def _create(self, data):
if self._save_to_db(data):
self._set_updated_date()
self.db_job.save()

def create(self, data):
self._create(data)
Expand Down
46 changes: 20 additions & 26 deletions cvat/apps/engine/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
from drf_spectacular.utils import extend_schema_field

from cvat.apps.engine.utils import parse_specific_attributes
from cvat.apps.organizations.models import Organization
from cvat.apps.events.utils import cache_deleted

class SafeCharField(models.CharField):
Expand Down Expand Up @@ -316,18 +315,26 @@ class Image(models.Model):
class Meta:
default_permissions = ()

class Project(models.Model):
class TimestampedModel(models.Model):
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)

class Meta:
abstract = True

def touch(self) -> None:
self.save(update_fields=["updated_date"])

class Project(TimestampedModel):
name = SafeCharField(max_length=256)
owner = models.ForeignKey(User, null=True, blank=True,
on_delete=models.SET_NULL, related_name="+")
assignee = models.ForeignKey(User, null=True, blank=True,
on_delete=models.SET_NULL, related_name="+")
bug_tracker = models.CharField(max_length=2000, blank=True, default="")
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
status = models.CharField(max_length=32, choices=StatusChoice.choices(),
default=StatusChoice.ANNOTATION)
organization = models.ForeignKey(Organization, null=True, default=None,
organization = models.ForeignKey('organizations.Organization', null=True, default=None,
blank=True, on_delete=models.SET_NULL, related_name="projects")
source_storage = models.ForeignKey('Storage', null=True, default=None,
blank=True, on_delete=models.SET_NULL, related_name='+')
Expand Down Expand Up @@ -383,7 +390,7 @@ def with_job_summary(self):
)
)

class Task(models.Model):
class Task(TimestampedModel):
objects = TaskQuerySet.as_manager()

project = models.ForeignKey(Project, on_delete=models.CASCADE,
Expand All @@ -396,8 +403,6 @@ class Task(models.Model):
assignee = models.ForeignKey(User, null=True, blank=True,
on_delete=models.SET_NULL, related_name="assignees")
bug_tracker = models.CharField(max_length=2000, blank=True, default="")
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
overlap = models.PositiveIntegerField(null=True)
# Zero means that there are no limits (default)
# Note that the files can be split into jobs in a custom way in this case
Expand All @@ -407,7 +412,7 @@ class Task(models.Model):
data = models.ForeignKey(Data, on_delete=models.CASCADE, null=True, related_name="tasks")
dimension = models.CharField(max_length=2, choices=DimensionType.choices(), default=DimensionType.DIM_2D)
subset = models.CharField(max_length=64, blank=True, default="")
organization = models.ForeignKey(Organization, null=True, default=None,
organization = models.ForeignKey('organizations.Organization', null=True, default=None,
blank=True, on_delete=models.SET_NULL, related_name="tasks")
source_storage = models.ForeignKey('Storage', null=True, default=None,
blank=True, on_delete=models.SET_NULL, related_name='+')
Expand Down Expand Up @@ -656,15 +661,12 @@ def _validate_constraints(self, obj: Dict[str, Any]):



class Job(models.Model):
class Job(TimestampedModel):
objects = JobQuerySet.as_manager()

segment = models.ForeignKey(Segment, on_delete=models.CASCADE)
assignee = models.ForeignKey(User, null=True, blank=True, on_delete=models.SET_NULL)

created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)

# TODO: it has to be deleted in Job, Task, Project and replaced by (stage, state)
# The stage field cannot be changed by an assignee, but state field can be. For
# now status is read only and it will be updated by (stage, state). Thus we don't
Expand Down Expand Up @@ -962,16 +964,14 @@ class Profile(models.Model):
user = models.OneToOneField(User, on_delete=models.CASCADE)
rating = models.FloatField(default=0.0)

class Issue(models.Model):
class Issue(TimestampedModel):
frame = models.PositiveIntegerField()
position = FloatArrayField()
job = models.ForeignKey(Job, related_name='issues', on_delete=models.CASCADE)
owner = models.ForeignKey(User, null=True, blank=True, related_name='+',
on_delete=models.SET_NULL)
assignee = models.ForeignKey(User, null=True, blank=True, related_name='+',
on_delete=models.SET_NULL)
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
resolved = models.BooleanField(default=False)

def get_project_id(self):
Expand All @@ -991,12 +991,10 @@ def get_job_id(self):
return self.job_id


class Comment(models.Model):
class Comment(TimestampedModel):
issue = models.ForeignKey(Issue, related_name='comments', on_delete=models.CASCADE)
owner = models.ForeignKey(User, null=True, blank=True, on_delete=models.SET_NULL)
message = models.TextField(default='')
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)

def get_project_id(self):
return self.issue.get_project_id()
Expand Down Expand Up @@ -1072,7 +1070,7 @@ def __str__(self):
def list(cls):
return [i.value for i in cls]

class CloudStorage(models.Model):
class CloudStorage(TimestampedModel):
# restrictions:
# AWS bucket name, Azure container name - 63, Google bucket name - 63 without dots and 222 with dots
# https://cloud.google.com/storage/docs/naming-buckets#requirements
Expand All @@ -1090,13 +1088,11 @@ class CloudStorage(models.Model):
display_name = models.CharField(max_length=63)
owner = models.ForeignKey(User, null=True, blank=True,
on_delete=models.SET_NULL, related_name="cloud_storages")
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
credentials = models.CharField(max_length=1024, null=True, blank=True)
credentials_type = models.CharField(max_length=29, choices=CredentialsTypeChoice.choices())#auth_type
specific_attributes = models.CharField(max_length=1024, blank=True)
description = models.TextField(blank=True)
organization = models.ForeignKey(Organization, null=True, default=None,
organization = models.ForeignKey('organizations.Organization', null=True, default=None,
blank=True, on_delete=models.SET_NULL, related_name="cloudstorages")

class Meta:
Expand Down Expand Up @@ -1130,12 +1126,10 @@ class Storage(models.Model):
class Meta:
default_permissions = ()

class AnnotationGuide(models.Model):
class AnnotationGuide(TimestampedModel):
task = models.OneToOneField(Task, null=True, blank=True, on_delete=models.CASCADE, related_name="annotation_guide")
project = models.OneToOneField(Project, null=True, blank=True, on_delete=models.CASCADE, related_name="annotation_guide")
markdown = models.TextField(blank=True, default='')
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
is_public = models.BooleanField(default=False)

@property
Expand Down
7 changes: 3 additions & 4 deletions cvat/apps/engine/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def __save_job_handler(instance, created, **kwargs):

if status != db_task.status:
db_task.status = status
db_task.save()
db_task.save(update_fields=["status", "updated_date"])

@receiver(post_save, sender=User,
dispatch_uid=__name__ + ".save_user_handler")
Expand Down Expand Up @@ -66,9 +66,8 @@ def __delete_task_handler(instance, **kwargs):
instance.data.delete()

try:
if instance.project: # update project
db_project = instance.project
db_project.save()
if db_project := instance.project: # update project
db_project.touch()
except Project.DoesNotExist:
pass # probably the project has been deleted

Expand Down
19 changes: 9 additions & 10 deletions cvat/apps/engine/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -894,9 +894,9 @@ def perform_update(self, serializer):
updated_instance = serializer.instance

if instance.project:
instance.project.save()
if updated_instance.project:
updated_instance.project.save()
instance.project.touch()
if updated_instance.project and updated_instance.project != instance.project:
updated_instance.project.touch()

@transaction.atomic
def perform_create(self, serializer, **kwargs):
Expand All @@ -905,9 +905,8 @@ def perform_create(self, serializer, **kwargs):
organization=self.request.iam_context['organization']
)

if serializer.instance.project:
db_project = serializer.instance.project
db_project.save()
if db_project := serializer.instance.project:
db_project.touch()
assert serializer.instance.organization == db_project.organization

# Required for the extra summary information added in the queryset
Expand Down Expand Up @@ -1944,9 +1943,9 @@ def metadata(self, request, pk):
db_data.deleted_frames,
))
db_data = serializer.save()
db_job.segment.task.save()
db_job.segment.task.touch()
if db_job.segment.task.project:
db_job.segment.task.project.save()
db_job.segment.task.project.touch()

if hasattr(db_data, 'video'):
media = [db_data.video]
Expand Down Expand Up @@ -2285,10 +2284,10 @@ def perform_destroy(self, instance: models.Label):
code=status.HTTP_400_BAD_REQUEST)

if project := instance.project:
project.save(update_fields=['updated_date'])
project.touch()
ProjectWriteSerializer(project).update_child_objects_on_labels_update(project)
elif task := instance.task:
task.save(update_fields=['updated_date'])
task.touch()
TaskWriteSerializer(task).update_child_objects_on_labels_update(task)

return super().perform_destroy(instance)
Expand Down
2 changes: 1 addition & 1 deletion cvat/apps/events/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from django.core.exceptions import ObjectDoesNotExist

from cvat.apps.engine.models import (
Organization,
Project,
Task,
Job,
Expand All @@ -17,6 +16,7 @@
Comment,
Label,
)
from cvat.apps.organizations.models import Organization

from .handlers import handle_update, handle_create, handle_delete
from .event import EventScopeChoice, event_scope
Expand Down
6 changes: 3 additions & 3 deletions cvat/apps/organizations/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
from django.core.exceptions import ImproperlyConfigured
from django.utils import timezone

class Organization(models.Model):
from cvat.apps.engine.models import TimestampedModel

class Organization(TimestampedModel):
slug = models.SlugField(max_length=16, blank=False, unique=True)
name = models.CharField(max_length=64, blank=True)
description = models.TextField(blank=True)
created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)
contact = models.JSONField(blank=True, default=dict)

owner = models.ForeignKey(get_user_model(), null=True,
Expand Down
12 changes: 3 additions & 9 deletions cvat/apps/webhooks/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from django.contrib.auth.models import User
from django.db import models

from cvat.apps.engine.models import Project
from cvat.apps.engine.models import Project, TimestampedModel
from cvat.apps.organizations.models import Organization


Expand All @@ -34,7 +34,7 @@ def __str__(self):
return self.value


class Webhook(models.Model):
class Webhook(TimestampedModel):
target_url = models.URLField(max_length=8192)
description = models.CharField(max_length=128, default="", blank=True)

Expand All @@ -50,9 +50,6 @@ class Webhook(models.Model):
is_active = models.BooleanField(default=True)
enable_ssl = models.BooleanField(default=True)

created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)

owner = models.ForeignKey(
User, null=True, blank=True, on_delete=models.SET_NULL, related_name="+"
)
Expand Down Expand Up @@ -82,7 +79,7 @@ class Meta:
]


class WebhookDelivery(models.Model):
class WebhookDelivery(TimestampedModel):
webhook = models.ForeignKey(
Webhook, on_delete=models.CASCADE, related_name="deliveries"
)
Expand All @@ -91,9 +88,6 @@ class WebhookDelivery(models.Model):
status_code = models.PositiveIntegerField(null=True, default=None)
redelivery = models.BooleanField(default=False)

created_date = models.DateTimeField(auto_now_add=True)
updated_date = models.DateTimeField(auto_now=True)

changed_fields = models.CharField(max_length=4096, default="")

request = models.JSONField(default=dict)
Expand Down
16 changes: 8 additions & 8 deletions cvat/schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6521,14 +6521,6 @@ components:
type: string
maxLength: 1024
default: []
provider_type:
$ref: '#/components/schemas/ProviderTypeEnum'
resource:
type: string
maxLength: 222
display_name:
type: string
maxLength: 63
created_date:
type: string
format: date-time
Expand All @@ -6537,6 +6529,14 @@ components:
type: string
format: date-time
readOnly: true
provider_type:
$ref: '#/components/schemas/ProviderTypeEnum'
resource:
type: string
maxLength: 222
display_name:
type: string
maxLength: 63
credentials_type:
$ref: '#/components/schemas/CredentialsTypeEnum'
specific_attributes:
Expand Down

0 comments on commit 9c048f2

Please sign in to comment.