search

kubeflow / katib

Automated Machine Learning on Kubernetes
https://www.kubeflow.org/docs/components/katib
Apache License 2.0
1.51k stars 442 forks source link

[Proposal] Support JSON format for `file-metrics-collector` #1744

Closed tenzen-y closed 2 years ago

tenzen-y commented 2 years ago

/kind feature

Describe the solution you'd like [A clear and concise description of what you want to happen.]

Motivation

Currently, it is difficult to parse JSON format files by file-metrics-collector using regexp filter since file-metrics-collector is designed to use TEXT format files. I believe if file-metrics-collector supports JSON format files, we can be further made Katib powerful because we can make use of JSON format metrics files without regexp more easily. Therefore, I would like to support JSON format in file-metrics-collector, such as the following example, which is split by newlines.

{"foo": “bar", “fiz": “buz"…}
{“foo": “bar", “fiz": “buz"…}
{“foo": “bar", “fiz": “buz"…}
{“foo": “bar", “fiz": “buz"…}
…

This JSON format is also used in cloudml-hypertune recommended for use in GCP AI Platform or Vertex AI.

If you use a custom container for training or if you want to perform hyperparameter tuning with a framework other than TensorFlow, then you must use the cloudml-hypertune Python package to report your hyperparameter metric to AI Platform Training.

https://cloud.google.com/ai-platform/training/docs/using-hyperparameter-tuning#other_machine_learning_frameworks_or_custom_containers

Design

I'm thinking of the following Kubernetes API and webhook. Also, file-metrics-collector collects values whoose key is spec.objective.objectiveMetricName and spec.objective.additionalMetricNames from the metrcs file if FileSystemFileFormat is set Json.

+ type FileSystemFileFormat string
+
+ const (
+   TextFormat    FileSystemFileFormat = "Text"
+   JsonFormat    FileSystemFileFormat = "Json"
+ )

type FileSystemPath struct {
  Path string                     `json:"path,omitempty"`
  Kind FileSystemKind             `json:"kind,omitempty"`
+ FileFormat FileSystemFileFormat `json:"fileFormat,omitempty"`
}
func (g *DefaultValidator) validateMetricsCollector(inst *experimentsv1beta1.Experiment) error {
  mcSpec := inst.Spec.MetricsCollectorSpec
  mcKind := mcSpec.Collector.Kind
  ...
  switch mcKind {
  ...
  case commonapiv1beta1.FileCollector:
    ...
+     fileFormat := mcSpec.Source.FileSystemPath.FileSytemFileFormat
+     if fileFormat == "" {
+       fileFormat = commonapiv1beta1.TextFormat
+     } else if fileFormat != commonapiv1beta1.TextFormat && fileFormat != commonapiv1beta1.JsonFormat {
+         return return fmt.Errorf("The format of the metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.fileFormat.")
+     }
  ...

Does it sound good to you? @kubeflow/wg-automl-leads

Anything else you would like to add: [Miscellaneous information that will assist in solving the issue.]

tenzen-y commented 2 years ago

/kind feature

andreyvelich commented 2 years ago

Thank you for submitting this proposal @tenzen-y! I have few questions.

because we can make use of JSON format metrics files without regexp more easily

Do you mean it will be easier for user to create such Experiments ? E.g. they don't need to manually add regex to Metrics Collector spec.

How we will change the parsing process in File Metrics Collector ?

Will we have special regex for JSON that we are going to use automatically, when user selects fileFormat=JSON ?

What do you think @gaocegege @johnugeorge ?

tenzen-y commented 2 years ago

Sorry for the late reply. @andreyvelich

Do you mean it will be easier for user to create such Experiments ? E.g. they don't need to manually add regex to Metrics Collector spec.

Yes, users do not need to specify regexp using fileFormat=JSON.

Will we have special regex for JSON that we are going to use automatically, when user selects fileFormat=JSON ?

I think we can parse JSON files of the format shown in the proposal in the following script.

package main

import (
    "encoding/json"
    "fmt"

    "github.com/nxadm/tail"
)

func main() {
    t, _ := tail.TailFile("./log.json", tail.Config{Follow: true})
    metrics := []string{"loss"}

    for line := range t.Lines {

        logText := line.Text
        var jsonObj map[string]interface{}
        json.Unmarshal([]byte(logText), &jsonObj)

        for _, metric := range metrics {
            if _, exist := jsonObj[metric]; !exist {
                continue
            }
            fmt.Printf("%v\n", jsonObj[metric])
        }
    }
}
andreyvelich commented 2 years ago

Sounds good. Probably we should also refactor watchMetricsFile to support EarlyStopping in such Metrics Collector format.

WDYT about this proposal @gaocegege @johnugeorge ?

tenzen-y commented 2 years ago

Probably we should also refactor watchMetricsFile to support EarlyStopping in such Metrics Collector format.

It makes sense. @andreyvelich

tenzen-y commented 2 years ago

I'll wait for feedback from the community until December 10 (UTC+9).

andreyvelich commented 2 years ago

@gaocegege @johnugeorge Please give your feedback on this proposal

tenzen-y commented 2 years ago

I'll wait for feedback from the community until December 10 (UTC+9).

I'll postpone starting the implementation of this feature after December 18 (UTC+9) and wait for feedback from @gaocegege @johnugeorge

gaocegege commented 2 years ago

Sorry for the late reply.

I am wondering how to use the JSON feature from the user side. Could you please give us an example, e.g. Experiment YAML?

tenzen-y commented 2 years ago

Thanks for your reply! @gaocegege I think users can use the below YAML, does it sound good?

apiVersion: kubeflow.org/v1beta1
kind: Experiment
metadata:
  namespace: kubeflow
  name: file-metrics-collector
spec:
  objective:
    type: maximize
    goal: 0.99
    objectiveMetricName: accuracy
    additionalMetricNames:
      - loss
  metricsCollectorSpec:
    source:
-     filter:
-       metricsFormat:
-         - "{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"
      fileSystemPath:
        path: "/katib/mnist.log"
        kind: File
# omitempty; default=Text
+       fileFormat: Json
    collector:
      kind: File
...
gaocegege commented 2 years ago

Gotcha, LGTM.

BTW, the variable should be JSON instead of Json.

I think we can go ahead! 🎉

tenzen-y commented 2 years ago

Thank you for giving me feedback! @gaocegege

BTW, the variable should be JSON instead of Json.

It makes sense.

tenzen-y commented 2 years ago

I'll wait for feedback from the community until December 10 (UTC+9).

I'll postpone starting the implementation of this feature after December 18 (UTC+9) and wait for feedback from @gaocegege @johnugeorge

I'd like to start the implementation of this feature!

/assign

gaocegege commented 2 years ago

Please assign me when the PR is ready to review

Thanks for your contribution! :tada: :+1:

tenzen-y commented 2 years ago

Please assign me when the PR is ready to review

Thanks for your contribution! 🎉 👍

Sure, Thanks for your review! @gaocegege