Enable budget calculation for more than two phases

[koopmant]

New fields have been added to the ChallengeRequest model to define any number of phases for any number of tasks for the budget calculation.

The ChallengeRequestBudgetUpdateForm has been updated to allow changing these new fields. The new fields are initially filled with values from the old fields that are used by the ChallengeRequestCreateForm, which has not changed from the users perspective.

Because the challenge request can now define multiple tasks, runtime limits are set for the phases by supplying the corresponding task when they are converted to algorithm phases; unless there is only one task defined on the challenge request, then the input is not needed.

Related to https://github.com/DIAGNijmegen/rse-roadmap/issues/421
Related to #3787

[koopmant]

@jmsmkn I don't think it is worth splitting up the PR or creating stacked commits, but let me know if this is annoying to review.

[jmsmkn]

+1800 LOC is going to be too much for me to review this afternoon, sorry.

[koopmant]

No worries, let me see if I can present this in chunks

[koopmant]

Cleaned it into 6 commits. Fixed some issues along the way, so it's good that you did not review it before.

Should be ready now!

[chrisvanrun]

Sorry! Heavy conflict with #4495. We'll need to discuss how to combine this and what makes more sense to merge first.

[koopmant]

Ah, this get's automatically closed when the branch is up-to-date with main. I wanted to use this as a base for a feature branch with stacked PR's. Will open it again after the first PR.

[koopmant]

@chrisvanrun I'm replying to your comment here, as this will be the final PR.

I suggest we use this PR to discuss the details.

The tasks were originally meant as a quick-n-dirty multiplier. In addition, the phase-setup form is mostly a time-saving construct for us not to have to input all those values manually.

I think making some of the fields multi-dimensional (i.e. creating an array of those values) might be a quick pattern that will work in the short term but will likely bite us in the future.

Writing out-loud here: what if we create a ChallengeRequestPhase object, with all the typical per-phase configurable fields (i.e. runtime, maximum settings, et cetera) and we include some estimation parameters. That would best match what it is meant to model: the predicted run through of a single Phase. It would automatically capture tasks and could be used for all kind of future improvements. Setting up a Phase would involve selecting one of the ChallengeRequestPhase to set it up.

How do you think this will bite us in the future?

I don't follow how adding a new ChallengeRequestPhase model is going to automatically capture tasks. A task can have multiple phases and the budget depends on which task a phase is a part of, mainly because submissions to phases of the same tasks should not be counted separately, since a submissions to the final phase should already be submitted to earlier phases. I'd expect a ChallengeRequestTask would be necessary in that case. But either way, it would be way more complicated than this. I remember talking with James about adding a Task model and we agreed we shouldn't add models unless we absolutely have to. I don't think that is the case here.

[chrisvanrun]

I don't follow how adding a new ChallengeRequestPhase model is going to automatically capture tasks. A task can have multiple phases and the budget depends on which task a phase is a part of, mainly because submissions to phases of the same tasks should not be counted separately, since a submissions to the final phase should already be submitted to earlier phases.

In the idea that tasks are a 'simple' combination of phases. However, I had not taken into consideration that tasks are also unifying some costs related with submissions. That makes the modelling quite a bit more difficult!

In the future, we might want to add additional properties which might be easier added when modelled instead of increasing dimensionality in some of the fields. But I'd have to have a closer look at how the dimensionality is handled in your changes.

[chrisvanrun]

A quick study has me sketch your implementation as follows, please let me know if it is not correct.

Per Task

• inference_time_average_minutes_for_tasks - Average run time per algorithm job in minutes, for each task
• algorithm_selectable_gpu_type_choices_for_tasks - GPU type choices for algorithm inference jobs, for each task
• algorithm_maximum_settable_memory_gb_for_tasks - Maximum settable memory in GB, for each task
• average_size_test_case_mb_for_tasks - Average size of test image in MB, for each task

Per Phase

• number_of_teams_for_phases - Number of teams for each phase
• number_of_submissions_per_team_for_phases - Number of submissions per team for each phase
• number_of_test_cases_for_phases - Number of test images for each phase
• task_id_for_phases - Indicates which phase belongs to which task

Other

• task_ids - List of task IDs (not per-task or per-phase, but defines the tasks themselves)

We keep the main relationship between task and phases via task_id_for_phases. I like how we have everything in a single query. I do worry about maintainability and data integrity. Adding another field or relation becomes a bit of a hassle, which we'd get more easily done using Django ORM. However, I don't think we'll be adding a relation, since phases and tasks are sort of at the core of the challenge setup. Data integrity goes via array indexing and isn't currently checked. At the other hand, I don't think we have sufficient appetite to make this a model architecture: given the work already poured into this.

Hence, I suggest tackling the integrity and check the fields on the model:

• ensuring the tasks indices exist when referenced from task_id_for_phases
• ensuring equal length of each of these fields in the per-phase and per-task scope

[koopmant]

A quick study has me sketch your implementation as follows, please let me know if it is not correct.

That is all correct.

ensuring the tasks indices exist when referenced from task_id_for_phases
ensuring equal length of each of these fields in the per-phase and per-task scope

I believe I have these covered in the ChallengeRequestBudgetUpdateForm. In particular in _clean_task_id_for_phases, _clean_task_lists_equal_length, and self._clean_phases_lists_equal_length.

[chrisvanrun]

I believe I have these covered in the ChallengeRequestBudgetUpdateForm. In particular in _clean_task_id_for_phases, _clean_task_lists_equal_length, and self._clean_phases_lists_equal_length.

Ah, darn. Sorry! I used a reference lookup on the model.field, but that of course doesn't hit the form. Silly me.

One classic argument is moving those to the model so we don't end-up with integrity problems if we edit via the backend. Relevant, since we are maybe planning on removing the budget fields from the requests. However, I think we'll likely want to work on this via the frontend form anyway, even if we remove those. So then the argument is mute. Do you agree?

[koopmant]

Yes, the budget update form is for us to change it through the frontend. I think it is fine to keep the validation there.

[chrisvanrun]

It is alive again!

[chrisvanrun]

I'll double-check this as a whole one more time tomorrow!

[koopmant]

One test was duplicated because I hadn't rebased before merging #4504. I carefully backtracked, but that was the only difference. Stacked PRs and squashed merge commits are a total nightmare.

[chrisvanrun]

Stacked PRs and squashed merge commits are a total nightmare.

Yes, maybe we should not squash them when the stacking is 'merged down' and only do a final squash at the final merge into main? @jmsmkn would it make sense to allow that?

[chrisvanrun]

One final scan complete!

[jmsmkn]

Stacked PRs and squashed merge commits are a total nightmare.

Yes, maybe we should not squash them when the stacking is 'merged down' and only do a final squash at the final merge into main? @jmsmkn would it make sense to allow that?

That would then allow non-squashed commits into main, so is a no-go. PRs should be made to main that are small and reviewable.

[koopmant]

That would then allow non-squashed commits into main, so is a no-go.

Can we allow non-squashed merges if main is not the target branch?

PRs should be made to main that are small and reviewable.

Generally, I think this is the way to go. Stacked PR's are a slippery slope that lead to work that is hard to review. Just because it is split up, doesn't always make it easier to review. @chrisvanrun I think you'll agree?

[chrisvanrun]

Generally, I think this is the way to go. Stacked PR's are a slippery slope that lead to work that is hard to review. Just because it is split up, doesn't always make it easier to review. @chrisvanrun I think you'll agree?

Tricky. In this case it was all the same change (in essence) but it propagated to different parts of the code base. Any split would have broken main, so I think a feature branch was the obvious choice for creating smaller PRs.

Splitting a large PR into smaller PRs post hoc, like with this change set, always runs into problems with upstream changes and squashes. IMO - optimally you work up to a small change set that makes some sense as a whole, push it out in a PR for reviewing and then work on the next change set. And then make sure to never work on the next step after that, and consider to be blocked when the first PR is stuck in reviewing.