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

Budget alerts #3747

wants to merge 17 commits into from

Conversation

koopmant
Copy link
Contributor

@koopmant koopmant self-assigned this Dec 12, 2024
Comment on lines 69 to 75
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.

Copy link
Contributor

@amickan amickan left a comment

Choose a reason for hiding this comment

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

LGTM. Just some small comments.

I do have one general question: we're only taking into account compute costs here, not storage costs. I see that this is also what is checked in percent_budget_consumed and is what is displayed on the cost overview. Yet, with every new submission, we presumably also store a new Docker image, which affects the storage costs. Are the Docker storage costs usually overestimated on the invoice and hence hard to exceed?

@amickan
Copy link
Contributor

amickan commented Dec 13, 2024

Oh I see that James and I looked at this in parallel. Go with James' comments, obviously. ;-)

@jmsmkn
Copy link
Member

jmsmkn commented Dec 13, 2024

Oh I see that James and I looked at this in parallel. Go with James' comments, obviously. ;-)

Yes it seems like the review tab does not auto update like the comments one. Luckily (or rather unsurprisingly) we gave the same direction!

@jmsmkn
Copy link
Member

jmsmkn commented Dec 13, 2024

I do have one general question: we're only taking into account compute costs here, not storage costs. I see that this is also what is checked in percent_budget_consumed and is what is displayed on the cost overview. Yet, with every new submission, we presumably also store a new Docker image, which affects the storage costs. Are the Docker storage costs usually overestimated on the invoice and hence hard to exceed?

We should take into account storage but do not. We have the old problem that the two fields are separate (like how we used to assign budget/submissions per phase). I don't think there is a reason to consider them separately now. There is the problem that the storage costs are an ongoing rate, whereas the compute costs are fixed in time. We don't have per-month tracking of storage costs, I think to really make it work we would need that.

@koopmant koopmant requested review from jmsmkn and amickan December 18, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants