Arguments missing for custom metrics collector

[votti]

/kind bug

What steps did you take and what happened:
I want to run an experiment with a custom metrics collector using the following spec:

  metricsCollectorSpec:
    source:
      fileSystemPath:
        path: "/tmp/outputs/mlpipeline_metrics/data"
        kind: File
    collector:
      customCollector:
        image: votti/kfpv1-metricscollector:v0.0.7
        imagePullPolicy: Always
        name: custom-metrics-logger-and-collector
      kind: Custom

This is used for: #2019

What did you expect to happen:
I expected the metrics collector sidecar pod to be created to carry the arguments:

• -t trial.Name
• -m: metricNames
• -o-type: string(trial.Spec.Objective.Type)
• -s-db: katibmanagerv1beta1.GetDBManagerAddr()

which should have been added by the following code:

katib/pkg/webhook/v1beta1/pod/inject_webhook.go

Line 298 in 0a7453d

args := []string{"-t", trial.Name, "-m", metricNames, "-o-type", string(trial.Spec.Objective.Type), "-s-db", katibmanagerv1beta1.GetDBManagerAddr()}

Anything else you would like to add:

Environment:

• Katib version (check the Katib controller image version): docker.io/kubeflowkatib/katib-controller:v0.14.0"
• Kubernetes version: (kubectl version):
  Client Version: v1.26.0
  Kustomize Version: v4.5.7
  Server Version: v1.23.14-gke.1800
• OS (uname -a): Linux LAPTOP-SS9UALAT 4.19.128-microsoft-standard SMP Tue Jun 23 12:58:10 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

---

Impacted by this bug? Give it a 👍 We prioritize the issues with the most 👍

[andreyvelich]

Thank you for creating this @votti!

I think, originally we didn't modify Custom Metrics Collector container args. So user can set any container args without unexpected parameters.
To send Trial names for such cases, you can check this example: https://github.com/kubeflow/katib/blob/master/examples/v1beta1/metrics-collector/custom-metrics-collector.yaml#L32-L36.

Does it solve your problem ? Or you want to pass some other info to your container ?

[votti]

Ok let me try this again - I thought TrialNamePrefix from metadata.name was just the prefix of the trial, but not the actual trial_name.

I need the trial name to submit the metrics to the correct trial_name such that it is correctly registered:
https://github.com/d-one/katib/blob/0d25cbfc6a69e3014c3c25c9ea94b4c97b965473/cmd/metricscollector/v1beta1/kfpv1-metricscollector/main.py#L109

I wonder how the kubeflowkatib/custom-metrics-collector:latest managed to do this - but i guess that code is still lost: #1263

Once I get this working, I think I have the know-how together to make a new custom-metrics-collector example that should simplify any future development of such a collector.

[votti]

Actually this works and gives the name of the current pod.
In my case this is the TrialName+the suffix, so I will need some custom code to get the true TrialName - but this is definitely doable.

  metricsCollectorSpec:
    collector:
      customCollector:
        args:
          - -m
          - val-accuracy;accuracy
          - -s
          - katib-db-manager.kubeflow:6789
          - -path
          - /tmp/outputs/mlpipeline_metrics/data
          - -t
          - $(TrialNamePrefix)
        image: votti/kfpv1-metricscollector:v0.0.9
        imagePullPolicy: Always
        name: custom-metrics-logger-and-collector
        env:
          - name: TrialNamePrefix
            valueFrom:
              fieldRef:
                fieldPath: metadata.name
      kind: Custom
    source:
      fileSystemPath:
        kind: File
        path: /tmp/outputs/mlpipeline_metrics/data

Thanks for pointing me to this!

[andreyvelich]

Great. @votti I guess, it gives you Pod name where Training + Metrics Collector containers are running, right ?

[votti]

Yes I was just updating my comment as I noticed this.

But in my case the Pod name is the TrialName + suffix, so this will work.

[andreyvelich]

Thanks @votti. That means that Pod name must have Trial name, which might not work for every Kubernetes CRDs as Trial template (e.g. Argo Workflows).
@johnugeorge @tenzen-y I think we should keep this in mind when users try to create Custom Metrics Collectors.

Also, we mutate Trial name to the Pod labels, so another solution is to fetch this data from Pod's labels.

[votti]

Somehow having the option/a flag to add the standard CLI arguments to the customCollector as well would be a bit more elegant in my opinion.
Maybe this would work with a StandardCustom or so kind for the collector?

Then such a standard custom metrics collector just would need to stick to a CLI interface to get these parameters, instead of relying on additional workarounds.

Eg having to hard code the metrics as well as the katib-db-manager as in the example is a bit inelegant as well.

[votti]

@pod lables: in my case I do not find that label (should be katib.kubeflow.org/tria
Could be because I am actually working on an Argo Workflow, where the Trial kicks of a Workflow that orchestrates the pods.
Yet this does not seem to be a reliable solution as well.

[andreyvelich]

Then such a standard custom metrics collector just would need to stick to a CLI interface to get these parameters, instead of relying on additional workarounds.

We should think more about it, we can always inject args to container ENVs to not break the runtime.

@pod lables: in my case I do not find that label (should be katib.kubeflow.org/tria

@votti What version of Katib Controller you are using ? This feature was implemented recently (in Katib 0.15 release).

[votti]

@andreyvelich : 0.14 - will check in 0.15 soon

[tenzen-y]

Thanks @votti. That means that Pod name must have Trial name, which might not work for every Kubernetes CRDs as Trial template (e.g. Argo Workflows). @johnugeorge @tenzen-y I think we should keep this in mind when users try to create Custom Metrics Collectors.

Also, we mutate Trial name to the Pod labels, so another solution is to fetch this data from Pod's labels.

It seems that we don't have any documentation for the custom metrics collector. It would be good to add the documentation for that and mention about Pod name.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.