Update util for experiment in v1alpha2 #485
Conversation
|
/assign @johnugeorge |
| func (exp *Experiment) IsCompleted() bool { | ||
| return exp.IsSucceeded() || exp.IsFailed() | ||
| } | ||
|
|
||
| func (exp *Experiment) GetCurrentConditionType() ExperimentConditionType { | ||
| if exp.IsCreated() { |
There was a problem hiding this comment.
What would this function used for? If you want the latest condition, it would be better to directly lookup the Type of the last status.conditions?
There was a problem hiding this comment.
Is that true, that the latest Condition in this array: https://github.com/kubeflow/katib/blob/master/pkg/api/operators/apis/experiment/v1alpha2/experiment_types.go#L77, will be current?
I need to get Current Condition of Experiment.
| func (exp *Experiment) IsCompleted() bool { | ||
| return exp.IsSucceeded() || exp.IsFailed() | ||
| } | ||
|
|
||
| func (exp *Experiment) GetCurrentConditionType() ExperimentConditionType { |
There was a problem hiding this comment.
it will always return ExperimentCreated, I don't think you except it
There was a problem hiding this comment.
@hougangliu We can leave here only Succeeded or Failed or Running or Restarting condition. I think, for 1 experiment it can be one of the 4 conditions at the same time, isn't it?
There was a problem hiding this comment.
I mean exp.IsCreated() will always true
There was a problem hiding this comment.
What will happen in that situation:
Experiment ran and after that failed
Experiment restarted
Experiment is running again.
Will exp.IsRunning() and exp.IsFailed() return true ?
There was a problem hiding this comment.
My understanding, the exp.Status.Conditions is a history of conditions of the experiment. So the exp.IsCreated() will be always true.
The last value of exp.Status.Conditions is the latest condition.
There was a problem hiding this comment.
@YujiOshima @hougangliu Ok, thank you! I changed this function to get the latest condition, please , take a look. In the UI, where we monitor experiments, I think, we should show the latest condition.
| func (exp *Experiment) IsCompleted() bool { | ||
| return exp.IsSucceeded() || exp.IsFailed() | ||
| } | ||
|
|
||
| func (exp *Experiment) GetLastConditionType() ExperimentConditionType { |
There was a problem hiding this comment.
just for safe, we'd better guard len(exp.Status.Conditions) > 0 before below
There was a problem hiding this comment.
@hougangliu Added, with an additional error, if Experiment doesn't have any Condition.
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
@andreyvelich I think you can try to rebase upstream to include #489 to fix test error |
|
@hougangliu Thanks, done. |
…til-experiment-condition
|
/lgtm |
|
@andreyvelich: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@richardsliu @hougangliu Please, lgtm this PR again. |
|
/lgtm |
I added new functions in Experiment util. After that, we can get new 2 conditions of Experiment and get current condition type of Experiment.
I will use it in the UI.
This change is