Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Budget alerts #3747

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions app/grandchallenge/challenges/emails.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,24 @@ def send_challenge_status_update_email(challengerequest, challenge=None):
recipients=[challengerequest.creator],
subscription_type=EmailSubscriptionTypes.SYSTEM,
)


def send_email_percent_budget_consumed_alert(challenge, warning_threshold):
send_standard_email_batch(
site=Site.objects.get_current(),
subject=format_html(
"[{challenge_name}] Challenge {warning_threshold}% Budget Consumed Alert",
koopmant marked this conversation as resolved.
Show resolved Hide resolved
challenge_name=challenge.short_name,
warning_threshold=warning_threshold,
),
markdown_message=format_html(
"We would like to inform you that {percent_budget_consumed}% of the "
"compute budget for your challenge has been used.",
percent_budget_consumed=challenge.percent_budget_consumed,
),
recipients=[
*challenge.get_admins(),
koopmant marked this conversation as resolved.
Show resolved Hide resolved
*get_user_model().objects.filter(is_staff=True, is_active=True),
koopmant marked this conversation as resolved.
Show resolved Hide resolved
],
subscription_type=EmailSubscriptionTypes.SYSTEM,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Generated by Django 4.2.17 on 2024-12-11 10:16

from django.db import migrations, models

import grandchallenge.challenges.models
import grandchallenge.core.validators


class Migration(migrations.Migration):
dependencies = [
(
"challenges",
"0045_challengerequest_algorithm_maximum_settable_memory_gb_and_more",
),
]

operations = [
migrations.AddField(
model_name="challenge",
name="percent_budget_consumed_warning_thresholds",
field=models.JSONField(
default=grandchallenge.challenges.models.get_default_percent_budget_consumed_warning_thresholds,
validators=[
grandchallenge.core.validators.JSONValidator(
schema={
"$schema": "http://json-schema.org/draft-07/schema",
"items": {"type": "integer"},
"type": "array",
"uniqueItems": True,
}
)
],
),
),
]
24 changes: 21 additions & 3 deletions app/grandchallenge/challenges/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
GPUTypeChoices,
get_default_gpu_type_choices,
)
from grandchallenge.core.models import UUIDModel
from grandchallenge.core.models import FieldChangeMixin, UUIDModel
from grandchallenge.core.storage import (
get_banner_path,
get_logo_path,
Expand Down Expand Up @@ -211,7 +211,11 @@ class Meta:
abstract = True


class Challenge(ChallengeBase):
def get_default_percent_budget_consumed_warning_thresholds():
return [70, 90, 100]


class Challenge(ChallengeBase, FieldChangeMixin):
created = models.DateTimeField(auto_now_add=True)
modified = models.DateTimeField(auto_now=True)
description = models.CharField(
Expand Down Expand Up @@ -376,6 +380,20 @@ class Challenge(ChallengeBase):
help_text="This email will be listed as the contact email for the challenge and will be visible to all users of Grand Challenge.",
)

percent_budget_consumed_warning_thresholds = models.JSONField(
default=get_default_percent_budget_consumed_warning_thresholds,
validators=[
JSONValidator(
schema={
"$schema": "http://json-schema.org/draft-07/schema",
"type": "array",
"items": {"type": "integer"},
"uniqueItems": True,
}
)
],
)

accumulated_compute_cost_in_cents = deprecate_field(
models.IntegerField(default=0, blank=True)
)
Expand Down Expand Up @@ -707,7 +725,7 @@ def should_be_open_but_is_over_budget(self):
for phase in self.phase_set.all()
)

@cached_property
@property
koopmant marked this conversation as resolved.
Show resolved Hide resolved
def percent_budget_consumed(self):
if self.approved_compute_costs_euro_millicents:
return int(
Expand Down
27 changes: 26 additions & 1 deletion app/grandchallenge/challenges/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
annotate_compute_costs_and_storage_size,
annotate_job_duration_and_compute_costs,
)
from grandchallenge.challenges.emails import (
send_email_percent_budget_consumed_alert,
)
from grandchallenge.challenges.models import Challenge
from grandchallenge.core.celery import acks_late_2xlarge_task
from grandchallenge.evaluation.models import Evaluation, Phase
Expand Down Expand Up @@ -55,12 +58,34 @@ def update_challenge_results_cache():
)


def send_alert_if_budget_consumed_warning_threshold_exceeded(challenge):
if (
challenge.has_changed("compute_cost_euro_millicents")
and challenge.approved_compute_costs_euro_millicents
):
for threshold in sorted(
challenge.percent_budget_consumed_warning_thresholds, reverse=True
):
if (
challenge.initial_value("compute_cost_euro_millicents")
< challenge.approved_compute_costs_euro_millicents
* threshold
/ 100
<= challenge.compute_cost_euro_millicents
):
Copy link
Member

Choose a reason for hiding this comment

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

I find this if statement hard to grok. Can you maybe assign some variables to help with readability? Style wise, it is usually easiest for other developers to have everything laid out for them, and the if statement be quite straightforward:

if should_do_something:
    do_something()

then should_do_something is built up through logic:

it_is_sunday = datetime.datetime.now().weekday() == 6
feeling_under_the_weather = random.random() < 0.05

should_do_something = not it_is_sunday and not feeling_under_the_weather

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: I also find that hard to read. Black did not do this statement justice. All the operators "seem to be equal" even though this is an example of the rare ternary comparison... hard to parse!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry about this one. I was optimizing the code and it got a bit out of hand. And then, as Paul said, Black came in and made matters even worse... I'll clean it up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be a lot better now.

send_email_percent_budget_consumed_alert(challenge, threshold)
break


@acks_late_2xlarge_task
def update_compute_costs_and_storage_size():
challenges = Challenge.objects.all()

for challenge in challenges:
for challenge in challenges.with_available_compute():
annotate_compute_costs_and_storage_size(challenge=challenge)
send_alert_if_budget_consumed_warning_threshold_exceeded(
koopmant marked this conversation as resolved.
Show resolved Hide resolved
challenge=challenge
)

Challenge.objects.bulk_update(
challenges,
Expand Down
55 changes: 52 additions & 3 deletions app/tests/challenges_tests/test_tasks.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
import pytest
from django.core import mail

from grandchallenge.challenges.models import Challenge, ChallengeRequest
from grandchallenge.challenges.tasks import update_challenge_results_cache
from tests.evaluation_tests.factories import EvaluationFactory
from tests.factories import ChallengeFactory, ChallengeRequestFactory
from grandchallenge.challenges.tasks import (
update_challenge_results_cache,
update_compute_costs_and_storage_size,
)
from grandchallenge.invoices.models import PaymentStatusChoices
from tests.evaluation_tests.factories import EvaluationFactory, PhaseFactory
from tests.factories import (
ChallengeFactory,
ChallengeRequestFactory,
UserFactory,
)
from tests.invoices_tests.factories import InvoiceFactory


@pytest.mark.django_db
Expand Down Expand Up @@ -121,3 +131,42 @@ def test_challenge_request_budget_calculation(settings):
+ challenge_request.budget["Total phase 2"]
+ challenge_request.budget["Docker storage cost"]
)


@pytest.mark.django_db
def test_challenge_budget_alert_email():
koopmant marked this conversation as resolved.
Show resolved Hide resolved
challenge = ChallengeFactory(
short_name="test",
)
challenge_admin = UserFactory()
challenge.add_admin(challenge_admin)
staff_user = UserFactory(is_staff=True)
InvoiceFactory(
challenge=challenge,
support_costs_euros=0,
compute_costs_euros=10,
storage_costs_euros=0,
payment_status=PaymentStatusChoices.PAID,
)
phase = PhaseFactory(challenge=challenge)
EvaluationFactory(
submission__phase=phase,
compute_cost_euro_millicents=800000,
time_limit=60,
)
update_compute_costs_and_storage_size()
koopmant marked this conversation as resolved.
Show resolved Hide resolved
assert len(mail.outbox) == 3
recipients = [r for m in mail.outbox for r in m.to]
assert recipients == [
challenge.creator.email,
challenge_admin.email,
staff_user.email,
]
assert (
mail.outbox[0].subject
== "[testserver] [test] Challenge 70% Budget Consumed Alert"
)
assert (
"We would like to inform you that 80% of the compute budget for your challenge has been used."
in mail.outbox[0].body
)
Loading