fix: set max slots and checkpoint gc policy should comply with config policies

[amandavialva01]

Ticket

CM-590

Description

This PR fixes a couple of issues:

• det e set max-slots to comply with invariant config and constraints
• det e set gc-policy to comply with invariant configs

Test Plan

Checklist

• Changes have been manually QA'd
• New features have been approved by the corresponding PM
• User-facing API changes have the "User-facing API Change" label
• Release notes have been added as a separate file under docs/release-notes/
  See Release Note for details.
• Licenses have been included for new code which was copied and/or modified from any external code

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 54.77%. Comparing base (782f7a0) to head (caa9179).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
master/internal/api_experiment.go	92.59%	2 Missing ⚠️
master/internal/configpolicy/utils.go	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10140      +/-   ##
==========================================
+ Coverage   54.71%   54.77%   +0.06%     
==========================================
  Files        1266     1266              
  Lines      159970   159986      +16     
  Branches     3662     3661       -1     
==========================================
+ Hits        87525    87637     +112     
+ Misses      72312    72216      -96     
  Partials      133      133              

Flag	Coverage Δ
backend	46.20% <91.66%> (+0.18%)	⬆️
harness	72.56% <ø> (ø)
web	54.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ternal/configpolicy/postgres_task_config_policy.go	91.13% <100.00%> (ø)
master/internal/experiment.go	35.93% <100.00%> (+2.15%)	⬆️
master/internal/api_experiment.go	61.23% <92.59%> (+4.31%)	⬆️
master/internal/configpolicy/utils.go	73.20% <80.00%> (-0.85%)	⬇️

... and 3 files with indirect coverage changes

[netlify]

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	caa9179
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6720ec3c4a5c54000827db03

[stoksc]

patch experiment really sketches me out. e.SetGroupWeight(*newResources.Weight) and all call e.db.SaveExperimentConfig(e.ID, e.activeConfig) and so does the endpoint itself. i feel like there are definitely calls where i could make the exp in memory state and db state inconsistent..?

[amandavialva01]

patch experiment really sketches me out. e.SetGroupWeight(*newResources.Weight) and all call e.db.SaveExperimentConfig(e.ID, e.activeConfig) and so does the endpoint itself. i feel like there are definitely calls where i could make the exp in memory state and db state inconsistent..?

hmm yea good point.
Is the solution then to just add that func into setMaxSlots? instead of moving it in the patch handler? its the only of the three (max-slots, priority, weight) that doesn't have this call

[kkunapuli]

This PR changes/undoes some of the changes you just made in your previous PR ... why is that?

[amandavialva01]

This was my bad, when I made the previous PR I didn't realize that a lot of the code in the PATCH experiment API handler is specifically for det e set CLI commands, and relied too heavily on integration tests of the functions I implemented to use in the PATCH handler, rather than testing the handler itself.
After manually testing PATCH requests resulting from those commands and adding automated integration testing for such requests, I think these changes should properly address any remaining issues/inconsistencies with expected config policy-related behavior in this API handler

[stoksc]

i'm still slightly fuzzy here. just to make sure we're all on the same page.. resources.max_slots/slots/slots_per_trial are all different but we're ok with constraints.max_slots controlling all of them? in which case.. maybe if there is a constraint or invariant for max slots it should always get set here, because otherwise an experiment could exceed max slots? once again i confused the check when patched and the check when launched.

[amandavialva01]

Yea! This is just for patching the resources.max_slots field specifically. Somewhere in the request validation, it could be useful to check resouces.slots or resouces.slots_per_trial in this case to make sure it doesn't exceed the requested resources.max_slots, but I think this goes back to the conversation of not wanting a constraint to alter configs? I think it could be worth checking in this API handler that resources.max_slots >= resources.slots_per_trial in the experiment config, but that kinda goes beyond intended fixes for this ticket
Happy to add it here either way though, since a check like that does make sense!

[amandavialva01]

I wanna note that technically, users can still override the name and description of an experiment with det e set name and det e set description, but I think that should be allowed, @kkunapuli wdyt?

[kkunapuli]

I wanna note that technically, users can still override the name and description of an experiment with det e set name and det e set description, but I think that should be allowed, @kkunapuli wdyt?

Yeah, I agree.

[stoksc]

this comment seems removable to me? or maybe should go someone else ha.. def is a stray at this point though.

[amandavialva01]

Removed this!