From 40156db8bf6ad218806812743918e824b8a54515 Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Fri, 6 Dec 2024 10:44:53 +0100 Subject: [PATCH 01/18] Add AlgorithmInterface model (#3720) First PR for https://github.com/DIAGNijmegen/rse-roadmap/issues/153 This adds the `AlgorithmInterface` model, as well as the necessary through table, and removes the inputs and outputs fields from the algorithm form. The following restrictions apply: - AlgorithmInterfaces cannot be deleted (protection on model level and admin form level) - AlgorithmInterfaces cannot be updated (protection on admin form level) - AlgorithmInterfaces are unique (i.e. input and output combinations are unique, we check for that in the form) - An algorithm can only have 1 default algorithm interface - There can only be one entry per algorithm and interface in the through table. --------- Co-authored-by: James Meakin <12661555+jmsmkn@users.noreply.github.com> --- app/grandchallenge/algorithms/admin.py | 64 ++++++- app/grandchallenge/algorithms/forms.py | 98 ++++------- ...rface_algorithminterfaceoutput_and_more.py | 162 ++++++++++++++++++ .../0064_create_algorithm_interfaces.py | 41 +++++ app/grandchallenge/algorithms/models.py | 101 +++++++++++ app/grandchallenge/algorithms/views.py | 19 +- app/tests/algorithms_tests/factories.py | 22 ++- app/tests/algorithms_tests/test_admin.py | 12 -- app/tests/algorithms_tests/test_forms.py | 41 +++-- app/tests/algorithms_tests/test_models.py | 118 +++++++++++++ app/tests/algorithms_tests/test_views.py | 25 --- 11 files changed, 569 insertions(+), 134 deletions(-) create mode 100644 app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py create mode 100644 app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py delete mode 100644 app/tests/algorithms_tests/test_admin.py diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index b9ac6ff3fc..044393a2dc 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -2,19 +2,21 @@ from django.contrib import admin from django.contrib.admin import ModelAdmin -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import Count, Sum from django.forms import ModelForm from django.utils.html import format_html from guardian.admin import GuardedModelAdmin -from grandchallenge.algorithms.forms import AlgorithmIOValidationMixin +from grandchallenge.algorithms.forms import AlgorithmInterfaceBaseForm from grandchallenge.algorithms.models import ( Algorithm, + AlgorithmAlgorithmInterface, AlgorithmGroupObjectPermission, AlgorithmImage, AlgorithmImageGroupObjectPermission, AlgorithmImageUserObjectPermission, + AlgorithmInterface, AlgorithmModel, AlgorithmModelGroupObjectPermission, AlgorithmModelUserObjectPermission, @@ -36,12 +38,13 @@ UserObjectPermissionAdmin, ) from grandchallenge.core.templatetags.costs import millicents_to_euro +from grandchallenge.core.templatetags.remove_whitespace import oxford_comma from grandchallenge.core.utils.grand_challenge_forge import ( get_forge_algorithm_template_context, ) -class AlgorithmAdminForm(AlgorithmIOValidationMixin, ModelForm): +class AlgorithmAdminForm(ModelForm): class Meta: model = Algorithm fields = "__all__" @@ -49,11 +52,12 @@ class Meta: @admin.register(Algorithm) class AlgorithmAdmin(GuardedModelAdmin): - readonly_fields = ("algorithm_forge_json",) + readonly_fields = ("algorithm_forge_json", "inputs", "outputs") list_display = ( "title", "created", "public", + "default_io", "time_limit", "job_requires_gpu_type", "job_requires_memory_gb", @@ -75,6 +79,11 @@ def algorithm_forge_json(obj): "
{json_desc}
", json_desc=json.dumps(json_desc, indent=2) ) + def default_io(self, obj): + return AlgorithmAlgorithmInterface.objects.get( + algorithm=obj, is_default=True + ) + def get_queryset(self, request): queryset = super().get_queryset(request) queryset = queryset.annotate( @@ -235,6 +244,53 @@ class AlgorithmModelAdmin(GuardedModelAdmin): readonly_fields = ("creator", "algorithm", "sha256", "size_in_storage") +class AlgorithmInterfaceAdminForm(AlgorithmInterfaceBaseForm): + def clean(self): + cleaned_data = super().clean() + if cleaned_data["existing_io"]: + raise ValidationError( + "An AlgorithmIO with the same combination of inputs and outputs already exists." + ) + return cleaned_data + + +@admin.register(AlgorithmInterface) +class AlgorithmInterfaceAdmin(GuardedModelAdmin): + list_display = ( + "pk", + "algorithm_inputs", + "algorithm_outputs", + ) + search_fields = ( + "inputs__slug", + "outputs__slug", + ) + form = AlgorithmInterfaceAdminForm + + def algorithm_inputs(self, obj): + return oxford_comma(obj.inputs.all()) + + def algorithm_outputs(self, obj): + return oxford_comma(obj.outputs.all()) + + def has_change_permission(self, request, obj=None): + return False + + def has_delete_permission(self, request, obj=None): + return False + + +@admin.register(AlgorithmAlgorithmInterface) +class AlgorithmAlgorithmInterfaceAdmin(GuardedModelAdmin): + list_display = ( + "pk", + "interface", + "is_default", + "algorithm", + ) + list_filter = ("is_default", "algorithm") + + admin.site.register(AlgorithmUserObjectPermission, UserObjectPermissionAdmin) admin.site.register(AlgorithmGroupObjectPermission, GroupObjectPermissionAdmin) admin.site.register(AlgorithmImage, ComponentImageAdmin) diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index a61df6de75..9d873979ee 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -30,7 +30,6 @@ HiddenInput, ModelChoiceField, ModelForm, - ModelMultipleChoiceField, Select, TextInput, URLField, @@ -45,9 +44,11 @@ from grandchallenge.algorithms.models import ( Algorithm, AlgorithmImage, + AlgorithmInterface, AlgorithmModel, AlgorithmPermissionRequest, Job, + get_existing_interface_for_inputs_and_outputs, ) from grandchallenge.algorithms.serializers import ( AlgorithmImageSerializer, @@ -220,60 +221,11 @@ def reformat_inputs(self, *, cleaned_data): ] -class AlgorithmIOValidationMixin: - def clean(self): - cleaned_data = super().clean() - - duplicate_interfaces = {*cleaned_data.get("inputs", [])}.intersection( - {*cleaned_data.get("outputs", [])} - ) - - if duplicate_interfaces: - raise ValidationError( - f"The sets of Inputs and Outputs must be unique: " - f"{oxford_comma(duplicate_interfaces)} present in both" - ) - - return cleaned_data - - class AlgorithmForm( - AlgorithmIOValidationMixin, WorkstationUserFilterMixin, SaveFormInitMixin, ModelForm, ): - inputs = ModelMultipleChoiceField( - queryset=ComponentInterface.objects.exclude( - slug__in=[*NON_ALGORITHM_INTERFACES, "results-json-file"] - ), - widget=Select2MultipleWidget, - help_text=format_lazy( - ( - "The inputs to this algorithm. " - 'See the list of interfaces for more ' - "information about each interface. " - "Please contact support if your desired input is missing." - ), - reverse_lazy("components:component-interface-list-algorithms"), - ), - ) - outputs = ModelMultipleChoiceField( - queryset=ComponentInterface.objects.exclude( - slug__in=NON_ALGORITHM_INTERFACES - ), - widget=Select2MultipleWidget, - help_text=format_lazy( - ( - "The outputs to this algorithm. " - 'See the list of interfaces for more ' - "information about each interface. " - "Please contact support if your desired output is missing." - ), - reverse_lazy("components:component-interface-list-algorithms"), - ), - ) - class Meta: model = Algorithm fields = ( @@ -293,8 +245,6 @@ class Meta: "hanging_protocol", "optional_hanging_protocols", "view_content", - "inputs", - "outputs", "minimum_credits_per_job", "job_requires_gpu_type", "job_requires_memory_gb", @@ -638,15 +588,6 @@ def __init__(self, *args, **kwargs): ) -class AlgorithmUpdateForm(AlgorithmForm): - def __init__(self, *args, interfaces_editable, **kwargs): - super().__init__(*args, **kwargs) - - if not interfaces_editable: - for field_key in ("inputs", "outputs"): - self.fields.pop(field_key) - - class AlgorithmImageForm(ContainerImageForm): algorithm = ModelChoiceField(widget=HiddenInput(), queryset=None) @@ -1289,6 +1230,7 @@ def __init__( ) if hide_algorithm_model_input: + self.fields["algorithm_model"].widget = HiddenInput() self.helper = FormHelper(self) if activate: @@ -1317,3 +1259,37 @@ def clean_algorithm_model(self): raise ValidationError("Model updating already in progress.") return algorithm_model + + +class AlgorithmInterfaceBaseForm(SaveFormInitMixin, ModelForm): + class Meta: + model = AlgorithmInterface + fields = ("inputs", "outputs") + + def clean(self): + cleaned_data = super().clean() + + inputs = cleaned_data.get("inputs", []) + outputs = cleaned_data.get("outputs", []) + + if not inputs or not outputs: + raise ValidationError( + "You must provide at least 1 input and 1 output." + ) + + duplicate_interfaces = {*inputs}.intersection({*outputs}) + + if duplicate_interfaces: + raise ValidationError( + f"The sets of Inputs and Outputs must be unique: " + f"{oxford_comma(duplicate_interfaces)} present in both" + ) + + cleaned_data["existing_io"] = ( + get_existing_interface_for_inputs_and_outputs( + inputs=inputs, + outputs=outputs, + ) + ) + + return cleaned_data diff --git a/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py b/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py new file mode 100644 index 0000000000..9a0997b11b --- /dev/null +++ b/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py @@ -0,0 +1,162 @@ +# Generated by Django 4.2.16 on 2024-12-04 14:06 + +import uuid + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("components", "0022_alter_componentinterface_kind_and_more"), + ("algorithms", "0062_algorithmusercredit"), + ] + + operations = [ + migrations.CreateModel( + name="AlgorithmInterface", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("created", models.DateTimeField(auto_now_add=True)), + ("modified", models.DateTimeField(auto_now=True)), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="AlgorithmInterfaceOutput", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "interface", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="algorithms.algorithminterface", + ), + ), + ( + "output", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="components.componentinterface", + ), + ), + ], + ), + migrations.CreateModel( + name="AlgorithmInterfaceInput", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "input", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="components.componentinterface", + ), + ), + ( + "interface", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="algorithms.algorithminterface", + ), + ), + ], + ), + migrations.AddField( + model_name="algorithminterface", + name="inputs", + field=models.ManyToManyField( + related_name="inputs", + through="algorithms.AlgorithmInterfaceInput", + to="components.componentinterface", + ), + ), + migrations.AddField( + model_name="algorithminterface", + name="outputs", + field=models.ManyToManyField( + related_name="outputs", + through="algorithms.AlgorithmInterfaceOutput", + to="components.componentinterface", + ), + ), + migrations.CreateModel( + name="AlgorithmAlgorithmInterface", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("is_default", models.BooleanField(default=False)), + ( + "algorithm", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="algorithms.algorithm", + ), + ), + ( + "interface", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="algorithms.algorithminterface", + ), + ), + ], + ), + migrations.AddField( + model_name="algorithm", + name="interfaces", + field=models.ManyToManyField( + related_name="algorithm_interfaces", + through="algorithms.AlgorithmAlgorithmInterface", + to="algorithms.algorithminterface", + ), + ), + migrations.AddConstraint( + model_name="algorithmalgorithminterface", + constraint=models.UniqueConstraint( + condition=models.Q(("is_default", True)), + fields=("algorithm",), + name="unique_default_interface_per_algorithm", + ), + ), + migrations.AddConstraint( + model_name="algorithmalgorithminterface", + constraint=models.UniqueConstraint( + fields=("algorithm", "interface"), + name="unique_algorithm_interface_combination", + ), + ), + ] diff --git a/app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py b/app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py new file mode 100644 index 0000000000..0f3bb8e8dd --- /dev/null +++ b/app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py @@ -0,0 +1,41 @@ +from django.db import migrations + +from grandchallenge.algorithms.models import ( + get_existing_interface_for_inputs_and_outputs, +) + + +def create_algorithm_interfaces(apps, _schema_editor): + Algorithm = apps.get_model("algorithms", "Algorithm") # noqa: N806 + AlgorithmInterface = apps.get_model( # noqa: N806 + "algorithms", "AlgorithmInterface" + ) + + for algorithm in Algorithm.objects.prefetch_related( + "inputs", "outputs" + ).all(): + inputs = algorithm.inputs.all() + outputs = algorithm.outputs.all() + + io = get_existing_interface_for_inputs_and_outputs( + model=AlgorithmInterface, inputs=inputs, outputs=outputs + ) + if not io: + io = AlgorithmInterface.objects.create() + io.inputs.set(inputs) + io.outputs.set(outputs) + + algorithm.interfaces.add(io, through_defaults={"is_default": True}) + + +class Migration(migrations.Migration): + dependencies = [ + ( + "algorithms", + "0063_algorithminterface_algorithminterfaceoutput_and_more", + ), + ] + + operations = [ + migrations.RunPython(create_algorithm_interfaces, elidable=True), + ] diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 1e03a57f7a..981189c82f 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -69,6 +69,80 @@ JINJA_ENGINE = sandbox.ImmutableSandboxedEnvironment() +class AlgorithmInterfaceManager(models.Manager): + + def create( + self, + *, + inputs, + outputs, + **kwargs, + ): + obj = get_existing_interface_for_inputs_and_outputs( + inputs=inputs, outputs=outputs + ) + if not obj: + obj = super().create(**kwargs) + obj.inputs.set(inputs) + obj.outputs.set(outputs) + + return obj + + def delete(self): + raise NotImplementedError("Bulk delete is not allowed.") + + +class AlgorithmInterface(UUIDModel): + inputs = models.ManyToManyField( + to=ComponentInterface, + related_name="inputs", + through="algorithms.AlgorithmInterfaceInput", + ) + outputs = models.ManyToManyField( + to=ComponentInterface, + related_name="outputs", + through="algorithms.AlgorithmInterfaceOutput", + ) + + objects = AlgorithmInterfaceManager() + + def delete(self, *args, **kwargs): + raise ValidationError("AlgorithmInterfaces cannot be deleted.") + + +class AlgorithmInterfaceInput(models.Model): + input = models.ForeignKey(ComponentInterface, on_delete=models.CASCADE) + interface = models.ForeignKey(AlgorithmInterface, on_delete=models.CASCADE) + + +class AlgorithmInterfaceOutput(models.Model): + output = models.ForeignKey(ComponentInterface, on_delete=models.CASCADE) + interface = models.ForeignKey(AlgorithmInterface, on_delete=models.CASCADE) + + +def get_existing_interface_for_inputs_and_outputs( + *, inputs, outputs, model=AlgorithmInterface +): + try: + return model.objects.annotate( + input_count=Count("inputs", distinct=True), + output_count=Count("outputs", distinct=True), + relevant_input_count=Count( + "inputs", filter=Q(inputs__in=inputs), distinct=True + ), + relevant_output_count=Count( + "outputs", filter=Q(outputs__in=outputs), distinct=True + ), + ).get( + relevant_input_count=len(inputs), + relevant_output_count=len(outputs), + input_count=len(inputs), + output_count=len(outputs), + ) + except ObjectDoesNotExist: + return None + + class Algorithm(UUIDModel, TitleSlugDescriptionModel, HangingProtocolMixin): editors_group = models.OneToOneField( Group, @@ -148,6 +222,11 @@ class Algorithm(UUIDModel, TitleSlugDescriptionModel, HangingProtocolMixin): "{% endfor %}" ), ) + interfaces = models.ManyToManyField( + to=AlgorithmInterface, + related_name="algorithm_interfaces", + through="algorithms.AlgorithmAlgorithmInterface", + ) inputs = models.ManyToManyField( to=ComponentInterface, related_name="algorithm_inputs", blank=False ) @@ -521,6 +600,28 @@ def form_field_label(self): return title +class AlgorithmAlgorithmInterface(models.Model): + algorithm = models.ForeignKey(Algorithm, on_delete=models.CASCADE) + interface = models.ForeignKey(AlgorithmInterface, on_delete=models.CASCADE) + is_default = models.BooleanField(default=False) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["algorithm"], + condition=Q(is_default=True), + name="unique_default_interface_per_algorithm", + ), + models.UniqueConstraint( + fields=["algorithm", "interface"], + name="unique_algorithm_interface_combination", + ), + ] + + def __str__(self): + return str(self.interface) + + class AlgorithmUserObjectPermission(UserObjectPermissionBase): content_object = models.ForeignKey(Algorithm, on_delete=models.CASCADE) diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index 842f1ffc5f..8005f8c76c 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -52,7 +52,6 @@ AlgorithmPermissionRequestUpdateForm, AlgorithmPublishForm, AlgorithmRepoForm, - AlgorithmUpdateForm, DisplaySetFromJobForm, ImageActivateForm, JobCreateForm, @@ -253,26 +252,10 @@ class AlgorithmUpdate( UpdateView, ): model = Algorithm - form_class = AlgorithmUpdateForm + form_class = AlgorithmForm permission_required = "algorithms.change_algorithm" raise_exception = True - def get_form_kwargs(self): - kwargs = super().get_form_kwargs() - - # Only users with the add_algorithm permission can change - # the input and output interfaces, other users must use - # the interfaces pre-set by the Phase - kwargs.update( - { - "interfaces_editable": self.request.user.has_perm( - "algorithms.add_algorithm" - ) - } - ) - - return kwargs - class AlgorithmDescriptionUpdate( LoginRequiredMixin, diff --git a/app/tests/algorithms_tests/factories.py b/app/tests/algorithms_tests/factories.py index d198d83ae4..c3027e72de 100644 --- a/app/tests/algorithms_tests/factories.py +++ b/app/tests/algorithms_tests/factories.py @@ -3,13 +3,17 @@ from grandchallenge.algorithms.models import ( Algorithm, AlgorithmImage, + AlgorithmInterface, AlgorithmModel, AlgorithmPermissionRequest, AlgorithmUserCredit, Job, ) from grandchallenge.components.models import GPUTypeChoices -from tests.components_tests.factories import ComponentInterfaceValueFactory +from tests.components_tests.factories import ( + ComponentInterfaceFactory, + ComponentInterfaceValueFactory, +) from tests.factories import ( ImageFactory, UserFactory, @@ -84,3 +88,19 @@ class Meta: user = factory.SubFactory(UserFactory) algorithm = factory.SubFactory(AlgorithmFactory) + + +class AlgorithmInterfaceFactory(factory.django.DjangoModelFactory): + class Meta: + model = AlgorithmInterface + + @classmethod + def _create(cls, model_class, *args, **kwargs): + manager = cls._get_manager(model_class) + inputs = kwargs.pop("inputs", None) + outputs = kwargs.pop("outputs", None) + if not inputs: + inputs = [ComponentInterfaceFactory()] + if not outputs: + outputs = [ComponentInterfaceFactory()] + return manager.create(*args, inputs=inputs, outputs=outputs, **kwargs) diff --git a/app/tests/algorithms_tests/test_admin.py b/app/tests/algorithms_tests/test_admin.py deleted file mode 100644 index 0b33043456..0000000000 --- a/app/tests/algorithms_tests/test_admin.py +++ /dev/null @@ -1,12 +0,0 @@ -import pytest - -from grandchallenge.algorithms.admin import AlgorithmAdmin -from tests.components_tests.factories import ComponentInterfaceFactory - - -@pytest.mark.django_db -def test_disjoint_interfaces(): - i = ComponentInterfaceFactory() - form = AlgorithmAdmin.form(data={"inputs": [i.pk], "outputs": [i.pk]}) - assert form.is_valid() is False - assert "The sets of Inputs and Outputs must be unique" in str(form.errors) diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 87ad0a2bf5..a16d38b695 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -5,6 +5,7 @@ from django.core.validators import MaxValueValidator, MinValueValidator from factory.django import ImageField +from grandchallenge.algorithms.admin import AlgorithmInterfaceAdminForm from grandchallenge.algorithms.forms import ( AlgorithmForm, AlgorithmForPhaseForm, @@ -35,6 +36,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, AlgorithmModelFactory, AlgorithmPermissionRequestFactory, @@ -155,7 +157,6 @@ def test_algorithm_create(client, uploaded_image): VerificationFactory(user=creator, is_verified=True) ws = WorkstationFactory() - ci = ComponentInterface.objects.get(slug="generic-medical-image") def try_create_algorithm(): return get_view_for_user( @@ -166,8 +167,7 @@ def try_create_algorithm(): "title": "foo bar", "logo": uploaded_image(), "workstation": ws.pk, - "inputs": [ci.pk], - "outputs": [ComponentInterfaceFactory().pk], + "interfaces": AlgorithmInterfaceFactory(), "minimum_credits_per_job": 20, "job_requires_gpu_type": GPUTypeChoices.NO_GPU, "job_requires_memory_gb": 4, @@ -407,16 +407,6 @@ def create_algorithm_with_input(slug): return alg, creator -@pytest.mark.django_db -def test_disjoint_interfaces(): - i = ComponentInterfaceFactory() - form = AlgorithmForm( - user=UserFactory(), data={"inputs": [i.pk], "outputs": [i.pk]} - ) - assert form.is_valid() is False - assert "The sets of Inputs and Outputs must be unique" in str(form.errors) - - @pytest.mark.django_db def test_publish_algorithm(): algorithm = AlgorithmFactory() @@ -889,3 +879,28 @@ def test_algorithm_for_phase_form_memory_limited(): ) assert max_validator is not None assert max_validator.limit_value == 32 + + +@pytest.mark.django_db +def test_algorithm_interface_check_for_uniqueness(): + interface = AlgorithmInterfaceFactory() + ci1, ci2 = ComponentInterfaceFactory.create_batch(2) + interface.inputs.set([ci1]) + interface.outputs.set([ci2]) + + form = AlgorithmInterfaceAdminForm( + data={"inputs": [ci1], "outputs": [ci2]} + ) + assert form.is_valid() is False + assert ( + "An AlgorithmIO with the same combination of inputs and outputs already exists." + in str(form.errors) + ) + + +@pytest.mark.django_db +def test_algorithm_interface_disjoint_interfaces(): + ci = ComponentInterfaceFactory() + form = AlgorithmInterfaceAdminForm(data={"inputs": [ci], "outputs": [ci]}) + assert form.is_valid() is False + assert "The sets of Inputs and Outputs must be unique" in str(form.errors) diff --git a/app/tests/algorithms_tests/test_models.py b/app/tests/algorithms_tests/test_models.py index 8ca3879413..f1c82b1b8a 100644 --- a/app/tests/algorithms_tests/test_models.py +++ b/app/tests/algorithms_tests/test_models.py @@ -6,14 +6,18 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.core.files.base import ContentFile +from django.db import IntegrityError, transaction from django.db.models import ProtectedError from django.test import TestCase from django.utils.timezone import now from grandchallenge.algorithms.models import ( Algorithm, + AlgorithmAlgorithmInterface, + AlgorithmInterface, AlgorithmUserCredit, Job, + get_existing_interface_for_inputs_and_outputs, ) from grandchallenge.components.models import ( CIVData, @@ -24,6 +28,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, AlgorithmModelFactory, AlgorithmUserCreditFactory, @@ -1379,3 +1384,116 @@ def test_active_credits_with_spent_credits(self): algorithm_image.get_remaining_non_complimentary_jobs(user=user) == 5 ) + + +@pytest.mark.django_db +def test_algorithm_interface_cannot_be_deleted(): + interface, _, _ = AlgorithmInterfaceFactory.create_batch(3) + + with pytest.raises(ValidationError): + interface.delete() + + with pytest.raises(NotImplementedError): + AlgorithmInterface.objects.delete() + + +@pytest.mark.django_db +def test_algorithmalgorithminterface_unique_constraints(): + interface1, interface2 = AlgorithmInterfaceFactory.create_batch(2) + algorithm = AlgorithmFactory() + + io = AlgorithmAlgorithmInterface.objects.create( + interface=interface1, algorithm=algorithm, is_default=True + ) + + # cannot add a second default interface to an algorithm + with pytest.raises(IntegrityError): + with transaction.atomic(): + AlgorithmAlgorithmInterface.objects.create( + interface=interface2, algorithm=algorithm, is_default=True + ) + + # cannot add a second time the same interface for the same algorithm + with pytest.raises(IntegrityError): + with transaction.atomic(): + AlgorithmAlgorithmInterface.objects.create( + interface=interface1, algorithm=algorithm, is_default=False + ) + + # but you can update an existing entry from default to non-default + io.is_default = False + io.save() + assert AlgorithmAlgorithmInterface.objects.count() == 1 + assert not AlgorithmAlgorithmInterface.objects.get().is_default + + +@pytest.mark.parametrize( + "inputs, outputs, expected_output", + ( + ([1], [2], 1), + ([1, 2], [3, 4], 2), + ([3, 4, 5], [6], 3), + ([1], [3, 4, 6], 4), + ([5, 6], [2], 5), + ([1], [3], None), + ([1], [3, 4], None), + ([2], [6], None), + ([2], [1], None), + ([3, 4, 5], [1], None), + ([1], [3, 4], None), + ([1, 3], [4], None), + ), +) +@pytest.mark.django_db +def test_get_existing_interface_for_inputs_and_outputs( + inputs, outputs, expected_output +): + io1, io2, io3, io4, io5, io6 = AlgorithmInterfaceFactory.create_batch(6) + ci1, ci2, ci3, ci4, ci5, ci6 = ComponentInterfaceFactory.create_batch(6) + + interfaces = [io1, io2, io3, io4, io5, io6] + cis = [ci1, ci2, ci3, ci4, ci5, ci6] + + io1.inputs.set([ci1]) + io2.inputs.set([ci1, ci2]) + io3.inputs.set([ci3, ci4, ci5]) + io4.inputs.set([ci1]) + io5.inputs.set([ci5, ci6]) + io6.inputs.set([ci1, ci2]) + + io1.outputs.set([ci2]) + io2.outputs.set([ci3, ci4]) + io3.outputs.set([ci6]) + io4.outputs.set([ci3, ci4, ci6]) + io5.outputs.set([ci2]) + io6.outputs.set([ci4]) + + inputs = [cis[i - 1] for i in inputs] + outputs = [cis[i - 1] for i in outputs] + + existing_interface = get_existing_interface_for_inputs_and_outputs( + inputs=inputs, outputs=outputs + ) + + if expected_output: + assert existing_interface == interfaces[expected_output - 1] + else: + assert not existing_interface + + +@pytest.mark.django_db +def test_algorithminterface_create(): + inputs = [ComponentInterfaceFactory(), ComponentInterfaceFactory()] + outputs = [ComponentInterfaceFactory(), ComponentInterfaceFactory()] + + with pytest.raises(TypeError) as e: + AlgorithmInterface.objects.create() + + assert ( + "AlgorithmInterfaceManager.create() missing 2 required keyword-only arguments: 'inputs' and 'outputs'" + in str(e) + ) + + io = AlgorithmInterface.objects.create(inputs=inputs, outputs=outputs) + assert list(io.inputs.all()) == inputs + assert list(io.outputs.all()) == outputs diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 2f1d3ee1d1..220ad5959c 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -1635,31 +1635,6 @@ def test_algorithm_image_activate( assert i2.is_in_registry -@pytest.mark.django_db -@pytest.mark.parametrize("interfaces_editable", (True, False)) -def test_algorithm_interfaces_editable(client, interfaces_editable): - creator = UserFactory() - VerificationFactory(user=creator, is_verified=True) - - if interfaces_editable: - assign_perm("algorithms.add_algorithm", creator) - - alg = AlgorithmFactory() - alg.add_editor(user=creator) - - response = get_view_for_user( - viewname="algorithms:update", - client=client, - reverse_kwargs={"slug": alg.slug}, - user=creator, - ) - - assert ("inputs" in response.context["form"].fields) is interfaces_editable - assert ( - "outputs" in response.context["form"].fields - ) is interfaces_editable - - @pytest.mark.django_db def test_job_time_limit(client): algorithm = AlgorithmFactory(time_limit=600) From 2f9a6b15eb9487869ecdd6de992afe356d3f11a1 Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Thu, 12 Dec 2024 13:41:27 +0100 Subject: [PATCH 02/18] Add create and list views for algorithm interfaces (#3735) Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 This adds a create and list view for algorithm interfaces that is accessible to algorithm editors with the global `add_algorithm` permission. Create view: ![image](https://github.com/user-attachments/assets/4af87e6b-ef54-4a79-91e5-6df9f4ffb8c3) List view: ![image](https://github.com/user-attachments/assets/221b9cc4-bf01-43e3-833c-db6a082106cb) --------- Co-authored-by: James Meakin <12661555+jmsmkn@users.noreply.github.com> --- app/grandchallenge/algorithms/admin.py | 32 ++-- app/grandchallenge/algorithms/forms.py | 68 ++++++-- app/grandchallenge/algorithms/models.py | 9 ++ .../algorithms/algorithm_detail.html | 4 + .../algorithmalgorithminterface_list.html | 52 ++++++ .../algorithms/algorithminterface_form.html | 42 +++++ app/grandchallenge/algorithms/urls.py | 12 ++ app/grandchallenge/algorithms/views.py | 60 +++++++ app/tests/algorithms_tests/test_forms.py | 152 +++++++++++++++--- app/tests/algorithms_tests/test_models.py | 13 ++ app/tests/algorithms_tests/test_views.py | 107 +++++++++++- 11 files changed, 502 insertions(+), 49 deletions(-) create mode 100644 app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html create mode 100644 app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index 044393a2dc..966a4af6f1 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -2,13 +2,12 @@ from django.contrib import admin from django.contrib.admin import ModelAdmin -from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ObjectDoesNotExist from django.db.models import Count, Sum from django.forms import ModelForm from django.utils.html import format_html from guardian.admin import GuardedModelAdmin -from grandchallenge.algorithms.forms import AlgorithmInterfaceBaseForm from grandchallenge.algorithms.models import ( Algorithm, AlgorithmAlgorithmInterface, @@ -52,12 +51,17 @@ class Meta: @admin.register(Algorithm) class AlgorithmAdmin(GuardedModelAdmin): - readonly_fields = ("algorithm_forge_json", "inputs", "outputs") + readonly_fields = ( + "algorithm_forge_json", + "default_interface", + "inputs", + "outputs", + ) list_display = ( "title", "created", "public", - "default_io", + "default_interface", "time_limit", "job_requires_gpu_type", "job_requires_memory_gb", @@ -79,11 +83,6 @@ def algorithm_forge_json(obj): "
{json_desc}
", json_desc=json.dumps(json_desc, indent=2) ) - def default_io(self, obj): - return AlgorithmAlgorithmInterface.objects.get( - algorithm=obj, is_default=True - ) - def get_queryset(self, request): queryset = super().get_queryset(request) queryset = queryset.annotate( @@ -244,16 +243,6 @@ class AlgorithmModelAdmin(GuardedModelAdmin): readonly_fields = ("creator", "algorithm", "sha256", "size_in_storage") -class AlgorithmInterfaceAdminForm(AlgorithmInterfaceBaseForm): - def clean(self): - cleaned_data = super().clean() - if cleaned_data["existing_io"]: - raise ValidationError( - "An AlgorithmIO with the same combination of inputs and outputs already exists." - ) - return cleaned_data - - @admin.register(AlgorithmInterface) class AlgorithmInterfaceAdmin(GuardedModelAdmin): list_display = ( @@ -262,10 +251,10 @@ class AlgorithmInterfaceAdmin(GuardedModelAdmin): "algorithm_outputs", ) search_fields = ( + "pk", "inputs__slug", "outputs__slug", ) - form = AlgorithmInterfaceAdminForm def algorithm_inputs(self, obj): return oxford_comma(obj.inputs.all()) @@ -279,6 +268,9 @@ def has_change_permission(self, request, obj=None): def has_delete_permission(self, request, obj=None): return False + def has_add_permission(self, request, obj=None): + return False + @admin.register(AlgorithmAlgorithmInterface) class AlgorithmAlgorithmInterfaceAdmin(GuardedModelAdmin): diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index 8c3b7935b2..8fd8070ec8 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -26,11 +26,13 @@ from django.db.models import Count, Exists, Max, OuterRef, Q from django.db.transaction import on_commit from django.forms import ( + BooleanField, CharField, Form, HiddenInput, ModelChoiceField, ModelForm, + ModelMultipleChoiceField, Select, TextInput, URLField, @@ -44,12 +46,12 @@ from grandchallenge.algorithms.models import ( Algorithm, + AlgorithmAlgorithmInterface, AlgorithmImage, AlgorithmInterface, AlgorithmModel, AlgorithmPermissionRequest, Job, - get_existing_interface_for_inputs_and_outputs, ) from grandchallenge.algorithms.serializers import ( AlgorithmImageSerializer, @@ -1345,10 +1347,35 @@ def clean_algorithm_model(self): return algorithm_model -class AlgorithmInterfaceBaseForm(SaveFormInitMixin, ModelForm): +class AlgorithmInterfaceForm(SaveFormInitMixin, ModelForm): + inputs = ModelMultipleChoiceField( + queryset=ComponentInterface.objects.exclude( + slug__in=[*NON_ALGORITHM_INTERFACES, "results-json-file"] + ), + widget=Select2MultipleWidget, + ) + outputs = ModelMultipleChoiceField( + queryset=ComponentInterface.objects.exclude( + slug__in=[*NON_ALGORITHM_INTERFACES, "results-json-file"] + ), + widget=Select2MultipleWidget, + ) + set_as_default = BooleanField(required=False) + class Meta: model = AlgorithmInterface - fields = ("inputs", "outputs") + fields = ( + "inputs", + "outputs", + "set_as_default", + ) + + def __init__(self, *args, algorithm, **kwargs): + super().__init__(*args, **kwargs) + self._algorithm = algorithm + + if not self._algorithm.default_interface: + self.fields["set_as_default"].initial = True def clean(self): cleaned_data = super().clean() @@ -1369,11 +1396,34 @@ def clean(self): f"{oxford_comma(duplicate_interfaces)} present in both" ) - cleaned_data["existing_io"] = ( - get_existing_interface_for_inputs_and_outputs( - inputs=inputs, - outputs=outputs, - ) + return cleaned_data + + def save(self): + interface = AlgorithmInterface.objects.create( + inputs=self.cleaned_data["inputs"], + outputs=self.cleaned_data["outputs"], ) - return cleaned_data + if self.cleaned_data["set_as_default"]: + AlgorithmAlgorithmInterface.objects.filter( + algorithm=self._algorithm + ).update(is_default=False) + + matched_rows = AlgorithmAlgorithmInterface.objects.filter( + algorithm=self._algorithm, interface=interface + ).update(is_default=self.cleaned_data["set_as_default"]) + + if matched_rows == 0: + self._algorithm.interfaces.add( + interface, + through_defaults={ + "is_default": self.cleaned_data["set_as_default"] + }, + ) + elif matched_rows > 1: + raise RuntimeError( + "This algorithm and interface are associated " + "with each other more than once." + ) + + return interface diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 162276b8c1..bb98a22e05 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -504,6 +504,15 @@ def default_workstation(self): return w + @cached_property + def default_interface(self): + try: + return self.interfaces.get( + algorithmalgorithminterface__is_default=True + ) + except ObjectDoesNotExist: + return None + def is_editor(self, user): return user.groups.filter(pk=self.editors_group.pk).exists() diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html index 408501fd94..5180ab6365 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html @@ -36,6 +36,10 @@ {% if "change_algorithm" in algorithm_perms %} {% if perms.algorithms.add_algorithm %} +  Interfaces + + + + + +{% endblock %} + +{% block content %} +

Algorithm Interfaces for {{ algorithm }}

+ +

+ The following interfaces are configured for your algorithm: +

+

Add new interface

+ +
+ + + + + + + + {% for object in object_list %} + + + + + + {% empty %} + + {% endfor %} + +
InputsOutputsDefault
{{ object.interface.inputs.all|oxford_comma }}{{ object.interface.outputs.all|oxford_comma }}{% if object.is_default %}{% else %}{% endif %}
This algorithm does not have any interfaces defined yet.
+
+{% endblock %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html b/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html new file mode 100644 index 0000000000..356412ffa0 --- /dev/null +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html @@ -0,0 +1,42 @@ +{% extends "base.html" %} +{% load crispy_forms_tags %} + +{% block title %} + Create Interface - {{ algorithm.title }} - {{ block.super }} +{% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content %} + +

Create An Algorithm Interface

+
+

+ Create an interface for your algorithm: define any combination of inputs and outputs, and optionally mark the interface as default for the algorithm. +

+

+ Please see the list of input options and the + list of output options + for more information and examples. +

+

+ If you cannot find suitable inputs or outputs, please contact support@grand-challenge.org. +

+ + {% crispy form %} + +{% endblock %} diff --git a/app/grandchallenge/algorithms/urls.py b/app/grandchallenge/algorithms/urls.py index 339c1c9207..72d406efbf 100644 --- a/app/grandchallenge/algorithms/urls.py +++ b/app/grandchallenge/algorithms/urls.py @@ -12,6 +12,8 @@ AlgorithmImageTemplate, AlgorithmImageUpdate, AlgorithmImportView, + AlgorithmInterfaceForAlgorithmCreate, + AlgorithmInterfacesForAlgorithmList, AlgorithmList, AlgorithmModelCreate, AlgorithmModelDetail, @@ -50,6 +52,16 @@ AlgorithmDescriptionUpdate.as_view(), name="description-update", ), + path( + "/interfaces/", + AlgorithmInterfacesForAlgorithmList.as_view(), + name="interface-list", + ), + path( + "/interfaces/create/", + AlgorithmInterfaceForAlgorithmCreate.as_view(), + name="interface-create", + ), path( "/repository/", AlgorithmRepositoryUpdate.as_view(), diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index 8005f8c76c..db9f837d6f 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -5,6 +5,7 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth.mixins import ( + AccessMixin, PermissionRequiredMixin, UserPassesTestMixin, ) @@ -46,6 +47,7 @@ AlgorithmImageForm, AlgorithmImageUpdateForm, AlgorithmImportForm, + AlgorithmInterfaceForm, AlgorithmModelForm, AlgorithmModelUpdateForm, AlgorithmModelVersionControlForm, @@ -61,7 +63,9 @@ ) from grandchallenge.algorithms.models import ( Algorithm, + AlgorithmAlgorithmInterface, AlgorithmImage, + AlgorithmInterface, AlgorithmModel, AlgorithmPermissionRequest, Job, @@ -1091,3 +1095,59 @@ def get(self, *_, **__): filename=f"{algorithm.slug}-template.zip", content_type="application/zip", ) + + +class AlgorithmInterfacePermissionMixin(AccessMixin): + @property + def algorithm(self): + return get_object_or_404(Algorithm, slug=self.kwargs["slug"]) + + def dispatch(self, request, *args, **kwargs): + if request.user.has_perm( + "change_algorithm", self.algorithm + ) and request.user.has_perm("algorithms.add_algorithm"): + return super().dispatch(request, *args, **kwargs) + else: + return self.handle_no_permission() + + +class AlgorithmInterfaceForAlgorithmCreate( + AlgorithmInterfacePermissionMixin, CreateView +): + model = AlgorithmInterface + form_class = AlgorithmInterfaceForm + success_message = "Algorithm interface successfully added" + + def get_success_url(self): + return reverse( + "algorithms:interface-list", kwargs={"slug": self.algorithm.slug} + ) + + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + context.update({"algorithm": self.algorithm}) + return context + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs.update({"algorithm": self.algorithm}) + return kwargs + + +class AlgorithmInterfacesForAlgorithmList( + AlgorithmInterfacePermissionMixin, ListView +): + model = AlgorithmAlgorithmInterface + + def get_queryset(self): + qs = super().get_queryset() + return qs.filter(algorithm=self.algorithm) + + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + context.update( + { + "algorithm": self.algorithm, + } + ) + return context diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 825caf2caa..07b9bbeca2 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -5,10 +5,10 @@ from django.core.validators import MaxValueValidator, MinValueValidator from factory.django import ImageField -from grandchallenge.algorithms.admin import AlgorithmInterfaceAdminForm from grandchallenge.algorithms.forms import ( AlgorithmForm, AlgorithmForPhaseForm, + AlgorithmInterfaceForm, AlgorithmModelForm, AlgorithmModelVersionControlForm, AlgorithmPublishForm, @@ -18,6 +18,7 @@ ) from grandchallenge.algorithms.models import ( Algorithm, + AlgorithmAlgorithmInterface, AlgorithmPermissionRequest, Job, ) @@ -1297,27 +1298,12 @@ def test_algorithm_for_phase_form_memory_limited(): assert max_validator.limit_value == 32 -@pytest.mark.django_db -def test_algorithm_interface_check_for_uniqueness(): - interface = AlgorithmInterfaceFactory() - ci1, ci2 = ComponentInterfaceFactory.create_batch(2) - interface.inputs.set([ci1]) - interface.outputs.set([ci2]) - - form = AlgorithmInterfaceAdminForm( - data={"inputs": [ci1], "outputs": [ci2]} - ) - assert form.is_valid() is False - assert ( - "An AlgorithmIO with the same combination of inputs and outputs already exists." - in str(form.errors) - ) - - @pytest.mark.django_db def test_algorithm_interface_disjoint_interfaces(): ci = ComponentInterfaceFactory() - form = AlgorithmInterfaceAdminForm(data={"inputs": [ci], "outputs": [ci]}) + form = AlgorithmInterfaceForm( + algorithm=AlgorithmFactory(), data={"inputs": [ci], "outputs": [ci]} + ) assert form.is_valid() is False assert "The sets of Inputs and Outputs must be unique" in str(form.errors) @@ -1347,3 +1333,131 @@ def test_algorithm_for_phase_form_memory(): ) assert max_validator is not None assert max_validator.limit_value == 42 + + +class TestAlgorithmInterfaceForm: + @pytest.mark.django_db + def test_set_as_default_initial_value(self): + alg = AlgorithmFactory() + + form = AlgorithmInterfaceForm( + algorithm=alg, + ) + assert form.fields["set_as_default"].initial + + alg.interfaces.add( + AlgorithmInterfaceFactory(), through_defaults={"is_default": True} + ) + + del alg.default_interface + + form = AlgorithmInterfaceForm( + algorithm=alg, + ) + assert not form.fields["set_as_default"].initial + + @pytest.mark.django_db + def test_existing_io_is_reused(self): + inp = ComponentInterfaceFactory() + out = ComponentInterfaceFactory() + io = AlgorithmInterfaceFactory() + io.inputs.set([inp]) + io.outputs.set([out]) + + alg = AlgorithmFactory() + + form = AlgorithmInterfaceForm( + algorithm=alg, + data={ + "inputs": [inp.pk], + "outputs": [out.pk], + "set_as_default": False, + }, + ) + assert form.is_valid() + new_io = form.save() + + assert io == new_io + + @pytest.mark.django_db + def test_new_default_interface_updates_related_interfaces(self): + ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) + alg = AlgorithmFactory() + io = AlgorithmInterfaceFactory() + alg.interfaces.add(io, through_defaults={"is_default": True}) + + old_iot = AlgorithmAlgorithmInterface.objects.get() + + form = AlgorithmInterfaceForm( + algorithm=alg, + data={ + "inputs": [ci_1.pk], + "outputs": [ci_2.pk], + "set_as_default": True, + }, + ) + form.is_valid() + new_io = form.save() + new_iot = AlgorithmAlgorithmInterface.objects.get(interface=new_io) + old_iot.refresh_from_db() + + assert new_io != io + assert new_iot.is_default + assert not old_iot.is_default + + @pytest.mark.django_db + def test_default_interface_for_algorithm_not_updated_when_adding_new_non_default_interface( + self, + ): + alg = AlgorithmFactory() + io = AlgorithmInterfaceFactory() + alg.interfaces.add(io, through_defaults={"is_default": True}) + old_iot = AlgorithmAlgorithmInterface.objects.get() + + ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) + + form = AlgorithmInterfaceForm( + algorithm=alg, + data={ + "inputs": [ci_1.pk], + "outputs": [ci_2.pk], + "set_as_default": False, + }, + ) + form.is_valid() + new_io = form.save() + old_iot.refresh_from_db() + new_iot = AlgorithmAlgorithmInterface.objects.get(interface=new_io) + + assert new_io != io + assert not new_iot.is_default + assert old_iot.is_default + + @pytest.mark.django_db + def test_is_default_is_updated_when_adding_an_already_existing_interface( + self, + ): + alg = AlgorithmFactory() + ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) + + io = AlgorithmInterfaceFactory() + io.inputs.set([ci_1]) + io.outputs.set([ci_2]) + + alg.interfaces.add(io, through_defaults={"is_default": True}) + old_iot = AlgorithmAlgorithmInterface.objects.get() + + form = AlgorithmInterfaceForm( + algorithm=alg, + data={ + "inputs": [ci_1.pk], + "outputs": [ci_2.pk], + "set_as_default": False, + }, + ) + form.is_valid() + new_io = form.save() + old_iot.refresh_from_db() + + assert new_io == io + assert not old_iot.is_default diff --git a/app/tests/algorithms_tests/test_models.py b/app/tests/algorithms_tests/test_models.py index 2de0cdeac0..6c7d8028be 100644 --- a/app/tests/algorithms_tests/test_models.py +++ b/app/tests/algorithms_tests/test_models.py @@ -1497,3 +1497,16 @@ def test_algorithminterface_create(): io = AlgorithmInterface.objects.create(inputs=inputs, outputs=outputs) assert list(io.inputs.all()) == inputs assert list(io.outputs.all()) == outputs + + +@pytest.mark.django_db +def test_has_default_interface(): + alg1, alg2, alg3 = AlgorithmFactory.create_batch(3) + io1, io2 = AlgorithmInterfaceFactory.create_batch(2) + + alg1.interfaces.add(io1, through_defaults={"is_default": True}) + alg2.interfaces.add(io2) + + assert alg1.default_interface == io1 + assert not alg2.default_interface + assert not alg3.default_interface diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 28073abbfa..822110fbec 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -15,7 +15,13 @@ from guardian.shortcuts import assign_perm, remove_perm from requests import put -from grandchallenge.algorithms.models import Algorithm, AlgorithmImage, Job +from grandchallenge.algorithms.models import ( + Algorithm, + AlgorithmAlgorithmInterface, + AlgorithmImage, + AlgorithmInterface, + Job, +) from grandchallenge.algorithms.views import JobsList from grandchallenge.components.models import ( ComponentInterface, @@ -29,6 +35,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, AlgorithmModelFactory, AlgorithmPermissionRequestFactory, @@ -2138,3 +2145,101 @@ def test_algorithm_template_download(client): assert ( file_name in zip_file.namelist() ), f"{file_name} is in the ZIP file" + + +@pytest.mark.parametrize( + "viewname", ["algorithms:interface-list", "algorithms:interface-create"] +) +@pytest.mark.django_db +def test_algorithm_interface_view_permission(client, viewname): + ( + user_with_alg_add_perm, + user_without_alg_add_perm, + algorithm_editor_with_alg_add, + algorithm_editor_without_alg_add, + ) = UserFactory.create_batch(4) + assign_perm("algorithms.add_algorithm", user_with_alg_add_perm) + assign_perm("algorithms.add_algorithm", algorithm_editor_with_alg_add) + + alg = AlgorithmFactory() + alg.add_editor(algorithm_editor_with_alg_add) + alg.add_editor(algorithm_editor_without_alg_add) + + for user, status in [ + [user_with_alg_add_perm, 403], + [user_without_alg_add_perm, 403], + [algorithm_editor_with_alg_add, 200], + [algorithm_editor_without_alg_add, 403], + ]: + response = get_view_for_user( + viewname=viewname, + client=client, + reverse_kwargs={"slug": alg.slug}, + user=user, + ) + assert response.status_code == status + + +@pytest.mark.django_db +def test_algorithm_interface_create(client): + user = UserFactory() + assign_perm("algorithms.add_algorithm", user) + alg = AlgorithmFactory() + alg.add_editor(user) + + ci_1 = ComponentInterfaceFactory() + ci_2 = ComponentInterfaceFactory() + + response = get_view_for_user( + viewname="algorithms:interface-create", + client=client, + method=client.post, + reverse_kwargs={"slug": alg.slug}, + data={ + "inputs": [ci_1.pk], + "outputs": [ci_2.pk], + "set_as_default": True, + }, + user=user, + ) + assert response.status_code == 302 + + assert AlgorithmInterface.objects.count() == 1 + io = AlgorithmInterface.objects.get() + assert io.inputs.get() == ci_1 + assert io.outputs.get() == ci_2 + + assert AlgorithmAlgorithmInterface.objects.count() == 1 + io_through = AlgorithmAlgorithmInterface.objects.get() + assert io_through.algorithm == alg + assert io_through.interface == io + assert io_through.is_default + + +@pytest.mark.django_db +def test_algorithm_interfaces_list_queryset(client): + user = UserFactory() + assign_perm("algorithms.add_algorithm", user) + alg, alg2 = AlgorithmFactory.create_batch(2) + alg.add_editor(user) + VerificationFactory(user=user, is_verified=True) + + io1, io2, io3, io4 = AlgorithmInterfaceFactory.create_batch(4) + + alg.interfaces.set([io1, io2]) + alg2.interfaces.set([io3, io4]) + + iots = AlgorithmAlgorithmInterface.objects.order_by("id").all() + + response = get_view_for_user( + viewname="algorithms:interface-list", + client=client, + reverse_kwargs={"slug": alg.slug}, + user=user, + ) + assert response.status_code == 200 + assert response.context["object_list"].count() == 2 + assert iots[0] in response.context["object_list"] + assert iots[1] in response.context["object_list"] + assert iots[2] not in response.context["object_list"] + assert iots[3] not in response.context["object_list"] From 6e2da3d8f27d30aac5c6729aa6a01131d5d156fa Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Wed, 18 Dec 2024 15:30:11 +0100 Subject: [PATCH 03/18] Require default interface for algorithm (#3759) Adds a check to both the form and the model that ensures that an algorithm has a default interface. We're not using the through model directly to create an interface for an algorithm through the UI, hence the check in 2 places. --- app/grandchallenge/algorithms/forms.py | 8 ++++++++ app/grandchallenge/algorithms/models.py | 6 ++++++ app/tests/algorithms_tests/test_forms.py | 26 +++++++++++++++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index bc37bc1503..070a0cdd9c 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -1377,6 +1377,14 @@ def __init__(self, *args, algorithm, **kwargs): if not self._algorithm.default_interface: self.fields["set_as_default"].initial = True + def clean_set_as_default(self): + set_as_default = self.cleaned_data["set_as_default"] + + if not set_as_default and not self._algorithm.default_interface: + raise ValidationError("Your algorithm needs a default interface.") + + return set_as_default + def clean(self): cleaned_data = super().clean() diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index bb98a22e05..56c2d95595 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -634,6 +634,12 @@ class Meta: def __str__(self): return str(self.interface) + def clean(self): + super().clean() + + if not self.is_default and not self.algorithm.default_interface: + raise ValidationError("This algorithm needs a default interface.") + class AlgorithmUserObjectPermission(UserObjectPermissionBase): content_object = models.ForeignKey(Algorithm, on_delete=models.CASCADE) diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 580ed73e33..fa023fcd24 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -1364,6 +1364,30 @@ def test_algorithm_interface_disjoint_interfaces(): assert "The sets of Inputs and Outputs must be unique" in str(form.errors) +@pytest.mark.django_db +def test_algorithm_interface_default_interface_required(): + ci1, ci2 = ComponentInterfaceFactory.create_batch(2) + alg = AlgorithmFactory() + form = AlgorithmInterfaceForm( + algorithm=alg, + data={"inputs": [ci1], "outputs": [ci2], "set_as_default": False}, + ) + assert form.is_valid() is False + assert "Your algorithm needs a default interface" in str( + form.errors["set_as_default"] + ) + + alg.interfaces.add( + AlgorithmInterfaceFactory(), through_defaults={"is_default": True} + ) + del alg.default_interface + form = AlgorithmInterfaceForm( + algorithm=alg, + data={"inputs": [ci1], "outputs": [ci2], "set_as_default": False}, + ) + assert form.is_valid() + + def test_algorithm_for_phase_form_memory(): form = AlgorithmForPhaseForm( workstation_config=WorkstationConfigFactory.build(), @@ -1427,7 +1451,7 @@ def test_existing_io_is_reused(self): data={ "inputs": [inp.pk], "outputs": [out.pk], - "set_as_default": False, + "set_as_default": True, }, ) assert form.is_valid() From f9e5d5ad4c601e47638d8b1d3339056ab6f7e8fb Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Thu, 16 Jan 2025 16:12:59 +0100 Subject: [PATCH 04/18] Enable interface selection for algorithm job (#3753) This enables selecting an interface when trying out an algorithm. Interface selection happens in a separate form and view, but is unnecessary if an algorithm has only 1 interface. ![image](https://github.com/user-attachments/assets/ee3c3f99-1b71-4e26-9db3-223d8625d5f6) Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 --- app/grandchallenge/algorithms/admin.py | 3 +- app/grandchallenge/algorithms/forms.py | 57 ++- ...rface_algorithminterfaceoutput_and_more.py | 10 + .../0065_create_interfaces_for_jobs.py | 27 ++ app/grandchallenge/algorithms/models.py | 9 +- .../algorithms/algorithm_detail.html | 6 +- .../templates/algorithms/job_detail.html | 2 +- .../templates/algorithms/job_form_create.html | 99 ++--- .../templates/algorithms/job_list.html | 2 +- .../algorithms/job_progress_detail.html | 2 +- app/grandchallenge/algorithms/urls.py | 12 +- app/grandchallenge/algorithms/views.py | 79 +++- app/tests/algorithms_tests/test_api.py | 1 + app/tests/algorithms_tests/test_forms.py | 91 +++-- app/tests/algorithms_tests/test_models.py | 67 +++- .../algorithms_tests/test_permissions.py | 12 +- .../algorithms_tests/test_serializers.py | 4 + app/tests/algorithms_tests/test_tasks.py | 132 +++++-- app/tests/algorithms_tests/test_views.py | 346 +++++++++--------- app/tests/conftest.py | 6 +- 20 files changed, 642 insertions(+), 325 deletions(-) create mode 100644 app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index 966a4af6f1..4b1de23ec4 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -61,7 +61,6 @@ class AlgorithmAdmin(GuardedModelAdmin): "title", "created", "public", - "default_interface", "time_limit", "job_requires_gpu_type", "job_requires_memory_gb", @@ -216,6 +215,7 @@ class JobAdmin(GuardedModelAdmin): "task_on_success", "task_on_failure", "runtime_metrics", + "algorithm_interface", ) search_fields = ( "creator__username", @@ -245,6 +245,7 @@ class AlgorithmModelAdmin(GuardedModelAdmin): @admin.register(AlgorithmInterface) class AlgorithmInterfaceAdmin(GuardedModelAdmin): + readonly_fields = ("algorithm_inputs", "algorithm_outputs") list_display = ( "pk", "algorithm_inputs", diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index 070a0cdd9c..8ad5fc4ee3 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -37,7 +37,11 @@ TextInput, URLField, ) -from django.forms.widgets import MultipleHiddenInput, PasswordInput +from django.forms.widgets import ( + MultipleHiddenInput, + PasswordInput, + RadioSelect, +) from django.urls import Resolver404, resolve from django.utils.functional import cached_property from django.utils.html import format_html @@ -116,10 +120,18 @@ class JobCreateForm(SaveFormInitMixin, Form): creator = ModelChoiceField( queryset=None, disabled=True, required=False, widget=HiddenInput ) + algorithm_interface = ModelChoiceField( + queryset=None, + disabled=True, + required=True, + widget=HiddenInput, + ) - def __init__(self, *args, algorithm, user, **kwargs): + def __init__(self, *args, algorithm, user, interface, **kwargs): super().__init__(*args, **kwargs) + self._algorithm = algorithm + self.helper = FormHelper() self._user = user @@ -128,7 +140,11 @@ def __init__(self, *args, algorithm, user, **kwargs): ) self.fields["creator"].initial = self._user - self._algorithm = algorithm + self.fields["algorithm_interface"].queryset = ( + self._algorithm.interfaces.all() + ) + self.fields["algorithm_interface"].initial = interface + self._algorithm_image = self._algorithm.active_image active_model = self._algorithm.active_model @@ -145,7 +161,7 @@ def __init__(self, *args, algorithm, user, **kwargs): ) self.fields["algorithm_model"].initial = active_model - for inp in self._algorithm.inputs.all(): + for inp in interface.inputs.all(): prefixed_interface_slug = ( f"{INTERFACE_FORM_FIELD_PREFIX}{inp.slug}" ) @@ -1435,3 +1451,36 @@ def save(self): ) return interface + + +class JobInterfaceSelectForm(SaveFormInitMixin, Form): + algorithm_interface = ModelChoiceField( + queryset=None, + required=True, + help_text="Select an input-output combination to use for this job.", + widget=RadioSelect, + ) + + def __init__(self, *args, algorithm, **kwargs): + super().__init__(*args, **kwargs) + + self._algorithm = algorithm + + self.fields["algorithm_interface"].queryset = ( + self._algorithm.interfaces.all() + ) + self.fields["algorithm_interface"].initial = ( + self._algorithm.default_interface + ) + self.fields["algorithm_interface"].widget.choices = { + ( + interface.pk, + format_html( + "
Inputs: {inputs}
" + "
Outputs: {outputs}
", + inputs=oxford_comma(interface.inputs.all()), + outputs=oxford_comma(interface.outputs.all()), + ), + ) + for interface in self._algorithm.interfaces.all() + } diff --git a/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py b/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py index 9a0997b11b..ffdcabea83 100644 --- a/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py +++ b/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py @@ -159,4 +159,14 @@ class Migration(migrations.Migration): name="unique_algorithm_interface_combination", ), ), + migrations.AddField( + model_name="job", + name="algorithm_interface", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="algorithms.algorithminterface", + ), + ), ] diff --git a/app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py b/app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py new file mode 100644 index 0000000000..f1e116af3d --- /dev/null +++ b/app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py @@ -0,0 +1,27 @@ +from django.db import migrations + + +def add_algorithm_interfaces_to_jobs(apps, _schema_editor): + Algorithm = apps.get_model("algorithms", "Algorithm") # noqa: N806 + Job = apps.get_model("algorithms", "Job") # noqa: N806 + + for algorithm in Algorithm.objects.prefetch_related("interfaces").all(): + default_interface = algorithm.interfaces.get( + algorithmalgorithminterface__is_default=True + ) + Job.objects.filter(algorithm_image__algorithm=algorithm).update( + algorithm_interface=default_interface + ) + + +class Migration(migrations.Migration): + dependencies = [ + ( + "algorithms", + "0064_create_algorithm_interfaces", + ), + ] + + operations = [ + migrations.RunPython(add_algorithm_interfaces_to_jobs, elidable=True), + ] diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 56c2d95595..6e5550cbc7 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -973,7 +973,7 @@ def get_jobs_with_same_inputs( unique_kwargs = { "algorithm_image": algorithm_image, } - input_interface_count = algorithm_image.algorithm.inputs.count() + input_interface_count = len(inputs) if algorithm_model: unique_kwargs["algorithm_model"] = algorithm_model @@ -1078,6 +1078,9 @@ class Job(CIVForObjectMixin, ComponentJob): algorithm_model = models.ForeignKey( AlgorithmModel, on_delete=models.PROTECT, null=True, blank=True ) + algorithm_interface = models.ForeignKey( + AlgorithmInterface, on_delete=models.PROTECT, null=True, blank=True + ) creator = models.ForeignKey( settings.AUTH_USER_MODEL, null=True, on_delete=models.SET_NULL ) @@ -1124,14 +1127,14 @@ def container(self): @property def output_interfaces(self): - return self.algorithm_image.algorithm.outputs + return self.algorithm_interface.outputs.all() @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()} + } == {*self.algorithm_interface.inputs.all()} @cached_property def rendered_result_text(self) -> str: diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html index 5180ab6365..31c63c5e87 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html @@ -79,7 +79,7 @@ {% if "execute_algorithm" in algorithm_perms and object.active_image %} + href="{% url 'algorithms:job-interface-select' slug=object.slug %}">  Try-out Algorithm {% endif %} @@ -118,7 +118,7 @@ {% if object.public and not object.public_test_case %} {% endif %} {% endif %} @@ -618,7 +618,7 @@
Publish algorithm diff --git a/app/grandchallenge/algorithms/templates/algorithms/job_detail.html b/app/grandchallenge/algorithms/templates/algorithms/job_detail.html index 2eb9edf587..f96b87de04 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/job_detail.html +++ b/app/grandchallenge/algorithms/templates/algorithms/job_detail.html @@ -55,7 +55,7 @@ {% if object.status == object.SUCCESS and perms.reader_studies.add_readerstudy %} + href="{% url "algorithms:display-set-from-job-create" slug=object.algorithm_image.algorithm.slug interface_pk=object.pk %}">  Edit in Reader Study {% endif %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/job_form_create.html b/app/grandchallenge/algorithms/templates/algorithms/job_form_create.html index 1914cf8adb..47527a4902 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/job_form_create.html +++ b/app/grandchallenge/algorithms/templates/algorithms/job_form_create.html @@ -36,59 +36,66 @@ {% block content %}

Try-out Algorithm

- {{ algorithm.job_create_page_markdown|md2html }} - {% get_obj_perms request.user for algorithm as "algorithm_perms" %} - {% if not algorithm.active_image %} -

- This algorithm is not ready to be used. - {% if 'change_algorithm' in algorithm_perms %} - Please upload a valid container image for this algorithm. - {% endif %} -

- {% elif form.jobs_limit < 1 %} -

- You have run out of credits to try this algorithm. - You can request more credits by sending an e-mail to - - support@grand-challenge.org. -

+ {% if not algorithm.interfaces.all and 'change_algorithm' in algorithm_perms %} +

Your algorithm does not have any interfaces yet. You need to define at least one interface (i.e. input - output combination) before you can try it out.

+

To define an interface, navigate here.

+ {% elif not algorithm.interfaces.all %} +

This algorithm is not fully configured yet and hence cannot be used.

{% else %} -

- Select the data that you would like to run the algorithm on. -

-

- {% if 'change_algorithm' in algorithm_perms %} - As an editor for this algorithm you can test and debug your algorithm in total {{ editors_job_limit }} times per unique algorithm image. - You share these credits with all other editors of this algorithm. - Once you have reached the limit, any extra jobs will be deducted from your personal algorithm credits, - of which you get {{ request.user.user_credit.credits }} per month. - {% else %} - You receive {{ request.user.user_credit.credits }} credits per month. - {% endif %} - Using this algorithm requires {{ algorithm.credits_per_job }} - credit{{ algorithm.credits_per_job|pluralize }} per job. - You can currently create up to {{ form.jobs_limit }} job{{ form.jobs_limit|pluralize }} for this algorithm. -

+ {{ algorithm.job_create_page_markdown|md2html }} + + {% if not algorithm.active_image %} +

+ This algorithm is not ready to be used. + {% if 'change_algorithm' in algorithm_perms %} + Please upload a valid container image for this algorithm. + {% endif %} +

+ {% elif form.jobs_limit < 1 %} +

+ You have run out of credits to try this algorithm. + You can request more credits by sending an e-mail to + + support@grand-challenge.org. +

+ {% else %} +

+ Select the data that you would like to run the algorithm on. +

+

+ {% if 'change_algorithm' in algorithm_perms %} + As an editor for this algorithm you can test and debug your algorithm in total {{ editors_job_limit }} times per unique algorithm image. + You share these credits with all other editors of this algorithm. + Once you have reached the limit, any extra jobs will be deducted from your personal algorithm credits, + of which you get {{ request.user.user_credit.credits }} per month. + {% else %} + You receive {{ request.user.user_credit.credits }} credits per month. + {% endif %} + Using this algorithm requires {{ algorithm.credits_per_job }} + credit{{ algorithm.credits_per_job|pluralize }} per job. + You can currently create up to {{ form.jobs_limit }} job{{ form.jobs_limit|pluralize }} for this algorithm. +

-
- {% csrf_token %} - {{ form|crispy }} - -
+
+ {% csrf_token %} + {{ form|crispy }} + +
-

- By running this algorithm you agree to the - General - Terms of Service{% if algorithm.additional_terms_markdown %}, - as well as this algorithm's specific Terms of Service: - {% else %}. - {% endif %} -

+

+ By running this algorithm you agree to the + General + Terms of Service{% if algorithm.additional_terms_markdown %}, + as well as this algorithm's specific Terms of Service: + {% else %}. + {% endif %} +

- {{ algorithm.additional_terms_markdown|md2html }} + {{ algorithm.additional_terms_markdown|md2html }} + {% endif %} {% endif %} {% endblock %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/job_list.html b/app/grandchallenge/algorithms/templates/algorithms/job_list.html index 5f216ec98c..ab3d5657b2 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/job_list.html +++ b/app/grandchallenge/algorithms/templates/algorithms/job_list.html @@ -23,7 +23,7 @@

Results for {{ algorithm.title }}

{% if "execute_algorithm" in algorithm_perms and algorithm.active_image %}

+ href="{% url 'algorithms:job-interface-select' slug=algorithm.slug %}">  Try-out Algorithm

diff --git a/app/grandchallenge/algorithms/templates/algorithms/job_progress_detail.html b/app/grandchallenge/algorithms/templates/algorithms/job_progress_detail.html index 2d8f2d8d28..39e4351db5 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/job_progress_detail.html +++ b/app/grandchallenge/algorithms/templates/algorithms/job_progress_detail.html @@ -35,7 +35,7 @@
Creating inputs
- diff --git a/app/grandchallenge/algorithms/urls.py b/app/grandchallenge/algorithms/urls.py index 72d406efbf..7b424a6e6e 100644 --- a/app/grandchallenge/algorithms/urls.py +++ b/app/grandchallenge/algorithms/urls.py @@ -30,6 +30,7 @@ EditorsUpdate, JobCreate, JobDetail, + JobInterfaceSelect, JobProgressDetail, JobsList, JobStatusDetail, @@ -133,7 +134,16 @@ name="model-update", ), path("/jobs/", JobsList.as_view(), name="job-list"), - path("/jobs/create/", JobCreate.as_view(), name="job-create"), + path( + "/jobs/interface-select/", + JobInterfaceSelect.as_view(), + name="job-interface-select", + ), + path( + "//jobs/create/", + JobCreate.as_view(), + name="job-create", + ), path("/jobs//", JobDetail.as_view(), name="job-detail"), path( "/jobs//status/", diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index db9f837d6f..6d947c0663 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -58,6 +58,7 @@ ImageActivateForm, JobCreateForm, JobForm, + JobInterfaceSelectForm, UsersForm, ViewersForm, ) @@ -478,28 +479,84 @@ def get_success_url(self): return self.algorithm.get_absolute_url() +class JobCreatePermissionMixin( + LoginRequiredMixin, VerificationRequiredMixin, AccessMixin +): + @cached_property + def algorithm(self) -> Algorithm: + return get_object_or_404(Algorithm, slug=self.kwargs["slug"]) + + def dispatch(self, request, *args, **kwargs): + if not request.user.has_perm( + "algorithms.execute_algorithm", self.algorithm + ): + return self.handle_no_permission() + return super().dispatch(request, *args, **kwargs) + + +class JobInterfaceSelect( + JobCreatePermissionMixin, + FormView, +): + form_class = JobInterfaceSelectForm + template_name = "algorithms/job_form_create.html" + selected_interface = None + + def get(self, request, *args, **kwargs): + if self.algorithm.interfaces.count() == 1: + self.selected_interface = self.algorithm.default_interface + return HttpResponseRedirect(self.get_success_url()) + else: + return super().get(request, *args, **kwargs) + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs.update({"algorithm": self.algorithm}) + return kwargs + + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + context.update( + { + "algorithm": self.algorithm, + "editors_job_limit": settings.ALGORITHM_IMAGES_COMPLIMENTARY_EDITOR_JOBS, + } + ) + return context + + def form_valid(self, form): + self.selected_interface = form.cleaned_data["algorithm_interface"] + return super().form_valid(form) + + def get_success_url(self): + return reverse( + "algorithms:job-create", + kwargs={ + "slug": self.algorithm.slug, + "interface_pk": self.selected_interface.pk, + }, + ) + + class JobCreate( - LoginRequiredMixin, - ObjectPermissionRequiredMixin, - VerificationRequiredMixin, + JobCreatePermissionMixin, UserFormKwargsMixin, FormView, ): form_class = JobCreateForm template_name = "algorithms/job_form_create.html" - permission_required = "algorithms.execute_algorithm" - raise_exception = True @cached_property - def algorithm(self) -> Algorithm: - return get_object_or_404(Algorithm, slug=self.kwargs["slug"]) - - def get_permission_object(self): - return self.algorithm + def interface(self): + return get_object_or_404( + AlgorithmInterface, pk=self.kwargs["interface_pk"] + ) def get_form_kwargs(self): kwargs = super().get_form_kwargs() - kwargs.update({"algorithm": self.algorithm}) + kwargs.update( + {"algorithm": self.algorithm, "interface": self.interface} + ) return kwargs def get_context_data(self, *args, **kwargs): diff --git a/app/tests/algorithms_tests/test_api.py b/app/tests/algorithms_tests/test_api.py index 7478f75081..2eb04c248c 100644 --- a/app/tests/algorithms_tests/test_api.py +++ b/app/tests/algorithms_tests/test_api.py @@ -92,6 +92,7 @@ def test_job_list_view_num_queries( assert len(response.json()["results"]) == num_jobs +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db class TestJobCreationThroughAPI: diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index fa023fcd24..5ff451df71 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -321,7 +321,10 @@ def test_create_job_input_fields( response = get_view_for_user( viewname="algorithms:job-create", client=client, - reverse_kwargs={"slug": alg.slug}, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": alg.default_interface.pk, + }, follow=True, user=creator, ) @@ -353,7 +356,10 @@ def test_create_job_json_input_field_validation( response = get_view_for_user( viewname="algorithms:job-create", client=client, - reverse_kwargs={"slug": alg.slug}, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": alg.default_interface.pk, + }, method=client.post, follow=True, user=creator, @@ -384,7 +390,10 @@ def test_create_job_simple_input_field_validation( response = get_view_for_user( viewname="algorithms:job-create", client=client, - reverse_kwargs={"slug": alg.slug}, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": alg.default_interface.pk, + }, method=client.post, follow=True, user=creator, @@ -400,7 +409,11 @@ def create_algorithm_with_input(slug): VerificationFactory(user=creator, is_verified=True) alg = AlgorithmFactory() alg.add_editor(user=creator) - alg.inputs.set([ComponentInterface.objects.get(slug=slug)]) + interface = AlgorithmInterfaceFactory( + inputs=[ComponentInterface.objects.get(slug=slug)], + outputs=[ComponentInterfaceFactory()], + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) AlgorithmImageFactory( algorithm=alg, is_manifest_valid=True, @@ -500,13 +513,34 @@ def test_only_publish_successful_jobs(): @pytest.mark.django_db class TestJobCreateLimits: + + def create_form(self, algorithm, user, algorithm_image=None): + ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) + interface = AlgorithmInterfaceFactory(inputs=[ci]) + algorithm.interfaces.add(interface) + + algorithm_image_kwargs = {} + if algorithm_image: + algorithm_image_kwargs = { + "algorithm_image": str(algorithm_image.pk) + } + + return JobCreateForm( + algorithm=algorithm, + user=user, + interface=interface, + data={ + **algorithm_image_kwargs, + **get_interface_form_data(interface_slug=ci.slug, data="Foo"), + }, + ) + def test_form_invalid_without_enough_credits(self, settings): algorithm = AlgorithmFactory( minimum_credits_per_job=( settings.ALGORITHMS_GENERAL_CREDITS_PER_MONTH_PER_USER + 1 ), ) - algorithm.inputs.clear() user = UserFactory() AlgorithmImageFactory( algorithm=algorithm, @@ -514,13 +548,11 @@ def test_form_invalid_without_enough_credits(self, settings): is_in_registry=True, is_desired_version=True, ) - - form = JobCreateForm(algorithm=algorithm, user=user, data={}) - + form = self.create_form(algorithm=algorithm, user=user) assert not form.is_valid() - assert form.errors == { - "__all__": ["You have run out of algorithm credits"], - } + assert "You have run out of algorithm credits" in str( + form.errors["__all__"] + ) def test_form_valid_for_editor(self, settings): algorithm = AlgorithmFactory( @@ -528,7 +560,6 @@ def test_form_valid_for_editor(self, settings): settings.ALGORITHMS_GENERAL_CREDITS_PER_MONTH_PER_USER + 1 ), ) - algorithm.inputs.clear() algorithm_image = AlgorithmImageFactory( algorithm=algorithm, is_manifest_valid=True, @@ -539,17 +570,15 @@ def test_form_valid_for_editor(self, settings): algorithm.add_editor(user=user) - form = JobCreateForm( + form = self.create_form( algorithm=algorithm, user=user, - data={"algorithm_image": str(algorithm_image.pk)}, + algorithm_image=algorithm_image, ) - assert form.is_valid() def test_form_valid_with_credits(self): algorithm = AlgorithmFactory(minimum_credits_per_job=1) - algorithm.inputs.clear() algorithm_image = AlgorithmImageFactory( algorithm=algorithm, is_manifest_valid=True, @@ -558,12 +587,11 @@ def test_form_valid_with_credits(self): ) user = UserFactory() - form = JobCreateForm( + form = self.create_form( algorithm=algorithm, user=user, - data={"algorithm_image": str(algorithm_image.pk)}, + algorithm_image=algorithm_image, ) - assert form.is_valid() @@ -710,7 +738,12 @@ def test_creator_queryset( ): algorithm = algorithm_with_image_and_model_and_two_inputs.algorithm editor = algorithm.editors_group.user_set.first() - form = JobCreateForm(algorithm=algorithm, user=editor, data={}) + form = JobCreateForm( + algorithm=algorithm, + user=editor, + interface=algorithm.default_interface, + data={}, + ) assert list(form.fields["creator"].queryset.all()) == [editor] assert form.fields["creator"].initial == editor @@ -726,7 +759,12 @@ def test_algorithm_image_queryset( is_manifest_valid=True, is_in_registry=True, ) - form = JobCreateForm(algorithm=algorithm, user=editor, data={}) + form = JobCreateForm( + algorithm=algorithm, + user=editor, + interface=algorithm.default_interface, + data={}, + ) ai_qs = form.fields["algorithm_image"].queryset.all() assert algorithm.active_image in ai_qs assert inactive_image not in ai_qs @@ -745,12 +783,14 @@ def test_cannot_create_job_with_same_inputs_twice( algorithm_model=algorithm.active_model, status=Job.SUCCESS, time_limit=123, + algorithm_interface=algorithm.default_interface, ) job.inputs.set(civs) form = JobCreateForm( algorithm=algorithm, user=editor, + interface=algorithm.default_interface, data={ "algorithm_image": algorithm.active_image, "algorithm_model": algorithm.active_model, @@ -775,13 +815,18 @@ def test_all_inputs_required_on_job_creation(algorithm_with_multiple_inputs): kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=True, ) - algorithm_with_multiple_inputs.algorithm.inputs.add( - ci_json_in_db_without_schema + interface = AlgorithmInterfaceFactory( + inputs=[ci_json_in_db_without_schema], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) form = JobCreateForm( algorithm=algorithm_with_multiple_inputs.algorithm, user=algorithm_with_multiple_inputs.editor, + interface=interface, data={}, ) diff --git a/app/tests/algorithms_tests/test_models.py b/app/tests/algorithms_tests/test_models.py index 6c7d8028be..b80a048e53 100644 --- a/app/tests/algorithms_tests/test_models.py +++ b/app/tests/algorithms_tests/test_models.py @@ -656,7 +656,9 @@ def test_job_with_same_image_different_model( data = self.get_civ_data(civs=civs) j = AlgorithmJobFactory( - algorithm_image=alg.active_image, time_limit=10 + algorithm_image=alg.active_image, + time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) @@ -678,6 +680,7 @@ def test_job_with_same_model_different_image( algorithm_image=AlgorithmImageFactory(), algorithm_model=alg.active_model, time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -698,6 +701,7 @@ def test_job_with_same_model_and_image( algorithm_model=alg.active_model, algorithm_image=alg.active_image, time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -719,6 +723,7 @@ def test_job_with_different_image_and_model( algorithm_model=AlgorithmModelFactory(), algorithm_image=AlgorithmImageFactory(), time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -739,10 +744,13 @@ def test_job_with_same_image_no_model_provided( algorithm_model=alg.active_model, algorithm_image=alg.active_image, time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( - inputs=data, algorithm_image=alg.active_image, algorithm_model=None + inputs=data, + algorithm_image=alg.active_image, + algorithm_model=None, ) assert len(jobs) == 0 @@ -754,11 +762,15 @@ def test_job_with_same_image_and_without_model( data = self.get_civ_data(civs=civs) j = AlgorithmJobFactory( - algorithm_image=alg.active_image, time_limit=10 + algorithm_image=alg.active_image, + time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( - inputs=data, algorithm_image=alg.active_image, algorithm_model=None + inputs=data, + algorithm_image=alg.active_image, + algorithm_model=None, ) assert j in jobs assert len(jobs) == 1 @@ -771,7 +783,9 @@ def test_job_with_different_input( data = self.get_civ_data(civs=civs) j = AlgorithmJobFactory( - algorithm_image=alg.active_image, time_limit=10 + algorithm_image=alg.active_image, + time_limit=10, + algorithm_interface=alg.default_interface, ) j.inputs.set( [ @@ -780,7 +794,34 @@ def test_job_with_different_input( ] ) jobs = Job.objects.get_jobs_with_same_inputs( - inputs=data, algorithm_image=alg.active_image, algorithm_model=None + inputs=data, + algorithm_image=alg.active_image, + algorithm_model=None, + ) + assert len(jobs) == 0 + + def test_job_with_partially_overlapping_input( + self, algorithm_with_image_and_model_and_two_inputs + ): + alg = algorithm_with_image_and_model_and_two_inputs.algorithm + civs = algorithm_with_image_and_model_and_two_inputs.civs + data = self.get_civ_data(civs=civs) + + j = AlgorithmJobFactory( + algorithm_image=alg.active_image, + time_limit=10, + algorithm_interface=alg.default_interface, + ) + j.inputs.set( + [ + civs[0], + ComponentInterfaceValueFactory(), + ] + ) + jobs = Job.objects.get_jobs_with_same_inputs( + inputs=data, + algorithm_image=alg.active_image, + algorithm_model=None, ) assert len(jobs) == 0 @@ -982,8 +1023,15 @@ def test_inputs_complete(): ci1, ci2, ci3 = ComponentInterfaceFactory.create_batch( 3, kind=ComponentInterface.Kind.STRING ) - alg.inputs.set([ci1, ci2, ci3]) - job = AlgorithmJobFactory(algorithm_image__algorithm=alg, time_limit=10) + interface = AlgorithmInterfaceFactory( + inputs=[ci1, ci2, ci3], outputs=[ComponentInterfaceFactory()] + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) + job = AlgorithmJobFactory( + algorithm_image__algorithm=alg, + time_limit=10, + algorithm_interface=alg.default_interface, + ) civ_with_value_1 = ComponentInterfaceValueFactory( interface=ci1, value="Foo" ) @@ -1505,7 +1553,8 @@ def test_has_default_interface(): io1, io2 = AlgorithmInterfaceFactory.create_batch(2) alg1.interfaces.add(io1, through_defaults={"is_default": True}) - alg2.interfaces.add(io2) + alg1.interfaces.add(io2) + alg2.interfaces.add(io1) assert alg1.default_interface == io1 assert not alg2.default_interface diff --git a/app/tests/algorithms_tests/test_permissions.py b/app/tests/algorithms_tests/test_permissions.py index 4349bd47e8..3a0ff8951c 100644 --- a/app/tests/algorithms_tests/test_permissions.py +++ b/app/tests/algorithms_tests/test_permissions.py @@ -18,6 +18,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, ) from tests.algorithms_tests.utils import TwoAlgorithms @@ -313,7 +314,12 @@ def test_job_permissions_from_template(self, client): kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=True, ) - algorithm_image.algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], outputs=[ComponentInterfaceFactory()] + ) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) response = get_view_for_user( viewname="algorithms:job-create", @@ -321,6 +327,7 @@ def test_job_permissions_from_template(self, client): method=client.post, reverse_kwargs={ "slug": algorithm_image.algorithm.slug, + "interface_pk": algorithm_image.algorithm.default_interface.pk, }, user=user, follow=True, @@ -338,6 +345,9 @@ def test_job_permissions_from_template(self, client): algorithm_image=algorithm_image, job=job, user=user ) + @pytest.mark.xfail( + reason="Still to be addressed for optional inputs pitch" + ) def test_job_permissions_from_api(self, rf): # setup user = UserFactory() diff --git a/app/tests/algorithms_tests/test_serializers.py b/app/tests/algorithms_tests/test_serializers.py index 1f4c88a891..eb5cd14412 100644 --- a/app/tests/algorithms_tests/test_serializers.py +++ b/app/tests/algorithms_tests/test_serializers.py @@ -35,6 +35,7 @@ def test_algorithm_relations_on_job_serializer(rf): ) +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db @pytest.mark.parametrize( "title, " @@ -183,6 +184,7 @@ def test_algorithm_job_post_serializer_validations( assert len(job.inputs.all()) == 2 +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_algorithm_job_post_serializer_create( rf, settings, django_capture_on_commit_callbacks @@ -240,6 +242,7 @@ def test_algorithm_job_post_serializer_create( assert len(job.inputs.all()) == 3 +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db class TestJobCreateLimits: def test_form_invalid_without_enough_credits(self, rf, settings): @@ -380,6 +383,7 @@ def test_reformat_inputs(rf): ) +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_algorithm_post_serializer_image_and_time_limit_fixed(rf): request = rf.get("/foo") diff --git a/app/tests/algorithms_tests/test_tasks.py b/app/tests/algorithms_tests/test_tasks.py index 2e81afe8bf..3f2baa705e 100644 --- a/app/tests/algorithms_tests/test_tasks.py +++ b/app/tests/algorithms_tests/test_tasks.py @@ -5,6 +5,7 @@ from actstream.models import Follow from django.core.exceptions import ObjectDoesNotExist from django.core.files.base import ContentFile +from guardian.shortcuts import assign_perm from grandchallenge.algorithms.models import Job from grandchallenge.algorithms.tasks import ( @@ -27,6 +28,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, AlgorithmModelFactory, ) @@ -36,6 +38,7 @@ ComponentInterfaceFactory, ComponentInterfaceValueFactory, ) +from tests.conftest import get_interface_form_data from tests.factories import ( GroupFactory, ImageFactory, @@ -44,6 +47,7 @@ UserFactory, ) from tests.utils import get_view_for_user, recurse_callbacks +from tests.verification_tests.factories import VerificationFactory @pytest.mark.django_db @@ -186,7 +190,7 @@ def test_jobs_workflow(django_capture_on_commit_callbacks): @pytest.mark.flaky(reruns=3) @pytest.mark.django_db def test_algorithm( - algorithm_image, settings, django_capture_on_commit_callbacks + algorithm_image, client, settings, django_capture_on_commit_callbacks ): # Override the celery settings settings.task_eager_propagates = (True,) @@ -201,6 +205,10 @@ def test_algorithm( with open(algorithm_image, "rb") as f: ai.image.save(algorithm_image, ContentFile(f.read())) + user = UserFactory() + ai.algorithm.add_editor(user) + VerificationFactory(user=user, is_verified=True) + recurse_callbacks( callbacks=callbacks, django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, @@ -211,6 +219,7 @@ def test_algorithm( image_file = ImageFileFactory( file__from_path=Path(__file__).parent / "resources" / "input_file.tif" ) + assign_perm("cases.view_image", user, image_file.image) input_interface = ComponentInterface.objects.get( slug="generic-medical-image" @@ -219,20 +228,31 @@ def test_algorithm( slug="results-json-file" ) heatmap_interface = ComponentInterface.objects.get(slug="generic-overlay") - ai.algorithm.inputs.set([input_interface]) - ai.algorithm.outputs.set([json_result_interface, heatmap_interface]) - - civ = ComponentInterfaceValueFactory( - image=image_file.image, interface=input_interface, file=None + interface = AlgorithmInterfaceFactory( + inputs=[input_interface], + outputs=[json_result_interface, heatmap_interface], + ) + ai.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) with django_capture_on_commit_callbacks() as callbacks: - create_algorithm_jobs( - algorithm_image=ai, - civ_sets=[{civ}], - time_limit=ai.algorithm.time_limit, - requires_gpu_type=ai.algorithm.job_requires_gpu_type, - requires_memory_gb=ai.algorithm.job_requires_memory_gb, + get_view_for_user( + viewname="algorithms:job-create", + client=client, + method=client.post, + user=user, + reverse_kwargs={ + "slug": ai.algorithm.slug, + "interface_pk": interface.pk, + }, + follow=True, + data={ + **get_interface_form_data( + interface_slug=input_interface.slug, + data=image_file.image.pk, + ), + }, ) recurse_callbacks( @@ -273,22 +293,37 @@ def test_algorithm( slug="detection-json-file", kind=ComponentInterface.Kind.ANY, ) - ai.algorithm.outputs.add(detection_interface) - ai.save() + interface2 = AlgorithmInterfaceFactory( + inputs=[input_interface], + outputs=[ + json_result_interface, + heatmap_interface, + detection_interface, + ], + ) + ai.algorithm.interfaces.add(interface2) image_file = ImageFileFactory( file__from_path=Path(__file__).parent / "resources" / "input_file.tif" ) - civ = ComponentInterfaceValueFactory( - image=image_file.image, interface=input_interface, file=None - ) + assign_perm("cases.view_image", user, image_file.image) with django_capture_on_commit_callbacks() as callbacks: - create_algorithm_jobs( - algorithm_image=ai, - civ_sets=[{civ}], - time_limit=ai.algorithm.time_limit, - requires_gpu_type=ai.algorithm.job_requires_gpu_type, - requires_memory_gb=ai.algorithm.job_requires_memory_gb, + get_view_for_user( + viewname="algorithms:job-create", + client=client, + method=client.post, + user=user, + reverse_kwargs={ + "slug": ai.algorithm.slug, + "interface_pk": interface2.pk, + }, + follow=True, + data={ + **get_interface_form_data( + interface_slug=input_interface.slug, + data=image_file.image.pk, + ), + }, ) recurse_callbacks( @@ -312,7 +347,7 @@ def test_algorithm( @pytest.mark.django_db def test_algorithm_with_invalid_output( - algorithm_image, settings, django_capture_on_commit_callbacks + algorithm_image, client, settings, django_capture_on_commit_callbacks ): # Override the celery settings settings.task_eager_propagates = (True,) @@ -327,6 +362,10 @@ def test_algorithm_with_invalid_output( with open(algorithm_image, "rb") as f: ai.image.save(algorithm_image, ContentFile(f.read())) + user = UserFactory() + ai.algorithm.add_editor(user) + VerificationFactory(user=user, is_verified=True) + recurse_callbacks( callbacks=callbacks, django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, @@ -343,26 +382,37 @@ def test_algorithm_with_invalid_output( slug="detection-json-file", kind=ComponentInterface.Kind.ANY, ) - ai.algorithm.inputs.add(input_interface) - ai.algorithm.outputs.add(detection_interface) - ai.save() + interface = AlgorithmInterfaceFactory( + inputs=[input_interface], outputs=[detection_interface] + ) + ai.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) image_file = ImageFileFactory( file__from_path=Path(__file__).parent / "resources" / "input_file.tif" ) - - civ = ComponentInterfaceValueFactory( - image=image_file.image, interface=input_interface, file=None - ) + assign_perm("cases.view_image", user, image_file.image) with django_capture_on_commit_callbacks() as callbacks: - create_algorithm_jobs( - algorithm_image=ai, - civ_sets=[{civ}], - time_limit=ai.algorithm.time_limit, - requires_gpu_type=ai.algorithm.job_requires_gpu_type, - requires_memory_gb=ai.algorithm.job_requires_memory_gb, + get_view_for_user( + viewname="algorithms:job-create", + client=client, + method=client.post, + user=user, + reverse_kwargs={ + "slug": ai.algorithm.slug, + "interface_pk": interface.pk, + }, + follow=True, + data={ + **get_interface_form_data( + interface_slug=input_interface.slug, + data=image_file.image.pk, + ), + }, ) + recurse_callbacks( callbacks=callbacks, django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, @@ -422,11 +472,17 @@ def test_execute_algorithm_job_for_missing_inputs(settings): # create the job without value for the ComponentInterfaceValues ci = ComponentInterface.objects.get(slug="generic-medical-image") ComponentInterfaceValue.objects.create(interface=ci) - alg.algorithm.inputs.add(ci) + interface = AlgorithmInterfaceFactory( + inputs=[ci], outputs=[ComponentInterfaceFactory()] + ) + alg.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) job = AlgorithmJobFactory( creator=creator, algorithm_image=alg, time_limit=alg.algorithm.time_limit, + algorithm_interface=interface, ) execute_algorithm_job_for_inputs(job_pk=job.pk) diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 822110fbec..b164a27add 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -1,7 +1,6 @@ import datetime import io import json -import tempfile import zipfile from pathlib import Path from unittest.mock import patch @@ -53,7 +52,7 @@ UserUploadFactory, create_upload_from_file, ) -from tests.utils import get_view_for_user, recurse_callbacks +from tests.utils import get_view_for_user from tests.verification_tests.factories import VerificationFactory @@ -355,6 +354,13 @@ def test_permission_required_views(self, client): time_limit=ai.algorithm.time_limit, ) p = AlgorithmPermissionRequestFactory(algorithm=ai.algorithm) + interface = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + ai.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) VerificationFactory(user=u, is_verified=True) @@ -421,7 +427,10 @@ def test_permission_required_views(self, client): ), ( "job-create", - {"slug": ai.algorithm.slug}, + { + "slug": ai.algorithm.slug, + "interface_pk": ai.algorithm.default_interface.pk, + }, "execute_algorithm", ai.algorithm, None, @@ -926,158 +935,6 @@ def list_algorithm_images(self, **__): assert image_interface.kind == ComponentInterface.Kind.IMAGE -@pytest.mark.django_db -def test_create_job_with_json_file( - client, settings, algorithm_io_image, django_capture_on_commit_callbacks -): - settings.task_eager_propagates = (True,) - settings.task_always_eager = (True,) - - with django_capture_on_commit_callbacks() as callbacks: - ai = AlgorithmImageFactory(image__from_path=algorithm_io_image) - recurse_callbacks( - callbacks=callbacks, - django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, - ) - - editor = UserFactory() - VerificationFactory(user=editor, is_verified=True) - ai.algorithm.add_editor(editor) - ci = ComponentInterfaceFactory( - kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=False - ) - ai.algorithm.inputs.set([ci]) - - with tempfile.NamedTemporaryFile(mode="w+", suffix=".json") as file: - json.dump('{"Foo": "bar"}', file) - file.seek(0) - upload = create_upload_from_file( - creator=editor, file_path=Path(file.name) - ) - with django_capture_on_commit_callbacks(execute=True): - with django_capture_on_commit_callbacks(execute=True): - response = get_view_for_user( - viewname="algorithms:job-create", - client=client, - method=client.post, - reverse_kwargs={ - "slug": ai.algorithm.slug, - }, - user=editor, - follow=True, - data={ - **get_interface_form_data( - interface_slug=ci.slug, data=upload.pk - ) - }, - ) - assert response.status_code == 200 - assert ( - file.name.split("/")[-1] - in Job.objects.get().inputs.first().file.name - ) - - -@pytest.mark.django_db -def test_algorithm_job_create_with_image_input( - settings, client, algorithm_io_image, django_capture_on_commit_callbacks -): - settings.task_eager_propagates = (True,) - settings.task_always_eager = (True,) - - with django_capture_on_commit_callbacks() as callbacks: - ai = AlgorithmImageFactory(image__from_path=algorithm_io_image) - recurse_callbacks( - callbacks=callbacks, - django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, - ) - - editor = UserFactory() - VerificationFactory(user=editor, is_verified=True) - ai.algorithm.add_editor(editor) - ci = ComponentInterfaceFactory( - kind=InterfaceKind.InterfaceKindChoices.IMAGE, store_in_database=False - ) - ai.algorithm.inputs.set([ci]) - - image1, image2 = ImageFactory.create_batch(2) - assign_perm("cases.view_image", editor, image1) - assign_perm("cases.view_image", editor, image2) - - civ = ComponentInterfaceValueFactory(interface=ci, image=image1) - with django_capture_on_commit_callbacks(execute=True): - with django_capture_on_commit_callbacks(execute=True): - response = get_view_for_user( - viewname="algorithms:job-create", - client=client, - method=client.post, - reverse_kwargs={ - "slug": ai.algorithm.slug, - }, - user=editor, - follow=True, - data={ - **get_interface_form_data( - interface_slug=ci.slug, - data=image1.pk, - existing_data=True, - ) - }, - ) - assert response.status_code == 200 - assert str(Job.objects.get().inputs.first().image.pk) == str(image1.pk) - # same civ reused - assert Job.objects.get().inputs.first() == civ - - with django_capture_on_commit_callbacks(execute=True): - with django_capture_on_commit_callbacks(execute=True): - response = get_view_for_user( - viewname="algorithms:job-create", - client=client, - method=client.post, - reverse_kwargs={ - "slug": ai.algorithm.slug, - }, - user=editor, - follow=True, - data={ - **get_interface_form_data( - interface_slug=ci.slug, - data=image2.pk, - existing_data=True, - ) - }, - ) - assert response.status_code == 200 - assert str(Job.objects.last().inputs.first().image.pk) == str(image2.pk) - assert Job.objects.last().inputs.first() != civ - - upload = create_upload_from_file( - file_path=RESOURCE_PATH / "image10x10x10.mha", - creator=editor, - ) - with django_capture_on_commit_callbacks(execute=True): - with django_capture_on_commit_callbacks(execute=True): - response = get_view_for_user( - viewname="algorithms:job-create", - client=client, - method=client.post, - reverse_kwargs={ - "slug": ai.algorithm.slug, - }, - user=editor, - follow=True, - data={ - **get_interface_form_data( - interface_slug=ci.slug, data=upload.pk - ) - }, - ) - assert response.status_code == 200 - assert Job.objects.last().inputs.first().image.name == "image10x10x10.mha" - assert Job.objects.last().inputs.first() != civ - - @pytest.mark.django_db class TestJobCreateView: @@ -1103,6 +960,7 @@ def create_job( user=user, reverse_kwargs={ "slug": algorithm.slug, + "interface_pk": algorithm.default_interface.pk, }, follow=True, data=inputs, @@ -1141,15 +999,19 @@ def test_create_job_with_multiple_new_inputs( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_json_in_db_with_schema, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, algorithm_with_multiple_inputs.ci_json_file, algorithm_with_multiple_inputs.ci_img_upload, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) assert ComponentInterfaceValue.objects.count() == 0 @@ -1206,7 +1068,7 @@ def test_create_job_with_multiple_new_inputs( assert sorted( [ int.pk - for int in algorithm_with_multiple_inputs.algorithm.inputs.all() + for int in algorithm_with_multiple_inputs.algorithm.default_interface.inputs.all() ] ) == sorted([civ.interface.pk for civ in job.inputs.all()]) @@ -1233,14 +1095,18 @@ def test_create_job_with_existing_inputs( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_json_in_db_with_schema, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, algorithm_with_multiple_inputs.ci_json_file, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) civ1, civ2, civ3, civ4, civ5 = self.create_existing_civs( @@ -1307,13 +1173,17 @@ def test_create_job_is_idempotent( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_json_in_db_with_schema, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) civ1, civ2, civ3, civ4, civ5 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs @@ -1371,8 +1241,12 @@ def test_create_job_with_faulty_file_input( algorithm_with_multiple_inputs, ): # configure file input - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_json_file] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_json_file], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) file_upload = UserUploadFactory( filename="file.json", creator=algorithm_with_multiple_inputs.editor @@ -1418,8 +1292,12 @@ def test_create_job_with_faulty_json_input( django_capture_on_commit_callbacks, algorithm_with_multiple_inputs, ): - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_json_in_db_with_schema] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_json_in_db_with_schema], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) response = self.create_job( client=client, @@ -1448,8 +1326,12 @@ def test_create_job_with_faulty_image_input( django_capture_on_commit_callbacks, algorithm_with_multiple_inputs, ): - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_img_upload] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_img_upload], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) user_upload = create_upload_from_file( creator=algorithm_with_multiple_inputs.editor, @@ -1501,11 +1383,11 @@ def test_create_job_with_multiple_faulty_existing_image_inputs( ] ci.save() - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ - ci1, - ci2, - ] + interface = AlgorithmInterfaceFactory( + inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) assert ComponentInterfaceValue.objects.count() == 0 @@ -1658,7 +1540,10 @@ def test_job_time_limit(client): ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=True ) - algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], outputs=[ComponentInterfaceFactory()] + ) + algorithm.interfaces.add(interface, through_defaults={"is_default": True}) response = get_view_for_user( viewname="algorithms:job-create", @@ -1666,6 +1551,7 @@ def test_job_time_limit(client): method=client.post, reverse_kwargs={ "slug": algorithm.slug, + "interface_pk": algorithm.default_interface.pk, }, user=user, follow=True, @@ -1705,7 +1591,10 @@ def test_job_gpu_type_set(client, settings): ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=True ) - algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], outputs=[ComponentInterfaceFactory()] + ) + algorithm.interfaces.add(interface, through_defaults={"is_default": True}) response = get_view_for_user( viewname="algorithms:job-create", @@ -1713,6 +1602,7 @@ def test_job_gpu_type_set(client, settings): method=client.post, reverse_kwargs={ "slug": algorithm.slug, + "interface_pk": algorithm.default_interface.pk, }, user=user, follow=True, @@ -1734,6 +1624,7 @@ def test_job_gpu_type_set(client, settings): assert algorithm.credits_per_job == 190 +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_job_gpu_type_set_with_api(client, settings): settings.COMPONENTS_DEFAULT_BACKEND = "grandchallenge.components.backends.amazon_sagemaker_training.AmazonSageMakerTrainingExecutor" @@ -1795,9 +1686,18 @@ def test_job_create_view_for_verified_users_only(client): alg.add_user(user) alg.add_user(editor) + interface = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) + response = get_view_for_user( viewname="algorithms:job-create", - reverse_kwargs={"slug": alg.slug}, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": alg.default_interface.pk, + }, client=client, user=user, ) @@ -1805,7 +1705,10 @@ def test_job_create_view_for_verified_users_only(client): response2 = get_view_for_user( viewname="algorithms:job-create", - reverse_kwargs={"slug": alg.slug}, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": alg.default_interface.pk, + }, client=client, user=editor, ) @@ -1903,7 +1806,10 @@ def test_job_create_denied_for_same_input_model_and_image(client): ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.IMAGE ) - alg.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], outputs=[ComponentInterfaceFactory()] + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) ai = AlgorithmImageFactory( algorithm=alg, is_manifest_valid=True, @@ -1926,6 +1832,7 @@ def test_job_create_denied_for_same_input_model_and_image(client): method=client.post, reverse_kwargs={ "slug": alg.slug, + "interface_pk": alg.default_interface.pk, }, user=creator, data={ @@ -2243,3 +2150,80 @@ def test_algorithm_interfaces_list_queryset(client): assert iots[1] in response.context["object_list"] assert iots[2] not in response.context["object_list"] assert iots[3] not in response.context["object_list"] + + +@pytest.mark.django_db +def test_interface_select_for_job_view_permission(client): + verified_user, unverified_user = UserFactory.create_batch(2) + VerificationFactory(user=verified_user, is_verified=True) + alg = AlgorithmFactory() + alg.add_user(verified_user) + alg.add_user(unverified_user) + + interface1 = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + interface2 = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + alg.interfaces.add(interface1, through_defaults={"is_default": True}) + alg.interfaces.add(interface2) + + response = get_view_for_user( + viewname="algorithms:job-interface-select", + reverse_kwargs={"slug": alg.slug}, + client=client, + user=unverified_user, + ) + assert response.status_code == 403 + + response = get_view_for_user( + viewname="algorithms:job-interface-select", + reverse_kwargs={"slug": alg.slug}, + client=client, + user=verified_user, + ) + assert response.status_code == 200 + + +@pytest.mark.django_db +def test_interface_select_automatic_redirect(client): + verified_user = UserFactory() + VerificationFactory(user=verified_user, is_verified=True) + alg = AlgorithmFactory() + alg.add_user(verified_user) + + interface = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) + + # with just 1 interface, user gets redirected immediately + response = get_view_for_user( + viewname="algorithms:job-interface-select", + reverse_kwargs={"slug": alg.slug}, + client=client, + user=verified_user, + ) + assert response.status_code == 302 + assert response.url == reverse( + "algorithms:job-create", + kwargs={"slug": alg.slug, "interface_pk": interface.pk}, + ) + + # with more than 1 interfaces, user has to choose the interface + interface2 = AlgorithmInterfaceFactory( + inputs=[ComponentInterfaceFactory()], + outputs=[ComponentInterfaceFactory()], + ) + alg.interfaces.add(interface2) + response = get_view_for_user( + viewname="algorithms:job-interface-select", + reverse_kwargs={"slug": alg.slug}, + client=client, + user=verified_user, + ) + assert response.status_code == 200 diff --git a/app/tests/conftest.py b/app/tests/conftest.py index 14ecdcdd37..a46b213262 100644 --- a/app/tests/conftest.py +++ b/app/tests/conftest.py @@ -21,6 +21,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmModelFactory, ) from tests.cases_tests import RESOURCE_PATH @@ -480,7 +481,10 @@ def algorithm_with_image_and_model_and_two_inputs(): ci1, ci2 = ComponentInterfaceFactory.create_batch( 2, kind=ComponentInterface.Kind.STRING ) - alg.inputs.set([ci1, ci2]) + interface = AlgorithmInterfaceFactory( + inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] + ) + alg.interfaces.add(interface, through_defaults={"is_default": True}) civs = [ ComponentInterfaceValueFactory(interface=ci1, value="foo"), ComponentInterfaceValueFactory(interface=ci2, value="bar"), From 86b0396d2636d9072a4c4e03109ef9f96cd6b2a2 Mon Sep 17 00:00:00 2001 From: amickan Date: Fri, 17 Jan 2025 09:13:16 +0100 Subject: [PATCH 05/18] Redo migrations --- ..._algorithminterface_algorithminterfaceoutput_and_more.py} | 5 ++++- ...thm_interfaces.py => 0065_create_algorithm_interfaces.py} | 2 +- ...rfaces_for_jobs.py => 0066_create_interfaces_for_jobs.py} | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) rename app/grandchallenge/algorithms/migrations/{0063_algorithminterface_algorithminterfaceoutput_and_more.py => 0064_algorithminterface_algorithminterfaceoutput_and_more.py} (97%) rename app/grandchallenge/algorithms/migrations/{0064_create_algorithm_interfaces.py => 0065_create_algorithm_interfaces.py} (91%) rename app/grandchallenge/algorithms/migrations/{0065_create_interfaces_for_jobs.py => 0066_create_interfaces_for_jobs.py} (91%) diff --git a/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py b/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py similarity index 97% rename from app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py rename to app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py index ffdcabea83..82c19b6d33 100644 --- a/app/grandchallenge/algorithms/migrations/0063_algorithminterface_algorithminterfaceoutput_and_more.py +++ b/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py @@ -9,7 +9,10 @@ class Migration(migrations.Migration): dependencies = [ ("components", "0022_alter_componentinterface_kind_and_more"), - ("algorithms", "0062_algorithmusercredit"), + ( + "algorithms", + "0063_alter_optionalhangingprotocolalgorithm_unique_together", + ), ] operations = [ diff --git a/app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py similarity index 91% rename from app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py rename to app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py index 0f3bb8e8dd..8e74cc9cd8 100644 --- a/app/grandchallenge/algorithms/migrations/0064_create_algorithm_interfaces.py +++ b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py @@ -32,7 +32,7 @@ class Migration(migrations.Migration): dependencies = [ ( "algorithms", - "0063_algorithminterface_algorithminterfaceoutput_and_more", + "0064_algorithminterface_algorithminterfaceoutput_and_more", ), ] diff --git a/app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py b/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py similarity index 91% rename from app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py rename to app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py index f1e116af3d..550a4066b9 100644 --- a/app/grandchallenge/algorithms/migrations/0065_create_interfaces_for_jobs.py +++ b/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py @@ -18,7 +18,7 @@ class Migration(migrations.Migration): dependencies = [ ( "algorithms", - "0064_create_algorithm_interfaces", + "0065_create_algorithm_interfaces", ), ] From 50793e69b54cdb7b47f9d263953e4000639f1c83 Mon Sep 17 00:00:00 2001 From: amickan Date: Fri, 17 Jan 2025 11:10:27 +0100 Subject: [PATCH 06/18] Fix new tests after merging main --- app/tests/algorithms_tests/test_admin.py | 1 + app/tests/algorithms_tests/test_views.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/tests/algorithms_tests/test_admin.py b/app/tests/algorithms_tests/test_admin.py index 5b09b4f177..8c90afbdfb 100644 --- a/app/tests/algorithms_tests/test_admin.py +++ b/app/tests/algorithms_tests/test_admin.py @@ -16,6 +16,7 @@ from tests.utils import recurse_callbacks +@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_job_updated_start_and_complete_times_after_admin_requeue( algorithm_image, settings, django_capture_on_commit_callbacks diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index aaf0dba230..2e1f57e49f 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -957,7 +957,12 @@ def test_create_job_with_json_file( ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=False ) - ai.algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], + ) + ai.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) with tempfile.NamedTemporaryFile(mode="w+", suffix=".json") as file: json.dump('{"Foo": "bar"}', file) @@ -973,6 +978,7 @@ def test_create_job_with_json_file( method=client.post, reverse_kwargs={ "slug": ai.algorithm.slug, + "interface_pk": interface.pk, }, user=editor, follow=True, @@ -1010,7 +1016,12 @@ def test_algorithm_job_create_with_image_input( ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.IMAGE, store_in_database=False ) - ai.algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], + ) + ai.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) image1, image2 = ImageFactory.create_batch(2) assign_perm("cases.view_image", editor, image1) @@ -1025,6 +1036,7 @@ def test_algorithm_job_create_with_image_input( method=client.post, reverse_kwargs={ "slug": ai.algorithm.slug, + "interface_pk": interface.pk, }, user=editor, follow=True, @@ -1049,6 +1061,7 @@ def test_algorithm_job_create_with_image_input( method=client.post, reverse_kwargs={ "slug": ai.algorithm.slug, + "interface_pk": interface.pk, }, user=editor, follow=True, @@ -1076,6 +1089,7 @@ def test_algorithm_job_create_with_image_input( method=client.post, reverse_kwargs={ "slug": ai.algorithm.slug, + "interface_pk": interface.pk, }, user=editor, follow=True, From a7ff28f4ec585e1f9399d2780d8fc354017a21dc Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Thu, 23 Jan 2025 11:36:43 +0100 Subject: [PATCH 07/18] Algorithm interface delete view (#3789) This enables users with the `add_algorithm` permission to remove interfaces from their algorithm. Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 --- ...ithmalgorithminterface_confirm_delete.html | 45 +++++++++ .../algorithmalgorithminterface_list.html | 7 ++ app/grandchallenge/algorithms/urls.py | 6 ++ app/grandchallenge/algorithms/views.py | 37 +++++++- app/tests/algorithms_tests/test_views.py | 91 +++++++++++++++++++ 5 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_confirm_delete.html diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_confirm_delete.html b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_confirm_delete.html new file mode 100644 index 0000000000..58ca9ae6a2 --- /dev/null +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_confirm_delete.html @@ -0,0 +1,45 @@ +{% extends "base.html" %} +{% load remove_whitespace %} + +{% block title %} + Delete Interface - {{ algorithm.title }} - {{ block.super }} +{% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content %} + +

Confirm Algorithm Interface Deletion

+
+ {% csrf_token %} +

Are you sure that you want to delete the following algorithm interface from your algorithm?

+
    +
  • Inputs: {{ object.interface.inputs.all|oxford_comma }}
  • +
  • Outputs: {{ object.interface.outputs.all|oxford_comma }}
  • +
+

+ WARNING: You are not able to undo this action. Once the interface is deleted, it is deleted forever. +

+ Cancel + +
+ +{% endblock %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html index 4dccf4ca9b..360db2f1fd 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html @@ -35,6 +35,7 @@

Algorithm Interfaces for {{ algorithm }}

Inputs Outputs Default + Delete {% for object in object_list %} @@ -42,6 +43,12 @@

Algorithm Interfaces for {{ algorithm }}

{{ object.interface.inputs.all|oxford_comma }} {{ object.interface.outputs.all|oxford_comma }} {% if object.is_default %}{% else %}{% endif %} + + + + + {% empty %} This algorithm does not have any interfaces defined yet. diff --git a/app/grandchallenge/algorithms/urls.py b/app/grandchallenge/algorithms/urls.py index 7b424a6e6e..83cd4e6c44 100644 --- a/app/grandchallenge/algorithms/urls.py +++ b/app/grandchallenge/algorithms/urls.py @@ -13,6 +13,7 @@ AlgorithmImageUpdate, AlgorithmImportView, AlgorithmInterfaceForAlgorithmCreate, + AlgorithmInterfaceForAlgorithmDelete, AlgorithmInterfacesForAlgorithmList, AlgorithmList, AlgorithmModelCreate, @@ -63,6 +64,11 @@ AlgorithmInterfaceForAlgorithmCreate.as_view(), name="interface-create", ), + path( + "/interfaces//delete/", + AlgorithmInterfaceForAlgorithmDelete.as_view(), + name="interface-delete", + ), path( "/repository/", AlgorithmRepositoryUpdate.as_view(), diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index b7fe1ba5d9..de0c7c5618 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -22,6 +22,7 @@ from django.utils.safestring import mark_safe from django.views.generic import ( CreateView, + DeleteView, DetailView, FormView, ListView, @@ -1174,7 +1175,8 @@ class AlgorithmInterfaceForAlgorithmCreate( def get_success_url(self): return reverse( - "algorithms:interface-list", kwargs={"slug": self.algorithm.slug} + "algorithms:interface-list", + kwargs={"slug": self.algorithm.slug}, ) def get_context_data(self, *args, **kwargs): @@ -1205,3 +1207,36 @@ def get_context_data(self, *args, **kwargs): } ) return context + + +class AlgorithmInterfaceForAlgorithmDelete( + AlgorithmInterfacePermissionMixin, DeleteView +): + model = AlgorithmAlgorithmInterface + + @property + def algorithm_interface(self): + return get_object_or_404( + klass=AlgorithmAlgorithmInterface, + algorithm=self.algorithm, + interface__pk=self.kwargs["interface_pk"], + is_default=False, + ) + + def get_object(self, queryset=None): + return self.algorithm_interface + + def get_success_url(self): + return reverse( + "algorithms:interface-list", + kwargs={"slug": self.algorithm.slug}, + ) + + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + context.update( + { + "algorithm": self.algorithm, + } + ) + return context diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 2e1f57e49f..cd2fcaeea5 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -2260,6 +2260,55 @@ def test_algorithm_interface_view_permission(client, viewname): assert response.status_code == status +@pytest.mark.django_db +def test_algorithm_interface_delete_permission(client): + ( + user_with_alg_add_perm, + user_without_alg_add_perm, + algorithm_editor_with_alg_add, + algorithm_editor_without_alg_add, + ) = UserFactory.create_batch(4) + assign_perm("algorithms.add_algorithm", user_with_alg_add_perm) + assign_perm("algorithms.add_algorithm", algorithm_editor_with_alg_add) + + alg = AlgorithmFactory() + alg.add_editor(algorithm_editor_with_alg_add) + alg.add_editor(algorithm_editor_without_alg_add) + + int1, int2 = AlgorithmInterfaceFactory.create_batch(2) + alg.interfaces.add(int1, through_defaults={"is_default": True}) + alg.interfaces.add(int2) + + for user, status1, status2 in [ + [user_with_alg_add_perm, 403, 403], + [user_without_alg_add_perm, 403, 403], + [algorithm_editor_with_alg_add, 200, 404], + [algorithm_editor_without_alg_add, 403, 403], + ]: + response = get_view_for_user( + viewname="algorithms:interface-delete", + client=client, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": int2.pk, + }, + user=user, + ) + assert response.status_code == status1 + + # default interface cannot be deleted + response = get_view_for_user( + viewname="algorithms:interface-delete", + client=client, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": int1.pk, + }, + user=user, + ) + assert response.status_code == status2 + + @pytest.mark.django_db def test_algorithm_interface_create(client): user = UserFactory() @@ -2325,6 +2374,48 @@ def test_algorithm_interfaces_list_queryset(client): assert iots[3] not in response.context["object_list"] +@pytest.mark.django_db +def test_algorithm_interface_delete(client): + user = UserFactory() + assign_perm("algorithms.add_algorithm", user) + alg = AlgorithmFactory() + alg.add_editor(user) + + int1, int2 = AlgorithmInterfaceFactory.create_batch(2) + alg.interfaces.add(int1, through_defaults={"is_default": True}) + alg.interfaces.add(int2) + + response = get_view_for_user( + viewname="algorithms:interface-delete", + client=client, + method=client.post, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": int1.pk, + }, + user=user, + ) + assert response.status_code == 404 + + response = get_view_for_user( + viewname="algorithms:interface-delete", + client=client, + method=client.post, + reverse_kwargs={ + "slug": alg.slug, + "interface_pk": int2.pk, + }, + user=user, + ) + assert response.status_code == 302 + # no interface was deleted + assert AlgorithmInterface.objects.count() == 2 + # only the relation between interface and algorithm was deleted + assert AlgorithmAlgorithmInterface.objects.count() == 1 + assert alg.interfaces.count() == 1 + assert alg.interfaces.get() == int1 + + @pytest.mark.django_db def test_interface_select_for_job_view_permission(client): verified_user, unverified_user = UserFactory.create_batch(2) From a8cfa597bf980706c4fa763ca93647736e98a104 Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Mon, 27 Jan 2025 11:50:51 +0100 Subject: [PATCH 08/18] Unique input sets per algorithm (#3797) Adds restriction that interfaces for an algorithm need to have unique sets of inputs, discussed [here](https://github.com/DIAGNijmegen/rse-roadmap/issues/153#issuecomment-2602182907). Also fixes an oversights from an earlier PR. --- app/grandchallenge/algorithms/admin.py | 11 +++++ app/grandchallenge/algorithms/forms.py | 60 ++++++++++++++++++----- app/grandchallenge/algorithms/models.py | 21 ++++++-- app/tests/algorithms_tests/test_forms.py | 46 +++++++---------- app/tests/algorithms_tests/test_models.py | 12 +++++ 5 files changed, 105 insertions(+), 45 deletions(-) diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index 4b1de23ec4..669d270ff7 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -264,12 +264,15 @@ def algorithm_outputs(self, obj): return oxford_comma(obj.outputs.all()) def has_change_permission(self, request, obj=None): + # interfaces cannot be modified return False def has_delete_permission(self, request, obj=None): + # interfaces cannot be deleted return False def has_add_permission(self, request, obj=None): + # interfaces should only be created through the UI return False @@ -283,6 +286,14 @@ class AlgorithmAlgorithmInterfaceAdmin(GuardedModelAdmin): ) list_filter = ("is_default", "algorithm") + def has_add_permission(self, request, obj=None): + # through table entries should only be created through the UI + return False + + def has_change_permission(self, request, obj=None): + # through table entries should only be updated through the UI + return False + admin.site.register(AlgorithmUserObjectPermission, UserObjectPermissionAdmin) admin.site.register(AlgorithmGroupObjectPermission, GroupObjectPermissionAdmin) diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index 8ad5fc4ee3..4154a48c67 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -1401,24 +1401,60 @@ def clean_set_as_default(self): return set_as_default - def clean(self): - cleaned_data = super().clean() + def clean_inputs(self): + inputs = self.cleaned_data.get("inputs", []) - inputs = cleaned_data.get("inputs", []) - outputs = cleaned_data.get("outputs", []) + if not inputs: + raise ValidationError("You must provide at least 1 input.") - if not inputs or not outputs: + if ( + AlgorithmAlgorithmInterface.objects.filter( + algorithm=self._algorithm + ) + .annotate( + input_count=Count("interface__inputs", distinct=True), + relevant_input_count=Count( + "interface__inputs", + filter=Q(interface__inputs__in=inputs), + distinct=True, + ), + ) + .filter(input_count=len(inputs), relevant_input_count=len(inputs)) + .exists() + ): raise ValidationError( - "You must provide at least 1 input and 1 output." + "An AlgorithmInterface for this algorithm with the " + "same inputs already exists. " + "Algorithm interfaces need to have unique sets of inputs." ) + return inputs - duplicate_interfaces = {*inputs}.intersection({*outputs}) + def clean_outputs(self): + outputs = self.cleaned_data.get("outputs", []) - if duplicate_interfaces: - raise ValidationError( - f"The sets of Inputs and Outputs must be unique: " - f"{oxford_comma(duplicate_interfaces)} present in both" - ) + if not outputs: + raise ValidationError("You must provide at least 1 output.") + + return outputs + + def clean(self): + cleaned_data = super().clean() + + # there should always be at least one input and one output, + # this is checked in the individual fields clean methods + inputs = cleaned_data.get("inputs") + outputs = cleaned_data.get("outputs") + + # if either of the two fields is not provided, no need to check for + # duplicates here + if inputs and outputs: + duplicate_interfaces = {*inputs}.intersection({*outputs}) + + if duplicate_interfaces: + raise ValidationError( + f"The sets of Inputs and Outputs must be unique: " + f"{oxford_comma(duplicate_interfaces)} present in both" + ) return cleaned_data diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 17bc7acf5b..d28a12a93f 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -78,6 +78,11 @@ def create( outputs, **kwargs, ): + if not inputs or not outputs: + raise ValidationError( + "An interface must have at least one input and one output." + ) + obj = get_existing_interface_for_inputs_and_outputs( inputs=inputs, outputs=outputs ) @@ -980,14 +985,22 @@ def get_jobs_with_same_inputs( else: unique_kwargs["algorithm_model__isnull"] = True + # annotate the number of inputs and the number of inputs that match + # the existing civs and filter on both counts so as to not include jobs + # with partially overlapping inputs + # or jobs with more inputs than the existing civs existing_jobs = ( Job.objects.filter(**unique_kwargs) .annotate( - inputs_match_count=Count( - "inputs", filter=Q(inputs__in=existing_civs) - ) + input_count=Count("inputs", distinct=True), + input_match_count=Count( + "inputs", filter=Q(inputs__in=existing_civs), distinct=True + ), + ) + .filter( + input_count=input_interface_count, + input_match_count=input_interface_count, ) - .filter(inputs_match_count=input_interface_count) ) return existing_jobs diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 5ff451df71..52a111de12 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -1433,6 +1433,23 @@ def test_algorithm_interface_default_interface_required(): assert form.is_valid() +@pytest.mark.django_db +def test_algorithm_interface_unique_inputs_required(): + ci1, ci2 = ComponentInterfaceFactory.create_batch(2) + alg = AlgorithmFactory() + interface = AlgorithmInterfaceFactory(inputs=[ci1]) + alg.interfaces.add(interface, through_defaults={"is_default": True}) + + form = AlgorithmInterfaceForm( + algorithm=alg, data={"inputs": [ci1], "outputs": [ci2]} + ) + assert form.is_valid() is False + assert ( + "An AlgorithmInterface for this algorithm with the same inputs already exists" + in str(form.errors) + ) + + def test_algorithm_for_phase_form_memory(): form = AlgorithmForPhaseForm( workstation_config=WorkstationConfigFactory.build(), @@ -1557,32 +1574,3 @@ def test_default_interface_for_algorithm_not_updated_when_adding_new_non_default assert new_io != io assert not new_iot.is_default assert old_iot.is_default - - @pytest.mark.django_db - def test_is_default_is_updated_when_adding_an_already_existing_interface( - self, - ): - alg = AlgorithmFactory() - ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) - - io = AlgorithmInterfaceFactory() - io.inputs.set([ci_1]) - io.outputs.set([ci_2]) - - alg.interfaces.add(io, through_defaults={"is_default": True}) - old_iot = AlgorithmAlgorithmInterface.objects.get() - - form = AlgorithmInterfaceForm( - algorithm=alg, - data={ - "inputs": [ci_1.pk], - "outputs": [ci_2.pk], - "set_as_default": False, - }, - ) - form.is_valid() - new_io = form.save() - old_iot.refresh_from_db() - - assert new_io == io - assert not old_iot.is_default diff --git a/app/tests/algorithms_tests/test_models.py b/app/tests/algorithms_tests/test_models.py index b80a048e53..45274c8f43 100644 --- a/app/tests/algorithms_tests/test_models.py +++ b/app/tests/algorithms_tests/test_models.py @@ -818,6 +818,18 @@ def test_job_with_partially_overlapping_input( ComponentInterfaceValueFactory(), ] ) + j2 = AlgorithmJobFactory( + algorithm_image=alg.active_image, + time_limit=10, + algorithm_interface=alg.default_interface, + ) + j2.inputs.set( + [ + civs[0], + civs[1], + ComponentInterfaceValueFactory(), + ] + ) jobs = Job.objects.get_jobs_with_same_inputs( inputs=data, algorithm_image=alg.active_image, From 5de9ea1f79224994441a906ae510c8118b3e3deb Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Mon, 27 Jan 2025 15:24:31 +0100 Subject: [PATCH 09/18] Create job with interface through API (#3799) This adds input-to-interface matching to the JobPostSerializer so that algorithm jobs created through the API also have the interface configured. This also adds a serializer for algorithm interfaces. Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 --- app/grandchallenge/algorithms/models.py | 27 ++-- app/grandchallenge/algorithms/serializers.py | 86 +++++----- app/tests/algorithms_tests/test_api.py | 75 +++++---- .../algorithms_tests/test_permissions.py | 26 +-- .../algorithms_tests/test_serializers.py | 149 ++++++++++++++++-- app/tests/algorithms_tests/test_views.py | 111 ++++++++++++- 6 files changed, 368 insertions(+), 106 deletions(-) diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index d28a12a93f..0392b369d8 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -96,6 +96,22 @@ def create( def delete(self): raise NotImplementedError("Bulk delete is not allowed.") + def with_input_output_counts(self, inputs=None, outputs=None): + return self.annotate( + input_count=Count("inputs", distinct=True), + output_count=Count("outputs", distinct=True), + relevant_input_count=Count( + "inputs", + filter=Q(inputs__in=inputs) if inputs is not None else Q(), + distinct=True, + ), + relevant_output_count=Count( + "outputs", + filter=Q(outputs__in=outputs) if outputs is not None else Q(), + distinct=True, + ), + ) + class AlgorithmInterface(UUIDModel): inputs = models.ManyToManyField( @@ -129,15 +145,8 @@ def get_existing_interface_for_inputs_and_outputs( *, inputs, outputs, model=AlgorithmInterface ): try: - return model.objects.annotate( - input_count=Count("inputs", distinct=True), - output_count=Count("outputs", distinct=True), - relevant_input_count=Count( - "inputs", filter=Q(inputs__in=inputs), distinct=True - ), - relevant_output_count=Count( - "outputs", filter=Q(outputs__in=outputs), distinct=True - ), + return model.objects.with_input_output_counts( + inputs=inputs, outputs=outputs ).get( relevant_input_count=len(inputs), relevant_output_count=len(outputs), diff --git a/app/grandchallenge/algorithms/serializers.py b/app/grandchallenge/algorithms/serializers.py index 28ece083a8..af9857b420 100644 --- a/app/grandchallenge/algorithms/serializers.py +++ b/app/grandchallenge/algorithms/serializers.py @@ -1,6 +1,6 @@ import logging -from django.core.exceptions import ValidationError +from django.core.exceptions import ObjectDoesNotExist, ValidationError from rest_framework import serializers from rest_framework.fields import ( CharField, @@ -16,13 +16,14 @@ from grandchallenge.algorithms.models import ( Algorithm, AlgorithmImage, + AlgorithmInterface, AlgorithmModel, Job, ) from grandchallenge.components.backends.exceptions import ( CIVNotEditableException, ) -from grandchallenge.components.models import CIVData, ComponentInterface +from grandchallenge.components.models import CIVData from grandchallenge.components.serializers import ( ComponentInterfaceSerializer, ComponentInterfaceValuePostSerializer, @@ -30,6 +31,7 @@ HyperlinkedComponentInterfaceValueSerializer, ) from grandchallenge.core.guardian import filter_by_permission +from grandchallenge.core.templatetags.remove_whitespace import oxford_comma from grandchallenge.hanging_protocols.serializers import ( HangingProtocolSerializer, ) @@ -37,12 +39,29 @@ logger = logging.getLogger(__name__) +class AlgorithmInterfaceSerializer(serializers.ModelSerializer): + """Serializer without hyperlinks for internal use""" + + inputs = ComponentInterfaceSerializer(many=True, read_only=True) + outputs = ComponentInterfaceSerializer(many=True, read_only=True) + + class Meta: + model = AlgorithmInterface + fields = [ + "inputs", + "outputs", + ] + + class AlgorithmSerializer(serializers.ModelSerializer): average_duration = SerializerMethodField() + # TODO: Still to be addressed for optional inputs pitch + # remove inputs and outputs inputs = ComponentInterfaceSerializer(many=True, read_only=True) outputs = ComponentInterfaceSerializer(many=True, read_only=True) logo = URLField(source="logo.x20.url", read_only=True) url = URLField(source="get_absolute_url", read_only=True) + interfaces = AlgorithmInterfaceSerializer(many=True, read_only=True) class Meta: model = Algorithm @@ -57,6 +76,7 @@ class Meta: "average_duration", "inputs", "outputs", + "interfaces", ] def get_average_duration(self, obj: Algorithm) -> float | None: @@ -158,7 +178,6 @@ class HyperlinkedJobSerializer(JobSerializer): view_name="api:algorithm-detail", read_only=True, ) - inputs = HyperlinkedComponentInterfaceValueSerializer(many=True) outputs = HyperlinkedComponentInterfaceValueSerializer(many=True) @@ -215,45 +234,10 @@ def validate(self, data): "You have run out of algorithm credits" ) - # validate that no inputs are provided that are not configured for the - # algorithm and that all interfaces without defaults are provided - algorithm_input_pks = {a.pk for a in self._algorithm.inputs.all()} - input_pks = {i["interface"].pk for i in data["inputs"]} - - # surplus inputs: provided but interfaces not configured for the algorithm - surplus = ComponentInterface.objects.filter( - id__in=list(input_pks - algorithm_input_pks) - ) - if surplus: - titles = ", ".join(ci.title for ci in surplus) - raise serializers.ValidationError( - f"Provided inputs(s) {titles} are not defined for this algorithm" - ) - - # missing inputs - missing = self._algorithm.inputs.filter( - id__in=list(algorithm_input_pks - input_pks), - default_value__isnull=True, - ) - if missing: - titles = ", ".join(ci.title for ci in missing) - raise serializers.ValidationError( - f"Interface(s) {titles} do not have a default value and should be provided." - ) - inputs = data.pop("inputs") - - default_inputs = self._algorithm.inputs.filter( - id__in=list(algorithm_input_pks - input_pks), - default_value__isnull=False, + data["algorithm_interface"] = ( + self.validate_inputs_and_return_matching_interface(inputs=inputs) ) - # Use default interface values if not present - for interface in default_inputs: - if interface.default_value: - inputs.append( - {"interface": interface, "value": interface.default_value} - ) - self.inputs = self.reformat_inputs(serialized_civs=inputs) if Job.objects.get_jobs_with_same_inputs( @@ -298,6 +282,28 @@ def create(self, validated_data): return job + def validate_inputs_and_return_matching_interface(self, *, inputs): + """ + Validates that the provided inputs match one of the configured interfaces of + the algorithm and returns that AlgorithmInterface + """ + provided_inputs = {i["interface"] for i in inputs} + try: + interface = self._algorithm.interfaces.with_input_output_counts( + inputs=provided_inputs + ).get( + relevant_input_count=len(provided_inputs), + input_count=len(provided_inputs), + ) + return interface + except ObjectDoesNotExist: + raise serializers.ValidationError( + f"The set of inputs provided does not match " + f"any of the algorithm's interfaces. This algorithm supports the " + f"following input combinations: " + f"{oxford_comma([f'Interface {n}: {oxford_comma(interface.inputs.all())}' for n, interface in enumerate(self._algorithm.interfaces.all(), start=1)])}" + ) + @staticmethod def reformat_inputs(*, serialized_civs): """Takes serialized CIV data and returns list of CIVData objects.""" diff --git a/app/tests/algorithms_tests/test_api.py b/app/tests/algorithms_tests/test_api.py index de7591df68..3b91399d83 100644 --- a/app/tests/algorithms_tests/test_api.py +++ b/app/tests/algorithms_tests/test_api.py @@ -18,6 +18,7 @@ from grandchallenge.uploads.models import UserUpload from tests.algorithms_tests.factories import ( AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, AlgorithmModelFactory, ) @@ -93,7 +94,6 @@ def test_job_list_view_num_queries( assert len(response.json()["results"]) == num_jobs -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db class TestJobCreationThroughAPI: @@ -151,15 +151,19 @@ def test_create_job_with_multiple_new_inputs( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_json_in_db_with_schema, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, algorithm_with_multiple_inputs.ci_json_file, algorithm_with_multiple_inputs.ci_img_upload, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) assert ComponentInterfaceValue.objects.count() == 0 @@ -217,12 +221,9 @@ def test_create_job_with_multiple_new_inputs( pk=algorithm_with_multiple_inputs.file_upload.pk ).exists() - assert sorted( - [ - int.pk - for int in algorithm_with_multiple_inputs.algorithm.inputs.all() - ] - ) == sorted([civ.interface.pk for civ in job.inputs.all()]) + assert sorted([int.pk for int in interface.inputs.all()]) == sorted( + [civ.interface.pk for civ in job.inputs.all()] + ) value_inputs = [civ.value for civ in job.inputs.all() if civ.value] assert "Foo" in value_inputs @@ -245,13 +246,17 @@ def test_create_job_with_existing_inputs( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_json_in_db_with_schema, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) civ1, civ2, civ3, civ4 = self.create_existing_civs( @@ -302,13 +307,17 @@ def test_create_job_is_idempotent( algorithm_with_multiple_inputs, ): # configure multiple inputs - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ + interface = AlgorithmInterfaceFactory( + inputs=[ algorithm_with_multiple_inputs.ci_str, algorithm_with_multiple_inputs.ci_bool, algorithm_with_multiple_inputs.ci_existing_img, algorithm_with_multiple_inputs.ci_json_in_db_with_schema, - ] + ], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) civ1, civ2, civ3, civ4 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs @@ -366,8 +375,12 @@ def test_create_job_with_faulty_file_input( algorithm_with_multiple_inputs, ): # configure file input - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_json_file] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_json_file], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) file_upload = UserUploadFactory( filename="file.json", creator=algorithm_with_multiple_inputs.editor @@ -414,8 +427,12 @@ def test_create_job_with_faulty_json_input( django_capture_on_commit_callbacks, algorithm_with_multiple_inputs, ): - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_json_in_db_with_schema] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_json_in_db_with_schema], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) response = self.create_job( @@ -445,8 +462,12 @@ def test_create_job_with_faulty_image_input( django_capture_on_commit_callbacks, algorithm_with_multiple_inputs, ): - algorithm_with_multiple_inputs.algorithm.inputs.set( - [algorithm_with_multiple_inputs.ci_img_upload] + interface = AlgorithmInterfaceFactory( + inputs=[algorithm_with_multiple_inputs.ci_img_upload], + outputs=[ComponentInterfaceFactory()], + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) user_upload = create_upload_from_file( creator=algorithm_with_multiple_inputs.editor, @@ -501,11 +522,11 @@ def test_create_job_with_multiple_faulty_existing_image_inputs( ] ci.save() - algorithm_with_multiple_inputs.algorithm.inputs.set( - [ - ci1, - ci2, - ] + interface = AlgorithmInterfaceFactory( + inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] + ) + algorithm_with_multiple_inputs.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) im = ImageFactory() diff --git a/app/tests/algorithms_tests/test_permissions.py b/app/tests/algorithms_tests/test_permissions.py index 3a0ff8951c..c96b0f03e5 100644 --- a/app/tests/algorithms_tests/test_permissions.py +++ b/app/tests/algorithms_tests/test_permissions.py @@ -345,9 +345,6 @@ def test_job_permissions_from_template(self, client): algorithm_image=algorithm_image, job=job, user=user ) - @pytest.mark.xfail( - reason="Still to be addressed for optional inputs pitch" - ) def test_job_permissions_from_api(self, rf): # setup user = UserFactory() @@ -356,18 +353,23 @@ def test_job_permissions_from_api(self, rf): is_in_registry=True, is_desired_version=True, ) - interfaces = { - ComponentInterfaceFactory( - kind=ComponentInterface.Kind.STRING, - title="TestInterface 1", - default_value="default", - ), - } - algorithm_image.algorithm.inputs.set(interfaces) + ci = ComponentInterfaceFactory( + kind=ComponentInterface.Kind.STRING, + title="TestInterface 1", + default_value="default", + ) + + interface = AlgorithmInterfaceFactory(inputs=[ci]) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) algorithm_image.algorithm.add_user(user) algorithm_image.algorithm.add_editor(UserFactory()) - job = {"algorithm": algorithm_image.algorithm.api_url, "inputs": []} + job = { + "algorithm": algorithm_image.algorithm.api_url, + "inputs": [{"interface": ci.slug, "value": "foo"}], + } # test request = rf.get("/foo") diff --git a/app/tests/algorithms_tests/test_serializers.py b/app/tests/algorithms_tests/test_serializers.py index eb5cd14412..4e9c90d7a2 100644 --- a/app/tests/algorithms_tests/test_serializers.py +++ b/app/tests/algorithms_tests/test_serializers.py @@ -12,6 +12,7 @@ from tests.algorithms_tests.factories import ( AlgorithmFactory, AlgorithmImageFactory, + AlgorithmInterfaceFactory, AlgorithmJobFactory, ) from tests.cases_tests.factories import RawImageUploadSessionFactory @@ -35,7 +36,6 @@ def test_algorithm_relations_on_job_serializer(rf): ) -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db @pytest.mark.parametrize( "title, " @@ -79,7 +79,7 @@ def test_algorithm_relations_on_job_serializer(rf): True, ("TestInterface 1", "TestInterface 2"), ("testinterface-1",), - "Interface(s) TestInterface 2 do not have a default value and should be provided.", + "The set of inputs provided does not match any of the algorithm's interfaces.", False, ), ( @@ -88,7 +88,7 @@ def test_algorithm_relations_on_job_serializer(rf): True, ("TestInterface 1",), ("testinterface-1", "testinterface-2"), - "Provided inputs(s) TestInterface 2 are not defined for this algorithm", + "The set of inputs provided does not match any of the algorithm's interfaces.", False, ), ( @@ -137,8 +137,11 @@ def test_algorithm_job_post_serializer_validations( is_desired_version=image_ready, ) algorithm_image.algorithm.title = title - algorithm_image.algorithm.inputs.set( - [interfaces[title] for title in algorithm_interface_titles] + interface = AlgorithmInterfaceFactory( + inputs=[interfaces[title] for title in algorithm_interface_titles] + ) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} ) if add_user: algorithm_image.algorithm.add_user(user) @@ -161,8 +164,7 @@ def test_algorithm_job_post_serializer_validations( job = { "algorithm": algorithm_image.algorithm.api_url, "inputs": [ - {"interface": interface, "value": "dummy"} - for interface in job_interface_slugs + {"interface": int, "value": "dummy"} for int in job_interface_slugs ], } @@ -184,7 +186,6 @@ def test_algorithm_job_post_serializer_validations( assert len(job.inputs.all()) == 2 -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_algorithm_job_post_serializer_create( rf, settings, django_capture_on_commit_callbacks @@ -212,7 +213,10 @@ def test_algorithm_job_post_serializer_create( ci_img1 = ComponentInterfaceFactory(kind=ComponentInterface.Kind.IMAGE) ci_img2 = ComponentInterfaceFactory(kind=ComponentInterface.Kind.IMAGE) - algorithm_image.algorithm.inputs.set([ci_string, ci_img2, ci_img1]) + interface = AlgorithmInterfaceFactory(inputs=[ci_string, ci_img2, ci_img1]) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) algorithm_image.algorithm.add_editor(user) job = { @@ -228,8 +232,22 @@ def test_algorithm_job_post_serializer_create( request.user = user serializer = JobPostSerializer(data=job, context={"request": request}) - # verify + # all inputs need to be provided, also those with default value + assert not serializer.is_valid() + + # add missing input + job = { + "algorithm": algorithm_image.algorithm.api_url, + "inputs": [ + {"interface": ci_img1.slug, "upload_session": upload.api_url}, + {"interface": ci_img2.slug, "image": image2.api_url}, + {"interface": ci_string.slug, "value": "foo"}, + ], + } + serializer = JobPostSerializer(data=job, context={"request": request}) + assert serializer.is_valid() + # fake successful upload upload.status = RawImageUploadSession.SUCCESS upload.save() @@ -240,9 +258,9 @@ def test_algorithm_job_post_serializer_create( job = Job.objects.first() assert job.creator == user assert len(job.inputs.all()) == 3 + assert job.algorithm_interface == interface -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db class TestJobCreateLimits: def test_form_invalid_without_enough_credits(self, rf, settings): @@ -292,13 +310,20 @@ def test_form_valid_for_editor(self, rf, settings): user = UserFactory() algorithm_image.algorithm.add_editor(user=user) + ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) + interface = AlgorithmInterfaceFactory(inputs=[ci]) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) request = rf.get("/foo") request.user = user serializer = JobPostSerializer( data={ "algorithm": algorithm_image.algorithm.api_url, - "inputs": [], + "inputs": [ + {"interface": ci.slug, "value": "foo"}, + ], }, context={"request": request}, ) @@ -315,7 +340,9 @@ def test_form_valid_for_editor(self, rf, settings): serializer = JobPostSerializer( data={ "algorithm": algorithm_image.algorithm.api_url, - "inputs": [], + "inputs": [ + {"interface": ci.slug, "value": "foo"}, + ], }, context={"request": request}, ) @@ -333,13 +360,20 @@ def test_form_valid_with_credits(self, rf): user = UserFactory() algorithm_image.algorithm.add_user(user=user) + ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) + interface = AlgorithmInterfaceFactory(inputs=[ci]) + algorithm_image.algorithm.interfaces.add( + interface, through_defaults={"is_default": True} + ) request = rf.get("/foo") request.user = user serializer = JobPostSerializer( data={ "algorithm": algorithm_image.algorithm.api_url, - "inputs": [], + "inputs": [ + {"interface": ci.slug, "value": "foo"}, + ], }, context={"request": request}, ) @@ -383,7 +417,6 @@ def test_reformat_inputs(rf): ) -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_algorithm_post_serializer_image_and_time_limit_fixed(rf): request = rf.get("/foo") @@ -398,7 +431,9 @@ def test_algorithm_post_serializer_image_and_time_limit_fixed(rf): ) different_ai = AlgorithmImageFactory(algorithm=alg) ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) - alg.inputs.set([ci]) + interface = AlgorithmInterfaceFactory(inputs=[ci]) + alg.interfaces.add(interface, through_defaults={"is_default": True}) + serializer = JobPostSerializer( data={ "algorithm": alg.api_url, @@ -416,3 +451,85 @@ def test_algorithm_post_serializer_image_and_time_limit_fixed(rf): assert job.algorithm_image != different_ai assert not job.algorithm_model assert job.time_limit == 10 + + +@pytest.mark.parametrize( + "inputs, interface", + ( + ([1], 1), # matches interface 1 of algorithm + ([1, 2], 2), # matches interface 2 of algorithm + ([3, 4, 5], 3), # matches interface 3 of algorithm + ([4], None), # matches interface 4, but not configured for algorithm + ( + [1, 2, 3], + None, + ), # matches interface 5, but not configured for algorithm + ([2], None), # matches no interface (implements part of interface 2) + ( + [1, 3, 4], + None, + ), # matches no interface (implements interface 3 and an additional input) + ), +) +@pytest.mark.django_db +def test_validate_inputs_on_job_serializer(inputs, interface, rf): + user = UserFactory() + algorithm = AlgorithmFactory() + algorithm.add_editor(user) + AlgorithmImageFactory( + algorithm=algorithm, + is_desired_version=True, + is_manifest_valid=True, + is_in_registry=True, + ) + + io1, io2, io3, io4, io5 = AlgorithmInterfaceFactory.create_batch(5) + ci1, ci2, ci3, ci4, ci5, ci6 = ComponentInterfaceFactory.create_batch( + 6, kind=ComponentInterface.Kind.STRING + ) + + interfaces = [io1, io2, io3] + cis = [ci1, ci2, ci3, ci4, ci5, ci6] + + io1.inputs.set([ci1]) + io2.inputs.set([ci1, ci2]) + io3.inputs.set([ci3, ci4, ci5]) + io4.inputs.set([ci1, ci2, ci3]) + io5.inputs.set([ci4]) + io1.outputs.set([ci6]) + io2.outputs.set([ci3]) + io3.outputs.set([ci1]) + io4.outputs.set([ci1]) + io5.outputs.set([ci1]) + + algorithm.interfaces.add(io1, through_defaults={"is_default": True}) + algorithm.interfaces.add(io2) + algorithm.interfaces.add(io3) + + algorithm_interface = interfaces[interface - 1] if interface else None + inputs = [cis[i - 1] for i in inputs] + + job = { + "algorithm": algorithm.api_url, + "inputs": [ + {"interface": int.slug, "value": "dummy"} for int in inputs + ], + } + + request = rf.get("/foo") + request.user = user + serializer = JobPostSerializer(data=job, context={"request": request}) + + if interface: + assert serializer.is_valid() + assert ( + serializer.validated_data["algorithm_interface"] + == algorithm_interface + ) + else: + assert not serializer.is_valid() + assert ( + "The set of inputs provided does not match any of the algorithm's interfaces." + in str(serializer.errors) + ) + assert "algorithm_interface" not in serializer.validated_data diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index 51d7fee597..cedf9e818e 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -830,6 +830,110 @@ def list_algorithms(self, **__): "look_up_table": None, }, ], + "interfaces": [ + { + "inputs": [ + { + "title": "Coronal T2 Prostate MRI", + "description": "Coronal T2 MRI of the Prostate", + "slug": "coronal-t2-prostate-mri", + "kind": "Image", + "pk": 31, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/coronal-t2-prostate-mri", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Transverse T2 Prostate MRI", + "description": "Transverse T2 MRI of the Prostate", + "slug": "transverse-t2-prostate-mri", + "kind": "Image", + "pk": 32, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/transverse-t2-prostate-mri", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Sagittal T2 Prostate MRI", + "description": "Sagittal T2 MRI of the Prostate", + "slug": "sagittal-t2-prostate-mri", + "kind": "Image", + "pk": 33, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/sagittal-t2-prostate-mri", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Transverse HBV Prostate MRI", + "description": "Transverse High B-Value Prostate MRI", + "slug": "transverse-hbv-prostate-mri", + "kind": "Image", + "pk": 47, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/transverse-hbv-prostate-mri", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Transverse ADC Prostate MRI", + "description": "Transverse Apparent Diffusion Coefficient Prostate MRI", + "slug": "transverse-adc-prostate-mri", + "kind": "Image", + "pk": 48, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/transverse-adc-prostate-mri", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Clinical Information Prostate MRI", + "description": "Clinical information to support clinically significant prostate cancer detection in prostate MRI. Provided information: patient age at time of examination (patient_age), PSA level in ng/mL as reported (PSA_report), PSA density in ng/mL^2 as reported (PSAD_report), prostate volume as reported (prostate_volume_report), prostate volume derived from automatic whole-gland segmentation (prostate_volume_automatic), scanner manufacturer (scanner_manufacturer), scanner model name (scanner_model_name), diffusion b-value of (calculated) high b-value diffusion map (diffusion_high_bvalue). Values acquired from radiology reports will be missing, if not reported.", + "slug": "clinical-information-prostate-mri", + "kind": "Anything", + "pk": 156, + "default_value": None, + "super_kind": "Value", + "relative_path": "clinical-information-prostate-mri.json", + "overlay_segments": [], + "look_up_table": None, + }, + ], + "outputs": [ + { + "title": "Case-level Cancer Likelihood Prostate MRI", + "description": "Case-level likelihood of harboring clinically significant prostate cancer, in range [0,1].", + "slug": "prostate-cancer-likelihood", + "kind": "Float", + "pk": 144, + "default_value": None, + "super_kind": "Value", + "relative_path": "cspca-case-level-likelihood.json", + "overlay_segments": [], + "look_up_table": None, + }, + { + "title": "Transverse Cancer Detection Map Prostate MRI", + "description": "Single-class, detection map of clinically significant prostate cancer lesions in 3D, where each voxel represents a floating point in range [0,1].", + "slug": "cspca-detection-map", + "kind": "Heat Map", + "pk": 151, + "default_value": None, + "super_kind": "Image", + "relative_path": "images/cspca-detection-map", + "overlay_segments": [], + "look_up_table": None, + }, + ], + } + ], } ], } @@ -1805,7 +1909,6 @@ def test_job_gpu_type_set(client, settings): assert algorithm.credits_per_job == 190 -@pytest.mark.xfail(reason="Still to be addressed for optional inputs pitch") @pytest.mark.django_db def test_job_gpu_type_set_with_api(client, settings): settings.COMPONENTS_DEFAULT_BACKEND = "grandchallenge.components.backends.amazon_sagemaker_training.AmazonSageMakerTrainingExecutor" @@ -1827,7 +1930,10 @@ def test_job_gpu_type_set_with_api(client, settings): ci = ComponentInterfaceFactory( kind=InterfaceKind.InterfaceKindChoices.ANY, store_in_database=True ) - algorithm.inputs.set([ci]) + interface = AlgorithmInterfaceFactory( + inputs=[ci], + ) + algorithm.interfaces.add(interface, through_defaults={"is_default": True}) response = get_view_for_user( viewname="api:algorithms-job-list", @@ -1851,6 +1957,7 @@ def test_job_gpu_type_set_with_api(client, settings): job = Job.objects.get() + assert job.algorithm_interface == interface assert job.algorithm_image == algorithm_image assert job.requires_gpu_type == GPUTypeChoices.A10G assert job.requires_memory_gb == 64 From 9a1bd62ce2bc8daeaf6ab069e7522cf261ebe7b2 Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Thu, 30 Jan 2025 15:33:22 +0100 Subject: [PATCH 10/18] Move input/output annotation from manager to separate method (#3804) I added an input/output count annotation to the `AlgorithmInterfaceManager` in my last PR, but had not tested the migrations again. In my migration check for the phase interfaces, I realized that the migrations break with that addition since they don't have access to custom manager methods. So I'm moving the annotation to a separate function here. The upside of this is that I can now use it in 2 other places where we're making the same annotation. So I'm calling this a win. Note that this does not add or change functionality, it just moves the code. So I didn't spend time on adding a test for the new function - it is indirectly covered by tests of all the places where it is used. --- app/grandchallenge/algorithms/forms.py | 58 +++++++++----------- app/grandchallenge/algorithms/models.py | 58 +++++++++----------- app/grandchallenge/algorithms/serializers.py | 8 ++- 3 files changed, 59 insertions(+), 65 deletions(-) diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index 4154a48c67..fd7a296751 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -56,6 +56,7 @@ AlgorithmModel, AlgorithmPermissionRequest, Job, + annotate_input_output_counts, ) from grandchallenge.algorithms.serializers import ( AlgorithmImageSerializer, @@ -462,37 +463,32 @@ def user_algorithms_for_phase(self): desired_model_subquery = AlgorithmModel.objects.filter( algorithm=OuterRef("pk"), is_desired_version=True ) - return ( - get_objects_for_user(self._user, "algorithms.change_algorithm") - .annotate( - total_input_count=Count("inputs", distinct=True), - total_output_count=Count("outputs", distinct=True), - relevant_input_count=Count( - "inputs", filter=Q(inputs__in=inputs), distinct=True - ), - relevant_output_count=Count( - "outputs", filter=Q(outputs__in=outputs), distinct=True - ), - has_active_image=Exists(desired_image_subquery), - active_image_pk=desired_image_subquery.values_list( - "pk", flat=True - ), - active_model_pk=desired_model_subquery.values_list( - "pk", flat=True - ), - active_image_comment=desired_image_subquery.values_list( - "comment", flat=True - ), - active_model_comment=desired_model_subquery.values_list( - "comment", flat=True - ), - ) - .filter( - total_input_count=len(inputs), - total_output_count=len(outputs), - relevant_input_count=len(inputs), - relevant_output_count=len(outputs), - ) + annotated_qs = annotate_input_output_counts( + queryset=get_objects_for_user( + self._user, "algorithms.change_algorithm" + ), + inputs=inputs, + outputs=outputs, + ) + return annotated_qs.annotate( + has_active_image=Exists(desired_image_subquery), + active_image_pk=desired_image_subquery.values_list( + "pk", flat=True + ), + active_model_pk=desired_model_subquery.values_list( + "pk", flat=True + ), + active_image_comment=desired_image_subquery.values_list( + "comment", flat=True + ), + active_model_comment=desired_model_subquery.values_list( + "comment", flat=True + ), + ).filter( + input_count=len(inputs), + output_count=len(outputs), + relevant_input_count=len(inputs), + relevant_output_count=len(outputs), ) @cached_property diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 0392b369d8..2c2958a93b 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -69,6 +69,23 @@ JINJA_ENGINE = sandbox.ImmutableSandboxedEnvironment() +def annotate_input_output_counts(queryset, inputs=None, outputs=None): + return queryset.annotate( + input_count=Count("inputs", distinct=True), + output_count=Count("outputs", distinct=True), + relevant_input_count=Count( + "inputs", + filter=Q(inputs__in=inputs) if inputs is not None else Q(), + distinct=True, + ), + relevant_output_count=Count( + "outputs", + filter=Q(outputs__in=outputs) if outputs is not None else Q(), + distinct=True, + ), + ) + + class AlgorithmInterfaceManager(models.Manager): def create( @@ -96,22 +113,6 @@ def create( def delete(self): raise NotImplementedError("Bulk delete is not allowed.") - def with_input_output_counts(self, inputs=None, outputs=None): - return self.annotate( - input_count=Count("inputs", distinct=True), - output_count=Count("outputs", distinct=True), - relevant_input_count=Count( - "inputs", - filter=Q(inputs__in=inputs) if inputs is not None else Q(), - distinct=True, - ), - relevant_output_count=Count( - "outputs", - filter=Q(outputs__in=outputs) if outputs is not None else Q(), - distinct=True, - ), - ) - class AlgorithmInterface(UUIDModel): inputs = models.ManyToManyField( @@ -144,10 +145,11 @@ class AlgorithmInterfaceOutput(models.Model): def get_existing_interface_for_inputs_and_outputs( *, inputs, outputs, model=AlgorithmInterface ): + annotated_qs = annotate_input_output_counts( + model.objects.all(), inputs=inputs, outputs=outputs + ) try: - return model.objects.with_input_output_counts( - inputs=inputs, outputs=outputs - ).get( + return annotated_qs.get( relevant_input_count=len(inputs), relevant_output_count=len(outputs), input_count=len(inputs), @@ -998,18 +1000,12 @@ def get_jobs_with_same_inputs( # the existing civs and filter on both counts so as to not include jobs # with partially overlapping inputs # or jobs with more inputs than the existing civs - existing_jobs = ( - Job.objects.filter(**unique_kwargs) - .annotate( - input_count=Count("inputs", distinct=True), - input_match_count=Count( - "inputs", filter=Q(inputs__in=existing_civs), distinct=True - ), - ) - .filter( - input_count=input_interface_count, - input_match_count=input_interface_count, - ) + annotated_qs = annotate_input_output_counts( + queryset=Job.objects.filter(**unique_kwargs), inputs=existing_civs + ) + existing_jobs = annotated_qs.filter( + input_count=input_interface_count, + relevant_input_count=input_interface_count, ) return existing_jobs diff --git a/app/grandchallenge/algorithms/serializers.py b/app/grandchallenge/algorithms/serializers.py index af9857b420..ac930f13d9 100644 --- a/app/grandchallenge/algorithms/serializers.py +++ b/app/grandchallenge/algorithms/serializers.py @@ -19,6 +19,7 @@ AlgorithmInterface, AlgorithmModel, Job, + annotate_input_output_counts, ) from grandchallenge.components.backends.exceptions import ( CIVNotEditableException, @@ -288,10 +289,11 @@ def validate_inputs_and_return_matching_interface(self, *, inputs): the algorithm and returns that AlgorithmInterface """ provided_inputs = {i["interface"] for i in inputs} + annotated_qs = annotate_input_output_counts( + self._algorithm.interfaces, inputs=provided_inputs + ) try: - interface = self._algorithm.interfaces.with_input_output_counts( - inputs=provided_inputs - ).get( + interface = annotated_qs.get( relevant_input_count=len(provided_inputs), input_count=len(provided_inputs), ) From 66a21cbb37c52f98659fee284c6cbc25064f549a Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Fri, 31 Jan 2025 07:56:37 +0100 Subject: [PATCH 11/18] Remove is_default field for algorithm interfaces (#3811) Discussed with James that we don't need the option to mark an interface as default, so removing it here. --- app/grandchallenge/algorithms/admin.py | 4 +- app/grandchallenge/algorithms/forms.py | 40 +----- ...rface_algorithminterfaceoutput_and_more.py | 9 -- .../0065_create_algorithm_interfaces.py | 2 +- .../0066_create_interfaces_for_jobs.py | 5 +- app/grandchallenge/algorithms/models.py | 21 --- .../algorithmalgorithminterface_list.html | 4 +- app/grandchallenge/algorithms/views.py | 3 +- app/tests/algorithms_tests/test_api.py | 28 +--- app/tests/algorithms_tests/test_forms.py | 122 ++---------------- app/tests/algorithms_tests/test_models.py | 63 ++------- .../algorithms_tests/test_permissions.py | 10 +- .../algorithms_tests/test_serializers.py | 21 +-- app/tests/algorithms_tests/test_tasks.py | 12 +- app/tests/algorithms_tests/test_views.py | 112 +++++----------- app/tests/conftest.py | 2 +- 16 files changed, 85 insertions(+), 373 deletions(-) diff --git a/app/grandchallenge/algorithms/admin.py b/app/grandchallenge/algorithms/admin.py index 669d270ff7..b04a7499e8 100644 --- a/app/grandchallenge/algorithms/admin.py +++ b/app/grandchallenge/algorithms/admin.py @@ -53,7 +53,6 @@ class Meta: class AlgorithmAdmin(GuardedModelAdmin): readonly_fields = ( "algorithm_forge_json", - "default_interface", "inputs", "outputs", ) @@ -281,10 +280,9 @@ class AlgorithmAlgorithmInterfaceAdmin(GuardedModelAdmin): list_display = ( "pk", "interface", - "is_default", "algorithm", ) - list_filter = ("is_default", "algorithm") + list_filter = ("algorithm",) def has_add_permission(self, request, obj=None): # through table entries should only be created through the UI diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index fd7a296751..ff0f4c1cb2 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -26,7 +26,6 @@ from django.db.models import Count, Exists, Max, OuterRef, Q from django.db.transaction import on_commit from django.forms import ( - BooleanField, CharField, Form, HiddenInput, @@ -1372,31 +1371,18 @@ class AlgorithmInterfaceForm(SaveFormInitMixin, ModelForm): ), widget=Select2MultipleWidget, ) - set_as_default = BooleanField(required=False) class Meta: model = AlgorithmInterface fields = ( "inputs", "outputs", - "set_as_default", ) def __init__(self, *args, algorithm, **kwargs): super().__init__(*args, **kwargs) self._algorithm = algorithm - if not self._algorithm.default_interface: - self.fields["set_as_default"].initial = True - - def clean_set_as_default(self): - set_as_default = self.cleaned_data["set_as_default"] - - if not set_as_default and not self._algorithm.default_interface: - raise ValidationError("Your algorithm needs a default interface.") - - return set_as_default - def clean_inputs(self): inputs = self.cleaned_data.get("inputs", []) @@ -1459,29 +1445,7 @@ def save(self): inputs=self.cleaned_data["inputs"], outputs=self.cleaned_data["outputs"], ) - - if self.cleaned_data["set_as_default"]: - AlgorithmAlgorithmInterface.objects.filter( - algorithm=self._algorithm - ).update(is_default=False) - - matched_rows = AlgorithmAlgorithmInterface.objects.filter( - algorithm=self._algorithm, interface=interface - ).update(is_default=self.cleaned_data["set_as_default"]) - - if matched_rows == 0: - self._algorithm.interfaces.add( - interface, - through_defaults={ - "is_default": self.cleaned_data["set_as_default"] - }, - ) - elif matched_rows > 1: - raise RuntimeError( - "This algorithm and interface are associated " - "with each other more than once." - ) - + self._algorithm.interfaces.add(interface) return interface @@ -1502,7 +1466,7 @@ def __init__(self, *args, algorithm, **kwargs): self._algorithm.interfaces.all() ) self.fields["algorithm_interface"].initial = ( - self._algorithm.default_interface + self._algorithm.interfaces.first() ) self.fields["algorithm_interface"].widget.choices = { ( diff --git a/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py b/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py index 82c19b6d33..b79bff954a 100644 --- a/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py +++ b/app/grandchallenge/algorithms/migrations/0064_algorithminterface_algorithminterfaceoutput_and_more.py @@ -121,7 +121,6 @@ class Migration(migrations.Migration): verbose_name="ID", ), ), - ("is_default", models.BooleanField(default=False)), ( "algorithm", models.ForeignKey( @@ -147,14 +146,6 @@ class Migration(migrations.Migration): to="algorithms.algorithminterface", ), ), - migrations.AddConstraint( - model_name="algorithmalgorithminterface", - constraint=models.UniqueConstraint( - condition=models.Q(("is_default", True)), - fields=("algorithm",), - name="unique_default_interface_per_algorithm", - ), - ), migrations.AddConstraint( model_name="algorithmalgorithminterface", constraint=models.UniqueConstraint( diff --git a/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py index 8e74cc9cd8..949f4eea93 100644 --- a/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py +++ b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py @@ -25,7 +25,7 @@ def create_algorithm_interfaces(apps, _schema_editor): io.inputs.set(inputs) io.outputs.set(outputs) - algorithm.interfaces.add(io, through_defaults={"is_default": True}) + algorithm.interfaces.add(io) class Migration(migrations.Migration): diff --git a/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py b/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py index 550a4066b9..55f7c33332 100644 --- a/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py +++ b/app/grandchallenge/algorithms/migrations/0066_create_interfaces_for_jobs.py @@ -6,11 +6,8 @@ def add_algorithm_interfaces_to_jobs(apps, _schema_editor): Job = apps.get_model("algorithms", "Job") # noqa: N806 for algorithm in Algorithm.objects.prefetch_related("interfaces").all(): - default_interface = algorithm.interfaces.get( - algorithmalgorithminterface__is_default=True - ) Job.objects.filter(algorithm_image__algorithm=algorithm).update( - algorithm_interface=default_interface + algorithm_interface=algorithm.interfaces.get() ) diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 2c2958a93b..caa8be7b85 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -520,15 +520,6 @@ def default_workstation(self): return w - @cached_property - def default_interface(self): - try: - return self.interfaces.get( - algorithmalgorithminterface__is_default=True - ) - except ObjectDoesNotExist: - return None - def is_editor(self, user): return user.groups.filter(pk=self.editors_group.pk).exists() @@ -632,15 +623,9 @@ def form_field_label(self): class AlgorithmAlgorithmInterface(models.Model): algorithm = models.ForeignKey(Algorithm, on_delete=models.CASCADE) interface = models.ForeignKey(AlgorithmInterface, on_delete=models.CASCADE) - is_default = models.BooleanField(default=False) class Meta: constraints = [ - models.UniqueConstraint( - fields=["algorithm"], - condition=Q(is_default=True), - name="unique_default_interface_per_algorithm", - ), models.UniqueConstraint( fields=["algorithm", "interface"], name="unique_algorithm_interface_combination", @@ -650,12 +635,6 @@ class Meta: def __str__(self): return str(self.interface) - def clean(self): - super().clean() - - if not self.is_default and not self.algorithm.default_interface: - raise ValidationError("This algorithm needs a default interface.") - class AlgorithmUserObjectPermission(UserObjectPermissionBase): content_object = models.ForeignKey(Algorithm, on_delete=models.CASCADE) diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html index 360db2f1fd..1efb894bb7 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html @@ -34,7 +34,6 @@

Algorithm Interfaces for {{ algorithm }}

Inputs Outputs - Default Delete @@ -42,9 +41,8 @@

Algorithm Interfaces for {{ algorithm }}

{{ object.interface.inputs.all|oxford_comma }} {{ object.interface.outputs.all|oxford_comma }} - {% if object.is_default %}{% else %}{% endif %} - diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index de391b0e82..83272f2984 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -511,7 +511,7 @@ class JobInterfaceSelect( def get(self, request, *args, **kwargs): if self.algorithm.interfaces.count() == 1: - self.selected_interface = self.algorithm.default_interface + self.selected_interface = self.algorithm.interfaces.get() return HttpResponseRedirect(self.get_success_url()) else: return super().get(request, *args, **kwargs) @@ -1229,7 +1229,6 @@ def algorithm_interface(self): klass=AlgorithmAlgorithmInterface, algorithm=self.algorithm, interface__pk=self.kwargs["interface_pk"], - is_default=False, ) def get_object(self, queryset=None): diff --git a/app/tests/algorithms_tests/test_api.py b/app/tests/algorithms_tests/test_api.py index 3b91399d83..78540a4f9e 100644 --- a/app/tests/algorithms_tests/test_api.py +++ b/app/tests/algorithms_tests/test_api.py @@ -162,9 +162,7 @@ def test_create_job_with_multiple_new_inputs( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) assert ComponentInterfaceValue.objects.count() == 0 @@ -255,9 +253,7 @@ def test_create_job_with_existing_inputs( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) civ1, civ2, civ3, civ4 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs @@ -316,9 +312,7 @@ def test_create_job_is_idempotent( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) civ1, civ2, civ3, civ4 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs ) @@ -379,9 +373,7 @@ def test_create_job_with_faulty_file_input( inputs=[algorithm_with_multiple_inputs.ci_json_file], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) file_upload = UserUploadFactory( filename="file.json", creator=algorithm_with_multiple_inputs.editor ) @@ -431,9 +423,7 @@ def test_create_job_with_faulty_json_input( inputs=[algorithm_with_multiple_inputs.ci_json_in_db_with_schema], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) response = self.create_job( client=client, @@ -466,9 +456,7 @@ def test_create_job_with_faulty_image_input( inputs=[algorithm_with_multiple_inputs.ci_img_upload], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) user_upload = create_upload_from_file( creator=algorithm_with_multiple_inputs.editor, file_path=RESOURCE_PATH / "corrupt.png", @@ -525,9 +513,7 @@ def test_create_job_with_multiple_faulty_existing_image_inputs( interface = AlgorithmInterfaceFactory( inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) im = ImageFactory() im.files.set([ImageFileFactoryWithMHDFile()]) diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 52a111de12..7358a36cb8 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -18,7 +18,6 @@ ) from grandchallenge.algorithms.models import ( Algorithm, - AlgorithmAlgorithmInterface, AlgorithmPermissionRequest, Job, ) @@ -323,7 +322,7 @@ def test_create_job_input_fields( client=client, reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, follow=True, user=creator, @@ -358,7 +357,7 @@ def test_create_job_json_input_field_validation( client=client, reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, method=client.post, follow=True, @@ -392,7 +391,7 @@ def test_create_job_simple_input_field_validation( client=client, reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, method=client.post, follow=True, @@ -413,7 +412,7 @@ def create_algorithm_with_input(slug): inputs=[ComponentInterface.objects.get(slug=slug)], outputs=[ComponentInterfaceFactory()], ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) AlgorithmImageFactory( algorithm=alg, is_manifest_valid=True, @@ -741,7 +740,7 @@ def test_creator_queryset( form = JobCreateForm( algorithm=algorithm, user=editor, - interface=algorithm.default_interface, + interface=algorithm.interfaces.first(), data={}, ) assert list(form.fields["creator"].queryset.all()) == [editor] @@ -762,7 +761,7 @@ def test_algorithm_image_queryset( form = JobCreateForm( algorithm=algorithm, user=editor, - interface=algorithm.default_interface, + interface=algorithm.interfaces.first(), data={}, ) ai_qs = form.fields["algorithm_image"].queryset.all() @@ -783,14 +782,14 @@ def test_cannot_create_job_with_same_inputs_twice( algorithm_model=algorithm.active_model, status=Job.SUCCESS, time_limit=123, - algorithm_interface=algorithm.default_interface, + algorithm_interface=algorithm.interfaces.first(), ) job.inputs.set(civs) form = JobCreateForm( algorithm=algorithm, user=editor, - interface=algorithm.default_interface, + interface=algorithm.interfaces.first(), data={ "algorithm_image": algorithm.active_image, "algorithm_model": algorithm.active_model, @@ -819,9 +818,7 @@ def test_all_inputs_required_on_job_creation(algorithm_with_multiple_inputs): inputs=[ci_json_in_db_without_schema], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) form = JobCreateForm( algorithm=algorithm_with_multiple_inputs.algorithm, @@ -1409,36 +1406,12 @@ def test_algorithm_interface_disjoint_interfaces(): assert "The sets of Inputs and Outputs must be unique" in str(form.errors) -@pytest.mark.django_db -def test_algorithm_interface_default_interface_required(): - ci1, ci2 = ComponentInterfaceFactory.create_batch(2) - alg = AlgorithmFactory() - form = AlgorithmInterfaceForm( - algorithm=alg, - data={"inputs": [ci1], "outputs": [ci2], "set_as_default": False}, - ) - assert form.is_valid() is False - assert "Your algorithm needs a default interface" in str( - form.errors["set_as_default"] - ) - - alg.interfaces.add( - AlgorithmInterfaceFactory(), through_defaults={"is_default": True} - ) - del alg.default_interface - form = AlgorithmInterfaceForm( - algorithm=alg, - data={"inputs": [ci1], "outputs": [ci2], "set_as_default": False}, - ) - assert form.is_valid() - - @pytest.mark.django_db def test_algorithm_interface_unique_inputs_required(): ci1, ci2 = ComponentInterfaceFactory.create_batch(2) alg = AlgorithmFactory() interface = AlgorithmInterfaceFactory(inputs=[ci1]) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) form = AlgorithmInterfaceForm( algorithm=alg, data={"inputs": [ci1], "outputs": [ci2]} @@ -1478,26 +1451,6 @@ def test_algorithm_for_phase_form_memory(): class TestAlgorithmInterfaceForm: - @pytest.mark.django_db - def test_set_as_default_initial_value(self): - alg = AlgorithmFactory() - - form = AlgorithmInterfaceForm( - algorithm=alg, - ) - assert form.fields["set_as_default"].initial - - alg.interfaces.add( - AlgorithmInterfaceFactory(), through_defaults={"is_default": True} - ) - - del alg.default_interface - - form = AlgorithmInterfaceForm( - algorithm=alg, - ) - assert not form.fields["set_as_default"].initial - @pytest.mark.django_db def test_existing_io_is_reused(self): inp = ComponentInterfaceFactory() @@ -1513,64 +1466,9 @@ def test_existing_io_is_reused(self): data={ "inputs": [inp.pk], "outputs": [out.pk], - "set_as_default": True, }, ) assert form.is_valid() new_io = form.save() assert io == new_io - - @pytest.mark.django_db - def test_new_default_interface_updates_related_interfaces(self): - ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) - alg = AlgorithmFactory() - io = AlgorithmInterfaceFactory() - alg.interfaces.add(io, through_defaults={"is_default": True}) - - old_iot = AlgorithmAlgorithmInterface.objects.get() - - form = AlgorithmInterfaceForm( - algorithm=alg, - data={ - "inputs": [ci_1.pk], - "outputs": [ci_2.pk], - "set_as_default": True, - }, - ) - form.is_valid() - new_io = form.save() - new_iot = AlgorithmAlgorithmInterface.objects.get(interface=new_io) - old_iot.refresh_from_db() - - assert new_io != io - assert new_iot.is_default - assert not old_iot.is_default - - @pytest.mark.django_db - def test_default_interface_for_algorithm_not_updated_when_adding_new_non_default_interface( - self, - ): - alg = AlgorithmFactory() - io = AlgorithmInterfaceFactory() - alg.interfaces.add(io, through_defaults={"is_default": True}) - old_iot = AlgorithmAlgorithmInterface.objects.get() - - ci_1, ci_2 = ComponentInterfaceFactory.create_batch(2) - - form = AlgorithmInterfaceForm( - algorithm=alg, - data={ - "inputs": [ci_1.pk], - "outputs": [ci_2.pk], - "set_as_default": False, - }, - ) - form.is_valid() - new_io = form.save() - old_iot.refresh_from_db() - new_iot = AlgorithmAlgorithmInterface.objects.get(interface=new_io) - - assert new_io != io - assert not new_iot.is_default - assert old_iot.is_default diff --git a/app/tests/algorithms_tests/test_models.py b/app/tests/algorithms_tests/test_models.py index 45274c8f43..96af3ff593 100644 --- a/app/tests/algorithms_tests/test_models.py +++ b/app/tests/algorithms_tests/test_models.py @@ -77,14 +77,6 @@ def test_group_deletion_reverse(group): getattr(algorithm, group).delete() -@pytest.mark.django_db -def test_no_default_interfaces_created(): - a = AlgorithmFactory() - - assert {i.kind for i in a.inputs.all()} == set() - assert {o.kind for o in a.outputs.all()} == set() - - @pytest.mark.django_db def test_rendered_result_text(): def create_result(jb, result: dict): @@ -658,7 +650,7 @@ def test_job_with_same_image_different_model( j = AlgorithmJobFactory( algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) @@ -680,7 +672,7 @@ def test_job_with_same_model_different_image( algorithm_image=AlgorithmImageFactory(), algorithm_model=alg.active_model, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -701,7 +693,7 @@ def test_job_with_same_model_and_image( algorithm_model=alg.active_model, algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -723,7 +715,7 @@ def test_job_with_different_image_and_model( algorithm_model=AlgorithmModelFactory(), algorithm_image=AlgorithmImageFactory(), time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -744,7 +736,7 @@ def test_job_with_same_image_no_model_provided( algorithm_model=alg.active_model, algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -764,7 +756,7 @@ def test_job_with_same_image_and_without_model( j = AlgorithmJobFactory( algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set(civs) jobs = Job.objects.get_jobs_with_same_inputs( @@ -785,7 +777,7 @@ def test_job_with_different_input( j = AlgorithmJobFactory( algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set( [ @@ -810,7 +802,7 @@ def test_job_with_partially_overlapping_input( j = AlgorithmJobFactory( algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j.inputs.set( [ @@ -821,7 +813,7 @@ def test_job_with_partially_overlapping_input( j2 = AlgorithmJobFactory( algorithm_image=alg.active_image, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) j2.inputs.set( [ @@ -1038,11 +1030,11 @@ def test_inputs_complete(): interface = AlgorithmInterfaceFactory( inputs=[ci1, ci2, ci3], outputs=[ComponentInterfaceFactory()] ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) job = AlgorithmJobFactory( algorithm_image__algorithm=alg, time_limit=10, - algorithm_interface=alg.default_interface, + algorithm_interface=alg.interfaces.first(), ) civ_with_value_1 = ComponentInterfaceValueFactory( interface=ci1, value="Foo" @@ -1462,30 +1454,17 @@ def test_algorithmalgorithminterface_unique_constraints(): interface1, interface2 = AlgorithmInterfaceFactory.create_batch(2) algorithm = AlgorithmFactory() - io = AlgorithmAlgorithmInterface.objects.create( - interface=interface1, algorithm=algorithm, is_default=True + AlgorithmAlgorithmInterface.objects.create( + interface=interface1, algorithm=algorithm ) - # cannot add a second default interface to an algorithm - with pytest.raises(IntegrityError): - with transaction.atomic(): - AlgorithmAlgorithmInterface.objects.create( - interface=interface2, algorithm=algorithm, is_default=True - ) - # cannot add a second time the same interface for the same algorithm with pytest.raises(IntegrityError): with transaction.atomic(): AlgorithmAlgorithmInterface.objects.create( - interface=interface1, algorithm=algorithm, is_default=False + interface=interface1, algorithm=algorithm ) - # but you can update an existing entry from default to non-default - io.is_default = False - io.save() - assert AlgorithmAlgorithmInterface.objects.count() == 1 - assert not AlgorithmAlgorithmInterface.objects.get().is_default - @pytest.mark.parametrize( "inputs, outputs, expected_output", @@ -1557,17 +1536,3 @@ def test_algorithminterface_create(): io = AlgorithmInterface.objects.create(inputs=inputs, outputs=outputs) assert list(io.inputs.all()) == inputs assert list(io.outputs.all()) == outputs - - -@pytest.mark.django_db -def test_has_default_interface(): - alg1, alg2, alg3 = AlgorithmFactory.create_batch(3) - io1, io2 = AlgorithmInterfaceFactory.create_batch(2) - - alg1.interfaces.add(io1, through_defaults={"is_default": True}) - alg1.interfaces.add(io2) - alg2.interfaces.add(io1) - - assert alg1.default_interface == io1 - assert not alg2.default_interface - assert not alg3.default_interface diff --git a/app/tests/algorithms_tests/test_permissions.py b/app/tests/algorithms_tests/test_permissions.py index c96b0f03e5..081ffea444 100644 --- a/app/tests/algorithms_tests/test_permissions.py +++ b/app/tests/algorithms_tests/test_permissions.py @@ -317,9 +317,7 @@ def test_job_permissions_from_template(self, client): interface = AlgorithmInterfaceFactory( inputs=[ci], outputs=[ComponentInterfaceFactory()] ) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) response = get_view_for_user( viewname="algorithms:job-create", @@ -327,7 +325,7 @@ def test_job_permissions_from_template(self, client): method=client.post, reverse_kwargs={ "slug": algorithm_image.algorithm.slug, - "interface_pk": algorithm_image.algorithm.default_interface.pk, + "interface_pk": algorithm_image.algorithm.interfaces.first().pk, }, user=user, follow=True, @@ -360,9 +358,7 @@ def test_job_permissions_from_api(self, rf): ) interface = AlgorithmInterfaceFactory(inputs=[ci]) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) algorithm_image.algorithm.add_user(user) algorithm_image.algorithm.add_editor(UserFactory()) diff --git a/app/tests/algorithms_tests/test_serializers.py b/app/tests/algorithms_tests/test_serializers.py index 4e9c90d7a2..b176f78b3e 100644 --- a/app/tests/algorithms_tests/test_serializers.py +++ b/app/tests/algorithms_tests/test_serializers.py @@ -140,9 +140,8 @@ def test_algorithm_job_post_serializer_validations( interface = AlgorithmInterfaceFactory( inputs=[interfaces[title] for title in algorithm_interface_titles] ) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) + if add_user: algorithm_image.algorithm.add_user(user) @@ -214,9 +213,7 @@ def test_algorithm_job_post_serializer_create( ci_img2 = ComponentInterfaceFactory(kind=ComponentInterface.Kind.IMAGE) interface = AlgorithmInterfaceFactory(inputs=[ci_string, ci_img2, ci_img1]) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) algorithm_image.algorithm.add_editor(user) job = { @@ -312,9 +309,7 @@ def test_form_valid_for_editor(self, rf, settings): algorithm_image.algorithm.add_editor(user=user) ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) interface = AlgorithmInterfaceFactory(inputs=[ci]) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) request = rf.get("/foo") request.user = user @@ -362,9 +357,7 @@ def test_form_valid_with_credits(self, rf): algorithm_image.algorithm.add_user(user=user) ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) interface = AlgorithmInterfaceFactory(inputs=[ci]) - algorithm_image.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_image.algorithm.interfaces.add(interface) request = rf.get("/foo") request.user = user @@ -432,7 +425,7 @@ def test_algorithm_post_serializer_image_and_time_limit_fixed(rf): different_ai = AlgorithmImageFactory(algorithm=alg) ci = ComponentInterfaceFactory(kind=ComponentInterface.Kind.STRING) interface = AlgorithmInterfaceFactory(inputs=[ci]) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) serializer = JobPostSerializer( data={ @@ -502,7 +495,7 @@ def test_validate_inputs_on_job_serializer(inputs, interface, rf): io4.outputs.set([ci1]) io5.outputs.set([ci1]) - algorithm.interfaces.add(io1, through_defaults={"is_default": True}) + algorithm.interfaces.add(io1) algorithm.interfaces.add(io2) algorithm.interfaces.add(io3) diff --git a/app/tests/algorithms_tests/test_tasks.py b/app/tests/algorithms_tests/test_tasks.py index 3f2baa705e..e4e75d2759 100644 --- a/app/tests/algorithms_tests/test_tasks.py +++ b/app/tests/algorithms_tests/test_tasks.py @@ -232,9 +232,7 @@ def test_algorithm( inputs=[input_interface], outputs=[json_result_interface, heatmap_interface], ) - ai.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + ai.algorithm.interfaces.add(interface) with django_capture_on_commit_callbacks() as callbacks: get_view_for_user( @@ -385,9 +383,7 @@ def test_algorithm_with_invalid_output( interface = AlgorithmInterfaceFactory( inputs=[input_interface], outputs=[detection_interface] ) - ai.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + ai.algorithm.interfaces.add(interface) image_file = ImageFileFactory( file__from_path=Path(__file__).parent / "resources" / "input_file.tif" @@ -475,9 +471,7 @@ def test_execute_algorithm_job_for_missing_inputs(settings): interface = AlgorithmInterfaceFactory( inputs=[ci], outputs=[ComponentInterfaceFactory()] ) - alg.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + alg.algorithm.interfaces.add(interface) job = AlgorithmJobFactory( creator=creator, algorithm_image=alg, diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py index cedf9e818e..bf057cb9bb 100644 --- a/app/tests/algorithms_tests/test_views.py +++ b/app/tests/algorithms_tests/test_views.py @@ -361,9 +361,7 @@ def test_permission_required_views(self, client): inputs=[ComponentInterfaceFactory()], outputs=[ComponentInterfaceFactory()], ) - ai.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + ai.algorithm.interfaces.add(interface) VerificationFactory(user=u, is_verified=True) @@ -439,7 +437,7 @@ def test_permission_required_views(self, client): "job-create", { "slug": ai.algorithm.slug, - "interface_pk": ai.algorithm.default_interface.pk, + "interface_pk": ai.algorithm.interfaces.first().pk, }, "execute_algorithm", ai.algorithm, @@ -1072,9 +1070,7 @@ def test_create_job_with_json_file( interface = AlgorithmInterfaceFactory( inputs=[ci], ) - ai.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + ai.algorithm.interfaces.add(interface) with tempfile.NamedTemporaryFile(mode="w+", suffix=".json") as file: json.dump('{"Foo": "bar"}', file) @@ -1131,9 +1127,7 @@ def test_algorithm_job_create_with_image_input( interface = AlgorithmInterfaceFactory( inputs=[ci], ) - ai.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + ai.algorithm.interfaces.add(interface) image1, image2 = ImageFactory.create_batch(2) assign_perm("cases.view_image", editor, image1) @@ -1241,7 +1235,7 @@ def create_job( user=user, reverse_kwargs={ "slug": algorithm.slug, - "interface_pk": algorithm.default_interface.pk, + "interface_pk": algorithm.interfaces.first().pk, }, follow=True, data=inputs, @@ -1291,9 +1285,7 @@ def test_create_job_with_multiple_new_inputs( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) assert ComponentInterfaceValue.objects.count() == 0 @@ -1353,7 +1345,7 @@ def test_create_job_with_multiple_new_inputs( assert sorted( [ int.pk - for int in algorithm_with_multiple_inputs.algorithm.default_interface.inputs.all() + for int in algorithm_with_multiple_inputs.algorithm.interfaces.first().inputs.all() ] ) == sorted([civ.interface.pk for civ in job.inputs.all()]) @@ -1390,9 +1382,7 @@ def test_create_job_with_existing_inputs( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) civ1, civ2, civ3, civ4, civ5 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs @@ -1467,9 +1457,7 @@ def test_create_job_is_idempotent( ], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) civ1, civ2, civ3, civ4, civ5 = self.create_existing_civs( interface_data=algorithm_with_multiple_inputs ) @@ -1530,9 +1518,7 @@ def test_create_job_with_faulty_file_input( inputs=[algorithm_with_multiple_inputs.ci_json_file], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) file_upload = UserUploadFactory( filename="file.json", creator=algorithm_with_multiple_inputs.editor ) @@ -1581,9 +1567,7 @@ def test_create_job_with_faulty_json_input( inputs=[algorithm_with_multiple_inputs.ci_json_in_db_with_schema], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) response = self.create_job( client=client, django_capture_on_commit_callbacks=django_capture_on_commit_callbacks, @@ -1615,9 +1599,7 @@ def test_create_job_with_faulty_image_input( inputs=[algorithm_with_multiple_inputs.ci_img_upload], outputs=[ComponentInterfaceFactory()], ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) user_upload = create_upload_from_file( creator=algorithm_with_multiple_inputs.editor, file_path=RESOURCE_PATH / "corrupt.png", @@ -1671,9 +1653,7 @@ def test_create_job_with_multiple_faulty_existing_image_inputs( interface = AlgorithmInterfaceFactory( inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] ) - algorithm_with_multiple_inputs.algorithm.interfaces.add( - interface, through_defaults={"is_default": True} - ) + algorithm_with_multiple_inputs.algorithm.interfaces.add(interface) assert ComponentInterfaceValue.objects.count() == 0 @@ -1828,7 +1808,7 @@ def test_job_time_limit(client): interface = AlgorithmInterfaceFactory( inputs=[ci], outputs=[ComponentInterfaceFactory()] ) - algorithm.interfaces.add(interface, through_defaults={"is_default": True}) + algorithm.interfaces.add(interface) response = get_view_for_user( viewname="algorithms:job-create", @@ -1836,7 +1816,7 @@ def test_job_time_limit(client): method=client.post, reverse_kwargs={ "slug": algorithm.slug, - "interface_pk": algorithm.default_interface.pk, + "interface_pk": algorithm.interfaces.first().pk, }, user=user, follow=True, @@ -1879,7 +1859,7 @@ def test_job_gpu_type_set(client, settings): interface = AlgorithmInterfaceFactory( inputs=[ci], outputs=[ComponentInterfaceFactory()] ) - algorithm.interfaces.add(interface, through_defaults={"is_default": True}) + algorithm.interfaces.add(interface) response = get_view_for_user( viewname="algorithms:job-create", @@ -1887,7 +1867,7 @@ def test_job_gpu_type_set(client, settings): method=client.post, reverse_kwargs={ "slug": algorithm.slug, - "interface_pk": algorithm.default_interface.pk, + "interface_pk": algorithm.interfaces.first().pk, }, user=user, follow=True, @@ -1933,7 +1913,7 @@ def test_job_gpu_type_set_with_api(client, settings): interface = AlgorithmInterfaceFactory( inputs=[ci], ) - algorithm.interfaces.add(interface, through_defaults={"is_default": True}) + algorithm.interfaces.add(interface) response = get_view_for_user( viewname="api:algorithms-job-list", @@ -1978,13 +1958,13 @@ def test_job_create_view_for_verified_users_only(client): inputs=[ComponentInterfaceFactory()], outputs=[ComponentInterfaceFactory()], ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) response = get_view_for_user( viewname="algorithms:job-create", reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, client=client, user=user, @@ -1995,7 +1975,7 @@ def test_job_create_view_for_verified_users_only(client): viewname="algorithms:job-create", reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, client=client, user=editor, @@ -2097,7 +2077,7 @@ def test_job_create_denied_for_same_input_model_and_image(client): interface = AlgorithmInterfaceFactory( inputs=[ci], outputs=[ComponentInterfaceFactory()] ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) ai = AlgorithmImageFactory( algorithm=alg, is_manifest_valid=True, @@ -2120,7 +2100,7 @@ def test_job_create_denied_for_same_input_model_and_image(client): method=client.post, reverse_kwargs={ "slug": alg.slug, - "interface_pk": alg.default_interface.pk, + "interface_pk": alg.interfaces.first().pk, }, user=creator, data={ @@ -2391,14 +2371,14 @@ def test_algorithm_interface_delete_permission(client): alg.add_editor(algorithm_editor_without_alg_add) int1, int2 = AlgorithmInterfaceFactory.create_batch(2) - alg.interfaces.add(int1, through_defaults={"is_default": True}) + alg.interfaces.add(int1) alg.interfaces.add(int2) - for user, status1, status2 in [ - [user_with_alg_add_perm, 403, 403], - [user_without_alg_add_perm, 403, 403], - [algorithm_editor_with_alg_add, 200, 404], - [algorithm_editor_without_alg_add, 403, 403], + for user, status in [ + [user_with_alg_add_perm, 403], + [user_without_alg_add_perm, 403], + [algorithm_editor_with_alg_add, 200], + [algorithm_editor_without_alg_add, 403], ]: response = get_view_for_user( viewname="algorithms:interface-delete", @@ -2409,19 +2389,7 @@ def test_algorithm_interface_delete_permission(client): }, user=user, ) - assert response.status_code == status1 - - # default interface cannot be deleted - response = get_view_for_user( - viewname="algorithms:interface-delete", - client=client, - reverse_kwargs={ - "slug": alg.slug, - "interface_pk": int1.pk, - }, - user=user, - ) - assert response.status_code == status2 + assert response.status_code == status @pytest.mark.django_db @@ -2442,7 +2410,6 @@ def test_algorithm_interface_create(client): data={ "inputs": [ci_1.pk], "outputs": [ci_2.pk], - "set_as_default": True, }, user=user, ) @@ -2457,7 +2424,6 @@ def test_algorithm_interface_create(client): io_through = AlgorithmAlgorithmInterface.objects.get() assert io_through.algorithm == alg assert io_through.interface == io - assert io_through.is_default @pytest.mark.django_db @@ -2497,21 +2463,9 @@ def test_algorithm_interface_delete(client): alg.add_editor(user) int1, int2 = AlgorithmInterfaceFactory.create_batch(2) - alg.interfaces.add(int1, through_defaults={"is_default": True}) + alg.interfaces.add(int1) alg.interfaces.add(int2) - response = get_view_for_user( - viewname="algorithms:interface-delete", - client=client, - method=client.post, - reverse_kwargs={ - "slug": alg.slug, - "interface_pk": int1.pk, - }, - user=user, - ) - assert response.status_code == 404 - response = get_view_for_user( viewname="algorithms:interface-delete", client=client, @@ -2547,7 +2501,7 @@ def test_interface_select_for_job_view_permission(client): inputs=[ComponentInterfaceFactory()], outputs=[ComponentInterfaceFactory()], ) - alg.interfaces.add(interface1, through_defaults={"is_default": True}) + alg.interfaces.add(interface1) alg.interfaces.add(interface2) response = get_view_for_user( @@ -2578,7 +2532,7 @@ def test_interface_select_automatic_redirect(client): inputs=[ComponentInterfaceFactory()], outputs=[ComponentInterfaceFactory()], ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) # with just 1 interface, user gets redirected immediately response = get_view_for_user( diff --git a/app/tests/conftest.py b/app/tests/conftest.py index a46b213262..6fbdb3ed3a 100644 --- a/app/tests/conftest.py +++ b/app/tests/conftest.py @@ -484,7 +484,7 @@ def algorithm_with_image_and_model_and_two_inputs(): interface = AlgorithmInterfaceFactory( inputs=[ci1, ci2], outputs=[ComponentInterfaceFactory()] ) - alg.interfaces.add(interface, through_defaults={"is_default": True}) + alg.interfaces.add(interface) civs = [ ComponentInterfaceValueFactory(interface=ci1, value="foo"), ComponentInterfaceValueFactory(interface=ci2, value="bar"), From c75cd76617db0b5f19bf65a134b056ea707bb32a Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Fri, 31 Jan 2025 12:13:12 +0100 Subject: [PATCH 12/18] Add algorithm interfaces to phases and migrate (#3803) This adds a `algorithm_interfaces` M2M to the `Phase` model and adds a custom through model for it. It also removes the input / output configuration option from both the phase admin and the `ConfigureAlgorithmPhases` view. In anticipation of PR 2, it refactors the interface create form so that it can be easily used for both phases and algorithms alike. Views to create and manage interfaces for phases will be added in a separate PR. Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 --- app/grandchallenge/algorithms/forms.py | 14 ++--- .../0065_create_algorithm_interfaces.py | 3 + app/grandchallenge/algorithms/models.py | 8 +++ app/grandchallenge/algorithms/views.py | 2 +- app/grandchallenge/evaluation/admin.py | 42 +++++++------ app/grandchallenge/evaluation/forms.py | 14 +---- ...ace_phase_algorithm_interfaces_and_more.py | 62 +++++++++++++++++++ .../0073_migrate_interfaces_for_phases.py | 49 +++++++++++++++ app/grandchallenge/evaluation/models.py | 31 +++++++++- .../configure_algorithm_phases_form.html | 3 + .../evaluation/views/__init__.py | 6 -- app/tests/algorithms_tests/test_forms.py | 6 +- app/tests/evaluation_tests/test_admin.py | 19 +----- app/tests/evaluation_tests/test_models.py | 3 +- app/tests/evaluation_tests/test_views.py | 4 -- 15 files changed, 189 insertions(+), 77 deletions(-) create mode 100644 app/grandchallenge/evaluation/migrations/0072_phasealgorithminterface_phase_algorithm_interfaces_and_more.py create mode 100644 app/grandchallenge/evaluation/migrations/0073_migrate_interfaces_for_phases.py diff --git a/app/grandchallenge/algorithms/forms.py b/app/grandchallenge/algorithms/forms.py index ff0f4c1cb2..7d988d7ed2 100644 --- a/app/grandchallenge/algorithms/forms.py +++ b/app/grandchallenge/algorithms/forms.py @@ -49,7 +49,6 @@ from grandchallenge.algorithms.models import ( Algorithm, - AlgorithmAlgorithmInterface, AlgorithmImage, AlgorithmInterface, AlgorithmModel, @@ -1379,9 +1378,9 @@ class Meta: "outputs", ) - def __init__(self, *args, algorithm, **kwargs): + def __init__(self, *args, base_obj, **kwargs): super().__init__(*args, **kwargs) - self._algorithm = algorithm + self._base_obj = base_obj def clean_inputs(self): inputs = self.cleaned_data.get("inputs", []) @@ -1390,10 +1389,7 @@ def clean_inputs(self): raise ValidationError("You must provide at least 1 input.") if ( - AlgorithmAlgorithmInterface.objects.filter( - algorithm=self._algorithm - ) - .annotate( + self._base_obj.algorithm_interface_through_model_manager.annotate( input_count=Count("interface__inputs", distinct=True), relevant_input_count=Count( "interface__inputs", @@ -1405,7 +1401,7 @@ def clean_inputs(self): .exists() ): raise ValidationError( - "An AlgorithmInterface for this algorithm with the " + f"An AlgorithmInterface for this {self._base_obj._meta.verbose_name} with the " "same inputs already exists. " "Algorithm interfaces need to have unique sets of inputs." ) @@ -1445,7 +1441,7 @@ def save(self): inputs=self.cleaned_data["inputs"], outputs=self.cleaned_data["outputs"], ) - self._algorithm.interfaces.add(interface) + self._base_obj.algorithm_interface_manager.add(interface) return interface diff --git a/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py index 949f4eea93..3caf518a7a 100644 --- a/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py +++ b/app/grandchallenge/algorithms/migrations/0065_create_algorithm_interfaces.py @@ -17,6 +17,9 @@ def create_algorithm_interfaces(apps, _schema_editor): inputs = algorithm.inputs.all() outputs = algorithm.outputs.all() + if not inputs or not outputs: + raise RuntimeError(f"{algorithm} is improperly configured.") + io = get_existing_interface_for_inputs_and_outputs( model=AlgorithmInterface, inputs=inputs, outputs=outputs ) diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index caa8be7b85..77666071e1 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -520,6 +520,14 @@ def default_workstation(self): return w + @property + def algorithm_interface_manager(self): + return self.interfaces + + @property + def algorithm_interface_through_model_manager(self): + return AlgorithmAlgorithmInterface.objects.filter(algorithm=self) + def is_editor(self, user): return user.groups.filter(pk=self.editors_group.pk).exists() diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py index 83272f2984..0e550eac14 100644 --- a/app/grandchallenge/algorithms/views.py +++ b/app/grandchallenge/algorithms/views.py @@ -1195,7 +1195,7 @@ def get_context_data(self, *args, **kwargs): def get_form_kwargs(self): kwargs = super().get_form_kwargs() - kwargs.update({"algorithm": self.algorithm}) + kwargs.update({"base_obj": self.algorithm}) return kwargs diff --git a/app/grandchallenge/evaluation/admin.py b/app/grandchallenge/evaluation/admin.py index aad7d674c0..d922ec8a66 100644 --- a/app/grandchallenge/evaluation/admin.py +++ b/app/grandchallenge/evaluation/admin.py @@ -1,9 +1,9 @@ import json from django.contrib import admin -from django.core.exceptions import ValidationError from django.forms import ModelForm from django.utils.html import format_html +from guardian.admin import GuardedModelAdmin from grandchallenge.components.admin import ( ComponentImageAdmin, @@ -15,7 +15,6 @@ GroupObjectPermissionAdmin, UserObjectPermissionAdmin, ) -from grandchallenge.core.templatetags.remove_whitespace import oxford_comma from grandchallenge.core.utils.grand_challenge_forge import ( get_forge_challenge_pack_context, ) @@ -29,6 +28,7 @@ MethodGroupObjectPermission, MethodUserObjectPermission, Phase, + PhaseAlgorithmInterface, PhaseGroupObjectPermission, PhaseUserObjectPermission, Submission, @@ -52,21 +52,6 @@ def __init__(self, *args, **kwargs): ) in self.instance.read_only_fields_for_dependent_phases: self.fields[field_name].disabled = True - def clean(self): - cleaned_data = super().clean() - - duplicate_interfaces = { - *cleaned_data.get("algorithm_inputs", []) - }.intersection({*cleaned_data.get("algorithm_outputs", [])}) - - if duplicate_interfaces: - raise ValidationError( - f"The sets of Algorithm Inputs and Algorithm Outputs must be unique: " - f"{oxford_comma(duplicate_interfaces)} present in both" - ) - - return cleaned_data - @admin.register(Phase) class PhaseAdmin(admin.ModelAdmin): @@ -97,13 +82,14 @@ class PhaseAdmin(admin.ModelAdmin): autocomplete_fields = ( "inputs", "outputs", - "algorithm_inputs", - "algorithm_outputs", "archive", ) readonly_fields = ( "give_algorithm_editors_job_view_permissions", "challenge_forge_json", + "algorithm_interfaces", + "algorithm_inputs", + "algorithm_outputs", ) form = PhaseAdminForm @@ -226,6 +212,24 @@ class EvaluationGroundTruthAdmin(admin.ModelAdmin): readonly_fields = ("creator", "phase", "sha256", "size_in_storage") +@admin.register(PhaseAlgorithmInterface) +class PhaseAlgorithmInterfaceAdmin(GuardedModelAdmin): + list_display = ( + "pk", + "interface", + "phase", + ) + list_filter = ("phase",) + + def has_add_permission(self, request, obj=None): + # through table entries should only be created through the UI + return False + + def has_change_permission(self, request, obj=None): + # through table entries should only be updated through the UI + return False + + admin.site.register(PhaseUserObjectPermission, UserObjectPermissionAdmin) admin.site.register(PhaseGroupObjectPermission, GroupObjectPermissionAdmin) admin.site.register(Method, ComponentImageAdmin) diff --git a/app/grandchallenge/evaluation/forms.py b/app/grandchallenge/evaluation/forms.py index 1bef795ad3..b2f5d768e3 100644 --- a/app/grandchallenge/evaluation/forms.py +++ b/app/grandchallenge/evaluation/forms.py @@ -21,16 +21,12 @@ ) from django.utils.html import format_html from django.utils.text import format_lazy -from django_select2.forms import Select2MultipleWidget from grandchallenge.algorithms.forms import UserAlgorithmsForPhaseMixin from grandchallenge.algorithms.models import Job from grandchallenge.challenges.models import Challenge, ChallengeRequest from grandchallenge.components.forms import ContainerImageForm -from grandchallenge.components.models import ( - ComponentInterface, - ImportStatusChoices, -) +from grandchallenge.components.models import ImportStatusChoices from grandchallenge.components.schemas import GPUTypeChoices from grandchallenge.components.tasks import assign_tarball_from_upload from grandchallenge.core.forms import ( @@ -742,14 +738,6 @@ class ConfigureAlgorithmPhasesForm(SaveFormInitMixin, Form): queryset=None, widget=CheckboxSelectMultiple, ) - algorithm_inputs = ModelMultipleChoiceField( - queryset=ComponentInterface.objects.all(), - widget=Select2MultipleWidget, - ) - algorithm_outputs = ModelMultipleChoiceField( - queryset=ComponentInterface.objects.all(), - widget=Select2MultipleWidget, - ) algorithm_time_limit = IntegerField( widget=forms.HiddenInput(), disabled=True, diff --git a/app/grandchallenge/evaluation/migrations/0072_phasealgorithminterface_phase_algorithm_interfaces_and_more.py b/app/grandchallenge/evaluation/migrations/0072_phasealgorithminterface_phase_algorithm_interfaces_and_more.py new file mode 100644 index 0000000000..6e3e24e173 --- /dev/null +++ b/app/grandchallenge/evaluation/migrations/0072_phasealgorithminterface_phase_algorithm_interfaces_and_more.py @@ -0,0 +1,62 @@ +# Generated by Django 4.2.18 on 2025-01-29 10:23 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("algorithms", "0066_create_interfaces_for_jobs"), + ( + "evaluation", + "0071_alter_combinedleaderboardphase_unique_together_and_more", + ), + ] + + operations = [ + migrations.CreateModel( + name="PhaseAlgorithmInterface", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "interface", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="algorithms.algorithminterface", + ), + ), + ( + "phase", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="evaluation.phase", + ), + ), + ], + ), + migrations.AddField( + model_name="phase", + name="algorithm_interfaces", + field=models.ManyToManyField( + blank=True, + help_text="The interfaces that an algorithm for this phase must implement.", + through="evaluation.PhaseAlgorithmInterface", + to="algorithms.algorithminterface", + ), + ), + migrations.AddConstraint( + model_name="phasealgorithminterface", + constraint=models.UniqueConstraint( + fields=("phase", "interface"), + name="unique_phase_interface_combination", + ), + ), + ] diff --git a/app/grandchallenge/evaluation/migrations/0073_migrate_interfaces_for_phases.py b/app/grandchallenge/evaluation/migrations/0073_migrate_interfaces_for_phases.py new file mode 100644 index 0000000000..56a0e90bb7 --- /dev/null +++ b/app/grandchallenge/evaluation/migrations/0073_migrate_interfaces_for_phases.py @@ -0,0 +1,49 @@ +from django.db import migrations + +from grandchallenge.algorithms.models import ( + get_existing_interface_for_inputs_and_outputs, +) +from grandchallenge.evaluation.utils import SubmissionKindChoices + + +def add_algorithm_interfaces_for_phases(apps, _schema_editor): + Phase = apps.get_model("evaluation", "Phase") # noqa: N806 + AlgorithmInterface = apps.get_model( # noqa: N806 + "algorithms", "AlgorithmInterface" + ) + + for phase in ( + Phase.objects.filter(submission_kind=SubmissionKindChoices.ALGORITHM) + .prefetch_related("algorithm_inputs", "algorithm_outputs") + .all() + ): + inputs = phase.algorithm_inputs.all() + outputs = phase.algorithm_outputs.all() + + if not inputs or not outputs: + raise RuntimeError(f"{phase} is improperly configured.") + + io = get_existing_interface_for_inputs_and_outputs( + model=AlgorithmInterface, inputs=inputs, outputs=outputs + ) + if not io: + io = AlgorithmInterface.objects.create() + io.inputs.set(inputs) + io.outputs.set(outputs) + + phase.algorithm_interfaces.add(io) + + +class Migration(migrations.Migration): + dependencies = [ + ( + "evaluation", + "0072_phasealgorithminterface_phase_algorithm_interfaces_and_more", + ), + ] + + operations = [ + migrations.RunPython( + add_algorithm_interfaces_for_phases, elidable=True + ), + ] diff --git a/app/grandchallenge/evaluation/models.py b/app/grandchallenge/evaluation/models.py index f57bfe926b..9459947be9 100644 --- a/app/grandchallenge/evaluation/models.py +++ b/app/grandchallenge/evaluation/models.py @@ -24,6 +24,7 @@ from grandchallenge.algorithms.models import ( AlgorithmImage, + AlgorithmInterface, AlgorithmModel, Job, ) @@ -460,7 +461,12 @@ class Phase(FieldChangeMixin, HangingProtocolMixin, UUIDModel): "metrics.json over the API." ), ) - + algorithm_interfaces = models.ManyToManyField( + to=AlgorithmInterface, + through="evaluation.PhaseAlgorithmInterface", + blank=True, + help_text="The interfaces that an algorithm for this phase must implement.", + ) inputs = models.ManyToManyField( to=ComponentInterface, related_name="evaluation_inputs" ) @@ -843,7 +849,7 @@ def valid_metrics(self): def read_only_fields_for_dependent_phases(self): common_fields = ["submission_kind"] if self.submission_kind == SubmissionKindChoices.ALGORITHM: - common_fields += ["algorithm_inputs", "algorithm_outputs"] + common_fields += ["algorithm_interfaces"] return common_fields def _clean_parent_phase(self): @@ -1198,6 +1204,14 @@ def parent_phase_choices(self): ) ) + @property + def algorithm_interface_manager(self): + return self.algorithm_interfaces + + @property + def algorithm_interface_through_model_manager(self): + return PhaseAlgorithmInterface.objects.filter(phase=self) + class PhaseUserObjectPermission(UserObjectPermissionBase): content_object = models.ForeignKey(Phase, on_delete=models.CASCADE) @@ -1207,6 +1221,19 @@ class PhaseGroupObjectPermission(GroupObjectPermissionBase): content_object = models.ForeignKey(Phase, on_delete=models.CASCADE) +class PhaseAlgorithmInterface(models.Model): + phase = models.ForeignKey(Phase, on_delete=models.CASCADE) + interface = models.ForeignKey(AlgorithmInterface, on_delete=models.CASCADE) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["phase", "interface"], + name="unique_phase_interface_combination", + ), + ] + + class Method(UUIDModel, ComponentImage): """Store the methods for performing an evaluation.""" diff --git a/app/grandchallenge/evaluation/templates/evaluation/configure_algorithm_phases_form.html b/app/grandchallenge/evaluation/templates/evaluation/configure_algorithm_phases_form.html index 612cb945d4..7a62485ac5 100644 --- a/app/grandchallenge/evaluation/templates/evaluation/configure_algorithm_phases_form.html +++ b/app/grandchallenge/evaluation/templates/evaluation/configure_algorithm_phases_form.html @@ -20,6 +20,9 @@ {% block content %}

Configure algorithm submission phases

+

Select the phases you would like to turn into algorithm submission phases.

+

This will create an archive for each selected phase, and populate the inference time limit, gpu type choices and memory requirements based on the corresponding challenge request.

+

To configure algorithm interfaces for each phase, please visit the phase settings after this step.

{% crispy form %} {% endblock %} diff --git a/app/grandchallenge/evaluation/views/__init__.py b/app/grandchallenge/evaluation/views/__init__.py index dbb94d17c8..8edbcd4ce3 100644 --- a/app/grandchallenge/evaluation/views/__init__.py +++ b/app/grandchallenge/evaluation/views/__init__.py @@ -1037,8 +1037,6 @@ def form_valid(self, form): for phase in form.cleaned_data["phases"]: self.turn_phase_into_algorithm_phase( phase=phase, - inputs=form.cleaned_data["algorithm_inputs"], - outputs=form.cleaned_data["algorithm_outputs"], algorithm_time_limit=form.cleaned_data["algorithm_time_limit"], algorithm_selectable_gpu_type_choices=form.cleaned_data[ "algorithm_selectable_gpu_type_choices" @@ -1059,8 +1057,6 @@ def turn_phase_into_algorithm_phase( self, *, phase, - inputs, - outputs, algorithm_time_limit, algorithm_selectable_gpu_type_choices, algorithm_maximum_settable_memory_gb, @@ -1097,8 +1093,6 @@ def turn_phase_into_algorithm_phase( phase.submission_kind = phase.SubmissionKindChoices.ALGORITHM phase.creator_must_be_verified = True phase.save() - phase.algorithm_outputs.add(*outputs) - phase.algorithm_inputs.add(*inputs) class EvaluationGroundTruthCreate( diff --git a/app/tests/algorithms_tests/test_forms.py b/app/tests/algorithms_tests/test_forms.py index 7358a36cb8..3e2499fd05 100644 --- a/app/tests/algorithms_tests/test_forms.py +++ b/app/tests/algorithms_tests/test_forms.py @@ -1400,7 +1400,7 @@ def test_algorithm_for_phase_form_memory_limited(): def test_algorithm_interface_disjoint_interfaces(): ci = ComponentInterfaceFactory() form = AlgorithmInterfaceForm( - algorithm=AlgorithmFactory(), data={"inputs": [ci], "outputs": [ci]} + base_obj=AlgorithmFactory(), data={"inputs": [ci], "outputs": [ci]} ) assert form.is_valid() is False assert "The sets of Inputs and Outputs must be unique" in str(form.errors) @@ -1414,7 +1414,7 @@ def test_algorithm_interface_unique_inputs_required(): alg.interfaces.add(interface) form = AlgorithmInterfaceForm( - algorithm=alg, data={"inputs": [ci1], "outputs": [ci2]} + base_obj=alg, data={"inputs": [ci1], "outputs": [ci2]} ) assert form.is_valid() is False assert ( @@ -1462,7 +1462,7 @@ def test_existing_io_is_reused(self): alg = AlgorithmFactory() form = AlgorithmInterfaceForm( - algorithm=alg, + base_obj=alg, data={ "inputs": [inp.pk], "outputs": [out.pk], diff --git a/app/tests/evaluation_tests/test_admin.py b/app/tests/evaluation_tests/test_admin.py index 9037e67a3d..9779ea94ad 100644 --- a/app/tests/evaluation_tests/test_admin.py +++ b/app/tests/evaluation_tests/test_admin.py @@ -2,26 +2,10 @@ from grandchallenge.evaluation.admin import PhaseAdmin from grandchallenge.evaluation.utils import SubmissionKindChoices -from tests.components_tests.factories import ComponentInterfaceFactory from tests.evaluation_tests.factories import PhaseFactory from tests.factories import ChallengeFactory -@pytest.mark.django_db -def test_disjoint_interfaces(): - i = ComponentInterfaceFactory() - p = PhaseFactory(challenge=ChallengeFactory()) - form = PhaseAdmin.form( - instance=p, - data={"algorithm_inputs": [i.pk], "algorithm_outputs": [i.pk]}, - ) - assert form.is_valid() is False - assert ( - "The sets of Algorithm Inputs and Algorithm Outputs must be unique" - in str(form.errors) - ) - - @pytest.mark.django_db def test_read_only_fields_disabled(): p1, p2 = PhaseFactory.create_batch( @@ -34,8 +18,7 @@ def test_read_only_fields_disabled(): form = PhaseAdmin.form( instance=p1, ) - assert form.fields["algorithm_inputs"].disabled - assert form.fields["algorithm_outputs"].disabled + assert form.fields["algorithm_interfaces"].disabled assert form.fields["submission_kind"].disabled p3, p4 = PhaseFactory.create_batch( diff --git a/app/tests/evaluation_tests/test_models.py b/app/tests/evaluation_tests/test_models.py index 96ea6f9d45..2e8360efd5 100644 --- a/app/tests/evaluation_tests/test_models.py +++ b/app/tests/evaluation_tests/test_models.py @@ -1056,8 +1056,7 @@ def test_read_only_fields_for_dependent_phases(): ) assert p1.read_only_fields_for_dependent_phases == [ "submission_kind", - "algorithm_inputs", - "algorithm_outputs", + "algorithm_interfaces", ] assert p2.read_only_fields_for_dependent_phases == ["submission_kind"] diff --git a/app/tests/evaluation_tests/test_views.py b/app/tests/evaluation_tests/test_views.py index 5759c17528..91120b7fdc 100644 --- a/app/tests/evaluation_tests/test_views.py +++ b/app/tests/evaluation_tests/test_views.py @@ -1373,8 +1373,6 @@ def test_configure_algorithm_phases_view(client): }, data={ "phases": [phase.pk], - "algorithm_inputs": [ci1.pk], - "algorithm_outputs": [ci2.pk], }, ) assert response.status_code == 302 @@ -1385,8 +1383,6 @@ def test_configure_algorithm_phases_view(client): phase.archive.title == f"{phase.challenge.short_name} {phase.title} dataset" ) - assert list(phase.algorithm_inputs.all()) == [ci1] - assert list(phase.algorithm_outputs.all()) == [ci2] assert ( phase.algorithm_time_limit == challenge_request.inference_time_limit_in_minutes * 60 From d840750a99a2a96b9074051de493de53729b142c Mon Sep 17 00:00:00 2001 From: Anne Mickan Date: Wed, 5 Feb 2025 10:35:55 +0100 Subject: [PATCH 13/18] Managements views for algorithm interfaces for phases (#3812) This adds the management views for algorithm interfaces for phases. The views are only accessible to staff users, and only for algorithm submission phases that are not external. The logic behind the views is the same as for the algorithm interfaces, but it only made sense to refactor the create view logic, and parts of the templates. ![image](https://github.com/user-attachments/assets/b9c3d86b-fd2e-4ee4-a558-7bef40f15ab5) Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/153 --- app/config/urls/challenge_subdomain.py | 4 + app/grandchallenge/algorithms/models.py | 14 ++ .../algorithms/algorithm_detail.html | 3 + ...ithmalgorithminterface_confirm_delete.html | 21 +- .../algorithmalgorithminterface_list.html | 35 +--- .../algorithms/algorithminterface_form.html | 25 +-- .../algorithminterface_confirm_delete.html | 20 ++ .../partials/algorithminterface_form.html | 15 ++ .../partials/algorithminterface_list.html | 36 ++++ app/grandchallenge/algorithms/views.py | 31 ++- app/grandchallenge/evaluation/models.py | 18 ++ .../algorithminterface_for_phase_form.html | 26 +++ ...hasealgorithminterface_confirm_delete.html | 26 +++ .../phasealgorithminterface_list.html | 23 +++ app/grandchallenge/evaluation/urls.py | 70 ++++--- .../evaluation/views/__init__.py | 102 ++++++++- .../templates/pages/phase_menu_sidebar.html | 14 +- app/tests/evaluation_tests/test_views.py | 194 +++++++++++++++++- 18 files changed, 564 insertions(+), 113 deletions(-) create mode 100644 app/grandchallenge/algorithms/templates/algorithms/partials/algorithminterface_confirm_delete.html create mode 100644 app/grandchallenge/algorithms/templates/algorithms/partials/algorithminterface_form.html create mode 100644 app/grandchallenge/algorithms/templates/algorithms/partials/algorithminterface_list.html create mode 100644 app/grandchallenge/evaluation/templates/evaluation/algorithminterface_for_phase_form.html create mode 100644 app/grandchallenge/evaluation/templates/evaluation/phasealgorithminterface_confirm_delete.html create mode 100644 app/grandchallenge/evaluation/templates/evaluation/phasealgorithminterface_list.html diff --git a/app/config/urls/challenge_subdomain.py b/app/config/urls/challenge_subdomain.py index 6c0b84b24f..07fff57d58 100644 --- a/app/config/urls/challenge_subdomain.py +++ b/app/config/urls/challenge_subdomain.py @@ -15,6 +15,10 @@ ), name="subdomain_robots_txt", ), + path( + "components/", + include("grandchallenge.components.urls", namespace="components"), + ), path( "evaluation/", include("grandchallenge.evaluation.urls", namespace="evaluation"), diff --git a/app/grandchallenge/algorithms/models.py b/app/grandchallenge/algorithms/models.py index 77666071e1..4345deb9cd 100644 --- a/app/grandchallenge/algorithms/models.py +++ b/app/grandchallenge/algorithms/models.py @@ -528,6 +528,20 @@ def algorithm_interface_manager(self): def algorithm_interface_through_model_manager(self): return AlgorithmAlgorithmInterface.objects.filter(algorithm=self) + @property + def algorithm_interface_create_url(self): + return reverse( + "algorithms:interface-create", kwargs={"slug": self.slug} + ) + + @property + def algorithm_interface_delete_viewname(self): + return "algorithms:interface-delete" + + @property + def algorithm_interface_list_url(self): + return reverse("algorithms:interface-list", kwargs={"slug": self.slug}) + def is_editor(self, user): return user.groups.filter(pk=self.editors_group.pk).exists() diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html index ded523c980..380a29a4f4 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithm_detail.html @@ -39,6 +39,9 @@  Interfaces + {% if not object.interfaces.all %}  + + {% endif %} Confirm Algorithm Interface Deletion -
- {% csrf_token %} -

Are you sure that you want to delete the following algorithm interface from your algorithm?

-
    -
  • Inputs: {{ object.interface.inputs.all|oxford_comma }}
  • -
  • Outputs: {{ object.interface.outputs.all|oxford_comma }}
  • -
-

- WARNING: You are not able to undo this action. Once the interface is deleted, it is deleted forever. -

-
Cancel - -
- + {% include 'algorithms/partials/algorithminterface_confirm_delete.html' with base_obj=algorithm %} {% endblock %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html index 1efb894bb7..883c635a44 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithmalgorithminterface_list.html @@ -1,6 +1,4 @@ {% extends "base.html" %} -{% load crispy_forms_tags %} -{% load remove_whitespace %} {% block title %} Interfaces - {{ algorithm.title }} - {{ block.super }} @@ -22,36 +20,5 @@ {% endblock %} {% block content %} -

Algorithm Interfaces for {{ algorithm }}

- -

- The following interfaces are configured for your algorithm: -

-

Add new interface

- -
- - - - - - - - {% for object in object_list %} - - - - - - {% empty %} - - {% endfor %} - -
InputsOutputsDelete
{{ object.interface.inputs.all|oxford_comma }}{{ object.interface.outputs.all|oxford_comma }} - - - -
This algorithm does not have any interfaces defined yet.
-
+ {% include 'algorithms/partials/algorithminterface_list.html' with base_obj=algorithm %} {% endblock %} diff --git a/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html b/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html index 356412ffa0..02614e610f 100644 --- a/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html +++ b/app/grandchallenge/algorithms/templates/algorithms/algorithminterface_form.html @@ -1,8 +1,7 @@ {% extends "base.html" %} -{% load crispy_forms_tags %} {% block title %} - Create Interface - {{ algorithm.title }} - {{ block.super }} + Create Interface - {{ base_obj.title }} - {{ block.super }} {% endblock %} {% block breadcrumbs %} @@ -10,10 +9,10 @@