Feature/waitallprocesses config #1394

robbertvdg · 2020-11-18T09:51:29Z

What this PR does / why we need it:
Adds the waitAllProcesses option to the katib-config metricsCollector spec. It is made optional, defaulting to true.

Which issue(s) this PR fixes
See #1383 for discussion

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Available waitAllProcesses option to katib-config's metrics-collector-sidecar spec.

aws-kf-ci-bot · 2020-11-18T09:51:40Z

Hi @robbertvdg. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

robbertvdg · 2020-11-18T09:52:54Z

@andreyvelich can you take a look at this?

andreyvelich

Thank you for doing this @robbertvdg!
I left few comments.
/ok-to-test
/cc @gaocegege @johnugeorge

andreyvelich · 2020-11-18T18:52:34Z

cmd/metricscollector/v1beta1/file-metricscollector/main.go

@@ -107,7 +107,7 @@ var (
 	metricFilters        = flag.String("f", "", "Metric filters")
 	pollInterval         = flag.Duration("p", common.DefaultPollInterval, "Poll interval between running processes check")
 	timeout              = flag.Duration("timeout", common.DefaultTimeout, "Timeout before invoke error during running processes check")
-	waitAll              = flag.Bool("w", common.DefaultWaitAll, "Whether wait for all other main process of container exiting")
+	waitAllProcesses     = flag.String("w", common.DefaultWaitAllProcesses, "Whether wait for all other main process of container exiting")


Do we need to keep this flag String, or we can use Bool?
I assume this flag can be only true or false.

I had problems with parsing booleans when a user has not specified the field in the katib-config. Do you know of a good way to do it?
I could leave it string in the katib-config and parse it to bool in GetMetricsCollectorConfigData, would that be better?

@robbertvdg It seems that empty bool in Go is false. So when user doesn't specify any values in config we send "false" to the metrics collector.
Maybe we can define MetricsCollectorConfig like this:

type MetricsCollectorConfig struct { Image string `json:"image"` ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy"` Resource corev1.ResourceRequirements `json:"resources"` WaitAllProcesses *bool `json:"waitAllProcesses"` }

In that case, you can set -w flag if WaitAllProcesses != nil. Otherwise the default value is set.

In Katib config I was able to define bool value like this:

"StdOut": { "image": "docker.io/andreyvelichkevich/file-metrics-collector", "imagePullPolicy": "Always", "waitAllProcesses": false },

The problem lies with the v1.Container.args which is string[]. Therefore every container argument has to be a string, which I think is not compatible with the flag.Bool() argument (I tested it, doesn't work).

@robbertvdg Got it.
In that case can we keep WaitAllProcesses *bool in MetricsCollectorConfig, convert it to string here:

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

Line 299 in 3f5d813

args = append(args, "-w", metricsCollectorConfigData.WaitAllProcesses)

Then, use the String type like you did in Metrics Collector and convert it to Bool after.

In that case, we can avoid situation when user specify wrong values in Katib config.

What do you think @robbertvdg @gaocegege ?

@andreyvelich those were my thoughts as well, will update PR soon.

andreyvelich · 2020-11-18T18:53:54Z

cmd/metricscollector/v1beta1/file-metricscollector/main.go

@@ -353,10 +353,16 @@ func main() {
 		go printMetricsFile(*metricsFilePath)
 	}

+	waitAll, err := strconv.ParseBool(*waitAllProcesses)


If we keep it Bool, we can skip conversion.

andreyvelich · 2020-11-18T18:55:11Z

pkg/util/v1beta1/katibconfig/config.go

+	Image            string                      `json:"image"`
+	ImagePullPolicy  corev1.PullPolicy           `json:"imagePullPolicy"`
+	Resource         corev1.ResourceRequirements `json:"resources"`
+	WaitAllProcesses string                      `json:"waitAllProcesses"`


Same as above, can we keep it bool ?

gaocegege · 2020-11-19T09:49:43Z

pkg/util/v1beta1/katibconfig/config.go

-	Image           string                      `json:"image"`
-	ImagePullPolicy corev1.PullPolicy           `json:"imagePullPolicy"`
-	Resource        corev1.ResourceRequirements `json:"resources"`
+	Image            string                      `json:"image"`


Should we add omitempty for the fieds?

I think we can add it on all parameters but Image.

robbertvdg · 2020-11-20T15:17:43Z

@andreyvelich @gaocegege updated PR, can you do a final check?

andreyvelich · 2020-11-20T16:31:11Z

pkg/util/v1beta1/katibconfig/config.go

+	Image            string                      `json:"image"`
+	ImagePullPolicy  corev1.PullPolicy           `json:"imagePullPolicy,omitempty"`
+	Resource         corev1.ResourceRequirements `json:"resources,omitempty"`
+	WaitAllProcesses *bool                       `json:"waitAllProcesses,omitempty"`


Can you add omitempty to the SuggestionConfig also please ? It should be in all parameters but Image.

andreyvelich · 2020-11-20T16:33:19Z

cmd/metricscollector/v1beta1/tfevent-metricscollector/main.py

@@ -38,6 +38,7 @@ def parse_options():
    logger.addHandler(handler)
    logger.propagate = False
    opt = parse_options()
+    wait_all_processes = opt.wait_all_processes.lower() == "true"


Does this compare (opt.wait_all_processes.lower() == "true") necessary ?
I assume wait_all_processes can be only "true" or "false" since WaitAllProcesses *bool in Katib config?

this is the python way of converting string to bool ;) should be fine since the string can only have two possibilities

andreyvelich

Thank you for your contribution @robbertvdg!
/lgtm
/cc @gaocegege

k8s-ci-robot · 2020-11-24T02:16:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, robbertvdg

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:

~~OWNERS~~ [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andreyvelich · 2020-11-24T02:16:45Z

/hold
Hold for the @gaocegege review.

gaocegege · 2020-11-24T03:55:10Z

/hold cancel
/lgtm

gaocegege · 2020-11-24T03:55:16Z

Thanks for your contribution! 🎉 👍

robbertvdg added 2 commits November 18, 2020 10:28

Add waitAllProcesses to metricsCollector config

c2fc532

wait_all_processes config for python metricscollector main

59e63f6

aws-kf-ci-bot added the needs-ok-to-test label Nov 18, 2020

k8s-ci-robot requested review from andreyvelich and sperlingxx November 18, 2020 09:51

Correct boolean check

3f5d813

andreyvelich reviewed Nov 18, 2020

View reviewed changes

k8s-ci-robot requested review from gaocegege and johnugeorge November 18, 2020 18:58

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Nov 18, 2020

gaocegege reviewed Nov 19, 2020

View reviewed changes

waitAllProcesses config as bool

76d9373

andreyvelich reviewed Nov 20, 2020

View reviewed changes

robbertvdg added 2 commits November 23, 2020 09:52

add omitempty to suggestion and earlystopping

d87fd6b

correct default config

0df1035

andreyvelich approved these changes Nov 24, 2020

View reviewed changes

k8s-ci-robot requested a review from gaocegege November 24, 2020 02:16

k8s-ci-robot assigned andreyvelich Nov 24, 2020

k8s-ci-robot added the lgtm label Nov 24, 2020

k8s-ci-robot added the approved label Nov 24, 2020

k8s-ci-robot added the do-not-merge/hold label Nov 24, 2020

k8s-ci-robot removed the do-not-merge/hold label Nov 24, 2020

k8s-ci-robot assigned gaocegege Nov 24, 2020

k8s-ci-robot merged commit 7104018 into kubeflow:master Nov 24, 2020

robbertvdg deleted the feature/waitallprocesses-config branch November 24, 2020 08:00

This was referenced Nov 25, 2020

[Documentation] Enable to specify wait all process parameter in Katib config #1398

Closed

Add WaitAll metrics collector flag to the Katib config #1387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/waitallprocesses config #1394

Feature/waitallprocesses config #1394

robbertvdg commented Nov 18, 2020

aws-kf-ci-bot commented Nov 18, 2020

robbertvdg commented Nov 18, 2020

andreyvelich left a comment

andreyvelich Nov 18, 2020

robbertvdg Nov 18, 2020 •

edited

Loading

andreyvelich Nov 18, 2020

robbertvdg Nov 19, 2020

andreyvelich Nov 19, 2020

robbertvdg Nov 20, 2020

andreyvelich Nov 18, 2020

andreyvelich Nov 18, 2020

gaocegege Nov 19, 2020

andreyvelich Nov 19, 2020

robbertvdg commented Nov 20, 2020

andreyvelich Nov 20, 2020

andreyvelich Nov 20, 2020

robbertvdg Nov 23, 2020

andreyvelich left a comment

k8s-ci-robot commented Nov 24, 2020

andreyvelich commented Nov 24, 2020 •

edited

Loading

gaocegege commented Nov 24, 2020

gaocegege commented Nov 24, 2020

Feature/waitallprocesses config #1394

Feature/waitallprocesses config #1394

Conversation

robbertvdg commented Nov 18, 2020

aws-kf-ci-bot commented Nov 18, 2020

robbertvdg commented Nov 18, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbertvdg Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbertvdg commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 24, 2020

andreyvelich commented Nov 24, 2020 • edited Loading

gaocegege commented Nov 24, 2020

gaocegege commented Nov 24, 2020

robbertvdg Nov 18, 2020 •

edited

Loading

andreyvelich commented Nov 24, 2020 •

edited

Loading