feat: Allow AnalysisRun to complete an experiment #345
Conversation
Codecov Report
@@ Coverage Diff @@
## master #345 +/- ##
==========================================
+ Coverage 84.04% 84.14% +0.09%
==========================================
Files 66 66
Lines 6055 6072 +17
==========================================
+ Hits 5089 5109 +20
+ Misses 689 688 -1
+ Partials 277 275 -2
Continue to review full report at Codecov.
|
dthomson25
force-pushed
the
ar-complete-ex
branch
from
64efa5c to
d2f5c71
Compare
| @@ -201,4 +203,6 @@ type ExperimentAnalysisRunStatus struct { | |||
| Phase AnalysisPhase `json:"phase"` | |||
| // Message is a message explaining the current status | |||
| Message string `json:"message,omitempty"` | |||
| // RequiredForCompletion indicates that experiment should complete after analysis finishes | |||
| RequiredForCompletion bool `json:"requiredForCompletion,omitempty"` | |||
There was a problem hiding this comment.
I feel we don't need to duplicate RequiredForCompletion in the status for this structure. Can't this be inferred from the ExperimentAnalysisTemplateRef?
experiments/experiment.go
Outdated
| for _, a := range ec.ex.Spec.Analyses { | ||
| as := experimentutil.GetAnalysisRunStatus(*ec.newStatus, a.Name) | ||
| if as.RequiredForCompletion && as.Phase == v1alpha1.AnalysisPhaseSuccessful { | ||
| requiredAnalysisRunCompleted = true |
There was a problem hiding this comment.
I think theres a bug here. This will set requiredAnalysisRunCompleted to be true as soon as any one of the required runs completes, but not all. I think this should be reversed to initialize as true and set it to false as soon as one required run is still not complete.
experiments/experiment.go
Outdated
| @@ -380,6 +385,9 @@ func (ec *experimentContext) calculateStatus() *v1alpha1.ExperimentStatus { | |||
| // We will now fail the experiment. | |||
| ec.newStatus.Phase = analysesStatus | |||
| ec.newStatus.Message = analysesMessage | |||
| } else if analysesMessage == requiredAnalysisCompletedMessage { | |||
There was a problem hiding this comment.
I think comparing the message against a well known message seems too magical. Is the point of this code to bubble up a better reason for a running experiment?
There was a problem hiding this comment.
That's a fair point. No, the purpose of this code is to provide a way to mark an experiment successful if the required analysis runs are completed. Since we shouldn't mark the experiment successful if the analysis is successful but not required, the magic string makes it easy to determine that case. Can you take another look?
dthomson25
force-pushed
the
ar-complete-ex
branch
from
3ac1c08 to
9b4b214
Compare
dthomson25
force-pushed
the
ar-complete-ex
branch
from
9b4b214 to
ab23de8
Compare
| } | ||
|
|
||
| // CompletedAllRequiredAnalysisRuns has at least one required for completition analysis run | ||
| // and it completed successfully |
There was a problem hiding this comment.
Can we rename this to: RequiredAnalysisRunsSuccessful() ? I didn't realize this would only only return when they were successful and not just completed.
|
LGTM, but suggest to rename method |
closes #248