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 all 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
68 changes: 65 additions & 3 deletions app/grandchallenge/algorithms/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@
AlgorithmSerializer,
)
from grandchallenge.algorithms.tasks import import_remote_algorithm_image
from grandchallenge.components.form_fields import InterfaceFormField
from grandchallenge.components.form_fields import (
INTERFACE_FORM_FIELD_PREFIX,
InterfaceFormField,
)
from grandchallenge.components.forms import ContainerImageForm
from grandchallenge.components.models import (
CIVData,
ComponentInterface,
ComponentJob,
ImportStatusChoices,
Expand Down Expand Up @@ -93,16 +97,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 +134,27 @@ def __init__(self, *args, algorithm, user, **kwargs):
self.fields["algorithm_model"].initial = active_model

for inp in self._algorithm.inputs.all():
self.fields[inp.slug] = InterfaceFormField(
prefixed_interface_slug = (
f"{INTERFACE_FORM_FIELD_PREFIX}{inp.slug}"
)

if prefixed_interface_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(prefixed_interface_slug)
else:
initial = self.data[prefixed_interface_slug]
else:
initial = None

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

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

cleaned_data = self.reformat_inputs(cleaned_data=cleaned_data)

if Job.objects.get_jobs_with_same_inputs(
inputs=cleaned_data["inputs"],
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

def reformat_inputs(self, *, cleaned_data):
keys_to_remove = []
inputs = []
for k, v in cleaned_data.items():
if k.startswith(INTERFACE_FORM_FIELD_PREFIX):
keys_to_remove.append(k)
inputs.append(
CIVData(
interface_slug=k[len(INTERFACE_FORM_FIELD_PREFIX) :],
value=v,
)
)

for key in keys_to_remove:
cleaned_data.pop(key)

cleaned_data["inputs"] = inputs

return cleaned_data


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Generated by Django 4.2.16 on 2024-09-20 08:40

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("algorithms", "0054_alter_algorithmimage_options_and_more"),
]

operations = [
migrations.AlterField(
model_name="job",
name="status",
field=models.PositiveSmallIntegerField(
choices=[
(0, "Queued"),
(1, "Started"),
(2, "Re-Queued"),
(3, "Failed"),
(4, "Succeeded"),
(5, "Cancelled"),
(6, "Provisioning"),
(7, "Provisioned"),
(8, "Executing"),
(9, "Executed"),
(10, "Parsing Outputs"),
(11, "Executing Algorithm"),
(12, "External Execution In Progress"),
(13, "Validating inputs"),
],
db_index=True,
default=0,
),
),
]
139 changes: 123 additions & 16 deletions app/grandchallenge/algorithms/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@
from grandchallenge.algorithms.tasks import update_algorithm_average_duration
from grandchallenge.anatomy.models import BodyStructure
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,
Expand Down Expand Up @@ -626,6 +628,78 @@ def create(

return obj

@staticmethod
def retrieve_existing_civs(*, civ_data):
"""
Checks if there are existing CIVs for the provided data and returns those.

Parameters
----------
civ_data
A list of CIVData objects.

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

"""
existing_civs = []
for civ in civ_data:
if (
civ.user_upload
or civ.upload_session
or civ.user_upload_queryset
):
# uploads will create new CIVs, so ignore these
continue
elif civ.image:
try:
civ = ComponentInterfaceValue.objects.filter(
interface__slug=civ.interface_slug, image=civ.image
).get()
existing_civs.append(civ)
except ObjectDoesNotExist:
continue
elif civ.file_civ:
existing_civs.append(civ.file_civ)
else:
# values can be of different types, including None and False
try:
civ = ComponentInterfaceValue.objects.filter(
interface__slug=civ.interface_slug, value=civ.value
).get()
existing_civs.append(civ)
except ObjectDoesNotExist:
continue

return existing_civs

def get_jobs_with_same_inputs(
self, *, inputs, algorithm_image, algorithm_model
):
existing_civs = self.retrieve_existing_civs(civ_data=inputs)
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 = (
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 spent_credits(self, user):
now = timezone.now()
period = timedelta(days=30)
Expand Down Expand Up @@ -714,7 +788,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 @@ -772,6 +846,13 @@ 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
return {
civ.interface for civ in self.inputs.all() if civ.has_value
} == {*self.algorithm_image.algorithm.inputs.all()}

@cached_property
def rendered_result_text(self) -> str:
try:
Expand Down Expand Up @@ -889,25 +970,51 @@ 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
def add_civ(self, *, civ):
super().add_civ(civ=civ)
return self.inputs.add(civ)

def remove_civ(self, *, civ):
super().remove_civ(civ=civ)
return self.inputs.remove(civ)

def get_civ_for_interface(self, interface):
return self.inputs.get(interface=interface)

def validate_inputs_and_execute(self, *, inputs):
from grandchallenge.algorithms.tasks import (
run_algorithm_job_for_inputs,
execute_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
linked_task = execute_algorithm_job_for_inputs.signature(
kwargs={"job_pk": self.pk}, immutable=True
)

if not self.is_editable:
raise RuntimeError(
"Job is not editable. No CIVs can be added or removed from it."
)
else:
for civ_data in inputs:
self.create_civ(
civ_data=civ_data,
user=self.creator,
linked_task=linked_task,
)

@property
def is_editable(self):
# staying with display set and archive item terminology here
# since this property is checked in create_civ()
if self.status == self.VALIDATING_INPUTS:
return True
else:
return False

@property
def base_object(self):
return self.algorithm_image.algorithm

@property
def executor_kwargs(self):
executor_kwargs = super().executor_kwargs
Expand Down
Loading