fix: don't ignore failed exps #9693
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9693 +/- ##
==========================================
- Coverage 54.29% 54.27% -0.03%
==========================================
Files 1261 1261
Lines 155642 155646 +4
Branches 3536 3536
==========================================
- Hits 84510 84473 -37
- Misses 70994 71035 +41
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more.
|
carolinaecalderon
force-pushed
the
carolinac/rm-70
branch
from
ff23411
to
918bb13
Compare
carolinaecalderon
requested review from
kkunapuli,
azhou-determined and
NicholasBlaskey
carolinaecalderon
force-pushed
the
carolinac/rm-70
branch
2 times, most recently
from
9bd7b15
to
0eb5e35
Compare
carolinaecalderon
force-pushed
the
carolinac/rm-70
branch
from
a47c39d
to
e4382c9
Compare
master/internal/experiment.go
Outdated
| @@ -1135,3 +1137,12 @@ func (e *internalExperiment) setRP(resourcePool string) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func notASearcherShutdown(ops []searcher.Operation) bool { | |||
There was a problem hiding this comment.
I think this function name made it harder to understand the intent of the function and how it was used.
What if the function was named something like allSearcherShutdown(...) and it returned false as soon as it found an operation that wasn't a shutdown error (and otherwise true)?
Then the if statement above would look like
if _, ok := model.StoppingStates[e.State]; ok && !allSearcherShutdown(ops) {
Just my 2cents. Feel free to ignore if this sounds more confusing to you.
There was a problem hiding this comment.
yeah this is what I was afraid of, thanks for the feedback
Ticket
RM-70
Description
If a searcher completes but the experiment fails in shutdown, the experiment should end in failure. Thus, if an experiment is cancelled, a trial failure should trump the CANCELLED state = the experiment should be marked as FAILED.
Test Plan
See new e2e test, and edited e2e tests. No manual testing needed.
Checklist
docs/release-notes/See Release Note for details.