Update budget based on gpu type and memory

[koopmant]

GPU type choices and maximum memory for algorithm inference jobs are now included in challenge requests. This information is used in the budget calculation and also passed down to the challenge phases when they are converted to Algorithm submission phases.

Related to https://github.com/DIAGNijmegen/rse-roadmap/issues/346

[koopmant]

I'm putting this up for review, but it's not complete yet. I'm running into the issue that the NO_GPU option becomes unselected after saving the ChallengeRequestBudgetUpdateForm. The option is correctly saved when creating a request. (['', 'T4']) When the budget update form is loaded, the option is still selected on the form. However, when the update form is saved, the data has the NO_GPU option removed. ('T4'])
I'm currently working on a fix.

(Will fix the failing tests regarding credits as well.)

[amickan]

When the budget update form is loaded, the option is still selected on the form. However, when the update form is saved, the data has the NO_GPU option removed. ('T4'])

Sounds like an HTMX issue / bug to me where empty values are not sent along with POST requests, or did you already find the reason / a fix for it?

[koopmant]

When the budget update form is loaded, the option is still selected on the form. However, when the update form is saved, the data has the NO_GPU option removed. ('T4'])

Sounds like an HTMX issue / bug to me where empty values are not sent along with POST requests, or did you already find the reason / a fix for it?

Feels like it indeed. Haven't found a fix yet.

[koopmant]

When the budget update form is loaded, the option is still selected on the form. However, when the update form is saved, the data has the NO_GPU option removed. ('T4'])

Sounds like an HTMX issue / bug to me where empty values are not sent along with POST requests, or did you already find the reason / a fix for it?

It indeed appears to be an issue with HTMX. Specifically, it appears HTMX is excluding the empty string checkbox value when other values in the same group exist, treating it as redundant.
The POST data after a create request includes empty string:
algorithm_selectable_gpu_type_choices:
algorithm_selectable_gpu_type_choices: T4
But the POST data after an update request with HTMX does not:
algorithm_selectable_gpu_type_choices: T4
Unless it is the only checkbox that is checked:
algorithm_selectable_gpu_type_choices:
If have made a workaround for it and it's working. I've changed the "" value in the choices with "no_gpu" and manually swap these during instantiation and in cleanup. I'm happy for suggestions if this can be done in a better way.

[amickan]

If have made a workaround for it and it's working. I've changed the "" value in the choices with "no_gpu" and manually swap these during instantiation and in cleanup. I'm happy for suggestions if this can be done in a better way.

Is there a reason that the NO_GPU option in the enum is ""? If not, I think updating the enum would be the way to go. Then you don't need a special work around.

[jmsmkn]

If have made a workaround for it and it's working. I've changed the "" value in the choices with "no_gpu" and manually swap these during instantiation and in cleanup. I'm happy for suggestions if this can be done in a better way.

Is there a reason that the NO_GPU option in the enum is ""? If not, I think updating the enum would be the way to go. Then you don't need a special work around.

That would involve a data migration to many fields, and is not how Django represents a blank choice everywhere else. It was chosen to be "" for framework consistency.

[jmsmkn]

This doesn't implement the setting of these values when phases are converted to Algorithm phases, I think? Is that intentional and coming in another PR?

[koopmant]

This doesn't implement the setting of these values when phases are converted to Algorithm phases, I think? Is that intentional and coming in another PR?

That was intended for another PR indeed. But it's a very small thing. I could add it here if you don't mind.

[jmsmkn]

This doesn't implement the setting of these values when phases are converted to Algorithm phases, I think? Is that intentional and coming in another PR?

That was intended for another PR indeed. But it's a very small thing. I could add it here if you don't mind.

Completely up to you.

[koopmant]

Ok, I've included setting the values on the phases when they are converted.

[jmsmkn]

Great, can be merge once the tests are fixed.