chore: experiment config slots to comply with constraint max slots #10054
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10054 +/- ##
==========================================
- Coverage 54.42% 54.41% -0.01%
==========================================
Files 1262 1262
Lines 158880 158888 +8
Branches 3631 3631
==========================================
- Hits 86463 86461 -2
- Misses 72283 72293 +10
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
amandavialva01
changed the title
| @@ -94,7 +94,7 @@ func ValidateExperimentConfig( | |||
| if cp.InvariantConfig.RawResources != nil { | |||
| checkAgainstGlobalPriority(priorityEnabledErr, cp.InvariantConfig.RawResources.RawPriority) | |||
| if err := checkConstraintConflicts(cp.Constraints, cp.InvariantConfig.RawResources.RawMaxSlots, | |||
| cp.InvariantConfig.RawResources.RawSlotsPerTrial, cp.InvariantConfig.RawResources.RawPriority); err != nil { | |||
| cp.InvariantConfig.RawResources.RawSlots, cp.InvariantConfig.RawResources.RawPriority); err != nil { | |||
There was a problem hiding this comment.
I don't understand this change. Experiments use slots per trial, not slots, right?
There was a problem hiding this comment.
You cannot set slots in the experiment config; you'll get an error. We should check that SlotsPerTrial is within MaxSlots ?
Does that make sense?
There was a problem hiding this comment.
If that's the case, @amandavialva01 can you also update the logic here to use slots per trial?
There was a problem hiding this comment.
Ooh my mistake! Changed this. I think the actual discrepancy was coming from the < / > issue that Kristine mentioned
There was a problem hiding this comment.
Synced with Bradley - for NTSC we'll compare "slots" to "constraints.resources.max_slots" and for experiments we'll compare "slotsPerTrial" to "constraints.resources.max_slots".
There was a problem hiding this comment.
Makes sense! Yea I just saw the comment in the ResourcesConfig struct, somehow glossed over that before lol
amandavialva01
force-pushed
the
amanda/fixConfigComplyWithConstraints
branch
2 times, most recently
from
cf10e8e
to
475fc41
Compare
amandavialva01
force-pushed
the
amanda/fixConfigComplyWithConstraints
branch
from
475fc41
to
655f635
Compare
| @@ -309,3 +310,45 @@ func TestUnmarshalJSONNTSC(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestCheckConstraintsConflicts(t *testing.T) { | |||
Ticket
CM-569
Description
Experiment config slots to comply with constraint max slots.
Test Plan
CI Passes.
Checklist
docs/release-notes/See Release Note for details.