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

Refactor job creation code #3465

Merged
merged 78 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
362d3c4
Refactor algorithm job creation and civ creation
amickan Aug 1, 2024
82ebb57
Remove unused debug statements
amickan Aug 1, 2024
1e595c7
Remove unused code
amickan Aug 1, 2024
724e7f3
Update post serializer to use same validation code
amickan Aug 1, 2024
2ce04b0
Move job creation to form_valid and serializer create methods
amickan Aug 1, 2024
89ad04a
Fix tests
amickan Aug 2, 2024
e5324ee
Small code improvements
amickan Aug 2, 2024
4e89d98
Add API view tests and test for test_retrieve_existing_civs
amickan Aug 2, 2024
85be1c7
Undo commenting out middleware
amickan Aug 2, 2024
1ee3796
Merge main
amickan Aug 21, 2024
8006337
Fix typos
amickan Aug 21, 2024
c42e4a7
Complete API view tests
amickan Aug 21, 2024
b584d08
Consolidate InterfaceFormField logic
amickan Aug 23, 2024
e8d3580
Add tests
amickan Aug 23, 2024
a89b8a3
Add test for inputs_complete
amickan Aug 23, 2024
606818c
Make add_file_to_object idempotent
amickan Aug 23, 2024
1e32d75
Move util methods to serializer and job manager
amickan Aug 26, 2024
18c4521
Small fixes
amickan Aug 26, 2024
0fe7eb8
Add object locking and atomic transaction marker to celery tasks
amickan Aug 26, 2024
6cb1eb9
Revert "Add object locking and atomic transaction marker to celery ta…
amickan Aug 28, 2024
e9ce96d
Address PR review
amickan Aug 28, 2024
49ac398
Catch more errors, add tests
amickan Aug 29, 2024
7afe9d3
Replace TestCase instances with fixture
amickan Aug 29, 2024
6ac0154
Fix tests
amickan Aug 29, 2024
3857d1c
Update reformat inputs
amickan Aug 29, 2024
d59e32e
Do object locking and status update within transaction
amickan Aug 29, 2024
d6ec56e
Add permission checks to file upload view
amickan Aug 29, 2024
ef574ec
Prepend interface slug on file upload view
amickan Aug 29, 2024
52465be
Make widget name a child class property for file upload view
amickan Aug 30, 2024
2209161
Add object locking
amickan Aug 30, 2024
c872738
Catch all exceptions in async tasks
amickan Aug 30, 2024
d38f02d
Fix tests
amickan Aug 30, 2024
230e1a2
Refactor create_civ method
amickan Aug 30, 2024
de459e9
Refactor error handling for civ creation
amickan Sep 2, 2024
69d4b6a
Address merge conflicts
amickan Sep 12, 2024
f9daede
Don't raise error when job already provisioned
amickan Sep 12, 2024
78a04c7
Change log level
amickan Sep 12, 2024
8fc5ea8
Resolve merge conflicts
amickan Sep 12, 2024
edfdfa3
Rerun pre-commit
amickan Sep 12, 2024
172014f
Make sure provision_job gets called only once
amickan Sep 16, 2024
b4d54a5
Fix line ending changes by pycharm
amickan Sep 16, 2024
d7e4545
Reintroduce checking for pending jobs
amickan Sep 16, 2024
501b90c
Refactor build_images notification sending
amickan Sep 16, 2024
35f5149
Fix tests
amickan Sep 16, 2024
9e63eaf
Merge branch 'main' into job_refactoring
jmsmkn Sep 16, 2024
5465984
Undo css changes
jmsmkn Sep 16, 2024
329fdfa
Merge main
amickan Sep 18, 2024
e04402c
Redo migrations
amickan Sep 18, 2024
fc5a8f2
Refactor error handling in build_images
amickan Sep 18, 2024
d2bc25f
Refactor CIV creation and validation
amickan Sep 18, 2024
736a3eb
Merge main
amickan Sep 19, 2024
0f5137f
Merge branch 'main' into job_refactoring
amickan Sep 19, 2024
399f219
Redo migrations
amickan Sep 19, 2024
91940ca
Add proper interface prefix to all interface form fields
amickan Sep 19, 2024
0d3c3b5
Add interface prefix in tests
amickan Sep 19, 2024
6d24448
Redesign CIVData
amickan Sep 19, 2024
cbd9210
Use just one view for file uploads and list all CIVs a user has permi…
amickan Sep 19, 2024
eabb979
Revert ValuesForInterfacesMixin changes
amickan Sep 19, 2024
4f199d1
Add test for file_civs_user_has_permission_to_use
amickan Sep 20, 2024
1556b06
Use VALIDATING_INPUTS status to determine execution readiness
amickan Sep 20, 2024
61038d0
Fix get_metrics test
amickan Sep 20, 2024
ab237f8
Remove leftover non_interface_fields, unused template and test
amickan Sep 20, 2024
9811d01
Refactor error handling and notification sending
amickan Sep 20, 2024
7ab897f
Only update linked_object on upload session failure
amickan Sep 20, 2024
0803ac4
Fix tests
amickan Sep 20, 2024
44c40a2
Subset selectable files by interface and update job session js
amickan Sep 23, 2024
17adc36
Remove leftover use of removed handle_error function
amickan Sep 23, 2024
8cb5901
Improve error message
amickan Sep 23, 2024
d2ce351
Fix tests
amickan Sep 23, 2024
d4788a5
Update test error message
amickan Sep 23, 2024
1500f7f
Merge branch 'main' into job_refactoring
amickan Oct 1, 2024
11d9da2
Addres review
amickan Oct 1, 2024
96959dc
Unify file civ serving permission checks
amickan Oct 2, 2024
cb01743
Switch to local import
amickan Oct 2, 2024
1008630
Move permission checking to serving app
amickan Oct 2, 2024
0267657
Update annotation and test
amickan Oct 3, 2024
0b469c7
Resolve conflicts with main
amickan Oct 3, 2024
6a6c3c5
Redo migrations
amickan Oct 3, 2024
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
52 changes: 50 additions & 2 deletions app/grandchallenge/algorithms/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
from grandchallenge.components.forms import ContainerImageForm
from grandchallenge.components.models import (
ComponentInterface,
ComponentInterfaceValue,
ComponentJob,
ImportStatusChoices,
InterfaceKindChoices,
Expand Down Expand Up @@ -93,16 +94,27 @@ class JobCreateForm(SaveFormInitMixin, Form):
algorithm_model = ModelChoiceField(
queryset=None, disabled=True, required=False, widget=HiddenInput
)
creator = ModelChoiceField(
queryset=None, disabled=True, required=False, widget=HiddenInput
)
time_limit = IntegerField(
disabled=True, required=False, widget=HiddenInput
)
amickan marked this conversation as resolved.
Show resolved Hide resolved

def __init__(self, *args, algorithm, user, **kwargs):
super().__init__(*args, **kwargs)

self.helper = FormHelper()

self._user = user
self.fields["creator"].queryset = get_user_model().objects.filter(
pk=self._user.pk
)
self.fields["creator"].initial = self._user

self._algorithm = algorithm
self._algorithm_image = self._algorithm.active_image
self.fields["time_limit"].initial = self._algorithm.time_limit

active_model = self._algorithm.active_model

Expand All @@ -119,12 +131,38 @@ def __init__(self, *args, algorithm, user, **kwargs):
self.fields["algorithm_model"].initial = active_model

for inp in self._algorithm.inputs.all():
if (
inp.requires_file
and inp.slug in self._algorithm.values_for_interfaces.keys()
):
existing_civs = ComponentInterfaceValue.objects.filter(
id__in=self._algorithm.values_for_interfaces[inp.slug]
)
else:
# Images are handled separately in FlexibleImageWidget
# Values stored in the DB need to be re-entered,
# we re-use existing CIVs for values later as well,
# but on the form level it does not make sense to check for those
existing_civs = None

if inp.slug in self.data:
if inp.kind == ComponentInterface.Kind.ANY:
# interfaces for which the data can be a list need
# to be retrieved with getlist() from the QueryDict
initial = self.data.getlist(inp.slug)
else:
initial = self.data[inp.slug]
else:
initial = None

self.fields[inp.slug] = InterfaceFormField(
instance=inp,
initial=inp.default_value,
initial=initial if initial else inp.default_value,
user=self._user,
required=inp.value_required,
required=True,
help_text=clean(inp.description) if inp.description else "",
existing_civs=existing_civs,
form_data=self.data,
).field

@cached_property
Expand All @@ -143,6 +181,16 @@ def clean(self):
if self.jobs_limit < 1:
raise ValidationError("You have run out of algorithm credits")

if Job.objects.get_jobs_with_same_inputs(
data=cleaned_data,
algorithm_image=cleaned_data["algorithm_image"],
algorithm_model=cleaned_data["algorithm_model"],
):
raise ValidationError(
"A result for these inputs with the current image "
"and model already exists."
)

return cleaned_data


Expand Down
177 changes: 154 additions & 23 deletions app/grandchallenge/algorithms/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
from django.core.exceptions import ObjectDoesNotExist
from django.core.validators import MaxValueValidator, MinValueValidator
from django.db import models
from django.db.models import Count, Min, Q, Sum
from django.db.models import Count, Min, Q, QuerySet, Sum
from django.db.models.signals import post_delete
from django.db.transaction import on_commit
from django.dispatch import receiver
from django.template.defaultfilters import truncatechars
from django.utils import timezone
Expand All @@ -26,14 +25,18 @@
from stdimage import JPEGField

from grandchallenge.anatomy.models import BodyStructure
from grandchallenge.cases.models import Image, RawImageUploadSession
from grandchallenge.charts.specs import stacked_bar
from grandchallenge.components.models import (
from grandchallenge.components.models import ( # noqa: F401
CIVForObjectMixin,
ComponentImage,
ComponentInterface,
ComponentInterfaceValue,
ComponentJob,
ComponentJobManager,
ImportStatusChoices,
Tarball,
ValuesForInterfacesMixin,
)
from grandchallenge.core.guardian import get_objects_for_group
from grandchallenge.core.models import RequestBase, UUIDModel
Expand All @@ -57,14 +60,20 @@
from grandchallenge.publications.models import Publication
from grandchallenge.reader_studies.models import DisplaySet
from grandchallenge.subdomains.utils import reverse
from grandchallenge.uploads.models import UserUpload
from grandchallenge.workstations.models import Workstation

logger = logging.getLogger(__name__)

JINJA_ENGINE = sandbox.ImmutableSandboxedEnvironment()


class Algorithm(UUIDModel, TitleSlugDescriptionModel, HangingProtocolMixin):
class Algorithm(
UUIDModel,
TitleSlugDescriptionModel,
ValuesForInterfacesMixin,
HangingProtocolMixin,
):
editors_group = models.OneToOneField(
Group,
on_delete=models.PROTECT,
Expand Down Expand Up @@ -420,6 +429,17 @@ def add_user(self, user):
def remove_user(self, user):
return user.groups.remove(self.users_group)

@property
def civ_set_lookup(self):
return "inputs"

@property
def civ_sets_related_manager(self):
# should return all jobs for this algorithm (not just for the active image)
amickan marked this conversation as resolved.
Show resolved Hide resolved
return Job.objects.filter(
algorithm_image__in=self.algorithm_container_images.all()
amickan marked this conversation as resolved.
Show resolved Hide resolved
)

@cached_property
def user_statistics(self):
return (
Expand Down Expand Up @@ -629,6 +649,118 @@ def create(

return obj

@property
def non_interface_fields(self):
return ["algorithm_image", "algorithm_model", "creator", "time_limit"]
amickan marked this conversation as resolved.
Show resolved Hide resolved

def retrieve_existing_civs(self, *, civ_data):
amickan marked this conversation as resolved.
Show resolved Hide resolved
"""
Checks if there are existing CIVs for the provided data and returns those.

Parameters
----------
civ_data
A dictionary with interface slugs as keys and CIV values as values.
amickan marked this conversation as resolved.
Show resolved Hide resolved

Returns
-------
A list of ComponentInterfaceValues

"""
existing_civs = []
for interface, value in civ_data.items():
amickan marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(value, ComponentInterfaceValue):
existing_civs.append(value)
elif isinstance(value, Image):
ci = ComponentInterface.objects.get(slug=interface)
try:
civ = ComponentInterfaceValue.objects.filter(
interface=ci, image=value
amickan marked this conversation as resolved.
Show resolved Hide resolved
).get()
existing_civs.append(civ)
except ObjectDoesNotExist:
continue
elif isinstance(
value, (RawImageUploadSession, UserUpload, QuerySet)
):
# uploads will create new CIVs, so ignore these
continue
amickan marked this conversation as resolved.
Show resolved Hide resolved
else:
# values can be of different types
ci = ComponentInterface.objects.get(slug=interface)
try:
civ = ComponentInterfaceValue.objects.filter(
interface=ci, value=value
amickan marked this conversation as resolved.
Show resolved Hide resolved
).get()
existing_civs.append(civ)
except ObjectDoesNotExist:
continue

return existing_civs

def get_jobs_with_same_inputs(
self, *, data, algorithm_image, algorithm_model
amickan marked this conversation as resolved.
Show resolved Hide resolved
):
civ_data = {
k: v for k, v in data.items() if k not in self.non_interface_fields
amickan marked this conversation as resolved.
Show resolved Hide resolved
}
existing_civs = self.retrieve_existing_civs(civ_data=civ_data)
unique_kwargs = {
"algorithm_image": algorithm_image,
}
input_interface_count = algorithm_image.algorithm.inputs.count()
if algorithm_model:
unique_kwargs["algorithm_model"] = algorithm_model
else:
unique_kwargs["algorithm_model__isnull"] = True
existing_jobs = (
amickan marked this conversation as resolved.
Show resolved Hide resolved
Job.objects.filter(**unique_kwargs)
.annotate(
inputs_match_count=Count(
"inputs", filter=Q(inputs__in=existing_civs)
)
)
.filter(inputs_match_count=input_interface_count)
jmsmkn marked this conversation as resolved.
Show resolved Hide resolved
)
return existing_jobs

def create_job_and_process_inputs(
self,
*,
data,
amickan marked this conversation as resolved.
Show resolved Hide resolved
extra_viewer_groups=None,
extra_logs_viewer_groups=None,
):
civ_data = {
k: v for k, v in data.items() if k not in self.non_interface_fields
}
non_civ_data = {
k: v for k, v in data.items() if k in self.non_interface_fields
}
job = Job.objects.create(
**non_civ_data,
amickan marked this conversation as resolved.
Show resolved Hide resolved
extra_viewer_groups=extra_viewer_groups,
extra_logs_viewer_groups=extra_logs_viewer_groups,
)
# local import to avoid circular dependency
from grandchallenge.algorithms.tasks import (
execute_algorithm_job_for_inputs,
)

linked_task = execute_algorithm_job_for_inputs.signature(
kwargs={"job_pk": job.pk}, immutable=True
)

for key, value in civ_data.items():
job.create_civ(
ci_slug=key,
new_value=value,
user=data["creator"],
linked_task=linked_task,
amickan marked this conversation as resolved.
Show resolved Hide resolved
)

return job

def spent_credits(self, user):
now = timezone.now()
period = timedelta(days=30)
Expand Down Expand Up @@ -712,7 +844,7 @@ class AlgorithmModelGroupObjectPermission(GroupObjectPermissionBase):
)


class Job(UUIDModel, ComponentJob):
class Job(UUIDModel, CIVForObjectMixin, ComponentJob):
objects = JobManager.as_manager()

algorithm_image = models.ForeignKey(
Expand Down Expand Up @@ -770,6 +902,20 @@ def container(self):
def output_interfaces(self):
return self.algorithm_image.algorithm.outputs

@cached_property
def inputs_complete(self):
# check if all inputs are present and if they all have a value
input_interfaces = self.algorithm_image.algorithm.inputs.all()
existing_inputs = [
civ.interface for civ in self.inputs.all() if civ.has_value
]
missing_inputs = [
interface
for interface in input_interfaces
if interface not in existing_inputs
amickan marked this conversation as resolved.
Show resolved Hide resolved
]
return not missing_inputs

@cached_property
def rendered_result_text(self) -> str:
try:
Expand Down Expand Up @@ -883,24 +1029,9 @@ def add_viewer(self, user):
def remove_viewer(self, user):
return user.groups.remove(self.viewers)

def sort_inputs_and_execute(
self, upload_session_pks=None, user_upload_pks=None
):
# Local import to avoid circular dependency
from grandchallenge.algorithms.tasks import (
run_algorithm_job_for_inputs,
)

on_commit(
run_algorithm_job_for_inputs.signature(
kwargs={
"job_pk": self.pk,
"upload_session_pks": upload_session_pks,
"user_upload_pks": user_upload_pks,
},
immutable=True,
).apply_async
)
@property
def base_object(self):
return self.algorithm_image.algorithm

@property
def executor_kwargs(self):
Expand Down
Loading