String type for metric values #1245
Conversation
|
This change is |
|
This PR sounds good to me, except I still fill not good about pending trial if its job succeed but metrics unavailable. Considering no warning messages will be shown on user interface, when this condition happens. |
Maybe we could notify user about this case in the Experiment monitor page in the Katib UI? |
|
SGTM. It can be in another PR. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich 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 |
* Change metric type from float to string * Check unavailable latest objective metric in isTrialObservationAvailable * Fix e2e test * Delete consts.UnavailableMetricValue from e2e
Fixes: #1227.
I changed metric value from
float64tostring.If metrics can't be reported,
unavailablevalue will be recorded.I added condition that
metric.Latestcan't beunavailablehere: https://github.com/andreyvelich/katib/blob/cd6b37b3614cd226781c0feac1dcf6d568d07f01/pkg/controller.v1beta1/trial/trial_controller_util.go#L133-L135.If Objective metric values were not reported, Trial will be in
Falsestatus andSucceededcondition with this message:Warning MetricsUnavailable 2m11s (x2 over 2m11s) trial-controller Metrics are not available for Job.Experiment will be still running and waiting until at least one Objective metric value will be reported.
I think it is correct workflow, since user can see that Objective metric is not printed by training container, modify the training container image and manually restart the Trial, by deleting appropriate BatchJob, TFJob, etc.. .
What do you think @sperlingxx @gaocegege @johnugeorge ?
/assign @gaocegege @johnugeorge @sperlingxx