fix: Skip resource checking for unmanaged exp #9372
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9372 +/- ##
=======================================
Coverage 46.07% 46.07%
=======================================
Files 1228 1228
Lines 155900 155902 +2
Branches 2439 2439
=======================================
+ Hits 71837 71838 +1
- Misses 83872 83873 +1
Partials 191 191
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
master/internal/core_experiment.go
Outdated
| slotsPerTrial = 0 | ||
| } | ||
|
|
||
| poolName, _, err := m.ResolveResources(resources.ResourcePool(), slotsPerTrial, workspaceID, isSingleNode) |
There was a problem hiding this comment.
is there any reason to call resolve resources at all if the experiment is unmanaged? e.g., we have a very similar bug if someone passes a non-existent resource pool.
There was a problem hiding this comment.
i guess what makes this one so painful is that the default is 1 slot which doesn't work
There was a problem hiding this comment.
+1, haran hit this recently as well: we don't need to validate resource pool names for unmanaged.
gt2345
changed the title
master/internal/core_experiment.go
Outdated
| taskContainerDefaults, err := m.rm.TaskContainerDefaults( | ||
| poolName, | ||
| m.config.TaskContainerDefaults, | ||
| ) | ||
| if err != nil { | ||
| return nil, nil, config, nil, nil, errors.Wrapf(err, "error getting TaskContainerDefaults") | ||
| } | ||
| taskSpec.TaskContainerDefaults = taskContainerDefaults | ||
| taskSpec.TaskContainerDefaults.MergeIntoExpConfig(&config) |
There was a problem hiding this comment.
I am not sure we need to put this under unmanaged check. I think it's supposed to work for unmanaged experiments, and having properly merged spec might become helpful if/when we want the master to have access to checkpoints produced by the unmanaged experiment, e.g. for downloads or GC. LGTM otherwise.
There was a problem hiding this comment.
Sure, I've moved taskContainerDefaults out of the condition
gt2345
force-pushed
the
gt/385-unmanaged-resource
branch
from
85b1281
to
a61f606
Compare
Ticket
MD-385
Description
For unmanaged experiments, the slots per trial should be 0 since we are not spending any resource for it.
Test Plan
Create a master without any agent connection, run unmanaged experiments and the experiments complete successfully even though there is no computing resource available
Checklist
docs/release-notes/.See Release Note for details.