More container fields for SuggestionConfig #2000

fischor · 2022-11-05T17:47:31Z

What this PR does / why we need it:

This PR adds more fields to the SuggestionConfig that is used to populate the main container of the Suggestion deployment. It adds fields command, args, envs that can be used to pass arguments to the executable. This allows the user to easily parametrize the suggestion algorithm/executable itself by passing values using these new fields.

Which issue(s) this PR fixes:
Fixes #1737 (at least partly)

Checklist:

Docs: the docs will need to mention the new fields

andreyvelich

Thank you for taking this @fischor!

andreyvelich · 2022-11-08T14:29:16Z

pkg/util/v1beta1/katibconfig/config.go

+	Args                      []string                         `json:"args,omitempty"`
+	WorkingDir                string                           `json:"workingDir,omitempty"`
+	EnvFrom                   []corev1.EnvFromSource           `json:"envFrom,omitempty"`
+	Env                       []corev1.EnvVar                  `json:"env,omitempty"`


Maybe we could start adding more fields to SuggestionConfig with corev1.Container{} field, similar to PodSpec API in K8s ? I think you can use inline param to avoid modification in existing Katib config YAML:

corev1.Container `json:",inline"`

I guess, at the end we want to give user ability to modify the whole Suggestion's Deployment spec via Katib config, but it requires much more changes.

WDYT @tenzen-y @johnugeorge @fischor ?

Maybe we could start adding more fields to SuggestionConfig with corev1.Container{} field, similar to PodSpec API in K8s ? I think you can use inline param to avoid modification in existing Katib config YAML

sgtm

I guess, at the end we want to give user ability to modify the whole Suggestion's Deployment spec via Katib config, but it requires much more changes.

I think so too. Maybe, to support customizing Tolerations, imagePullSecret and so on, we want to support customizing PodSpec for suggestion service deployment.

Yes, inlining corev1.Container would also be an option.
Certain fields of the container will be set by the katib-Controller, like some ports, the liveness and readiness probes and possibly the "suggestion-volume". How to handle these? Overwrite or not?

I think so too. Maybe, to support customizing Tolerations, imagePullSecret and so on, we want to support customizing PodSpec for suggestion service deployment.

To support his, how about specifying a default deployment in the suggestion config (instead of just the container)?.

For namespace specific specific suggestion deployments, we can allow to have a configmap in a user namespace(where experiment is run). If suggestion spec is provided in the configmap of the user namespace, it can override the global katib config.
Ref: #1976

Certain fields of the container will be set by the katib-Controller, like some ports, the liveness and readiness probes and possibly the "suggestion-volume". How to handle these? Overwrite or not?

@fischor I think if parameters are set by the user we should use them, otherwise use the default values.

To support his, how about specifying a default deployment in the suggestion config (instead of just the container)?.

We might want to set spec.template.spec for our Suggestion config, as I mentioned here: #1737 (comment), but that requires much more changes since we need to modify manifests.

If suggestion spec is provided in the configmap of the user namespace, it can override the global katib config.

@johnugeorge That might be the next step. We need to have discussion what is the best way to give user ability to set experiment-scoped Katib Config.

Sorry for the late response.

We might want to set spec.template.spec for our Suggestion config, as I mentioned here: #1737 (comment), but that requires much more changes since we need to modify manifests.

As the first step, in this PR, we might be good to provide only the feature to be able to customize the corev1.Container{}.

For namespace specific specific suggestion deployments, we can allow to have a configmap in a user namespace(where experiment is run). If suggestion spec is provided in the configmap of the user namespace, it can override the global katib config.
Ref: #1976

This is a good idea. As mentioned @ andreyvelich, I also think we would be able to discuss about the feature on the next step.

coveralls · 2022-11-08T15:42:49Z

Coverage: 73.247% (+0.8%) from 72.455% when pulling 6cc917e on fischor:issue-1737-more-fields-in-suggestion-config into 54b020b on kubeflow:master.

johnugeorge · 2022-12-12T05:29:23Z

@fischor Can you rebase and fix?

fischor · 2022-12-17T10:48:25Z

The name of the suggestion algorithm container was not defaulted. Creating the suggestion deployment failed because of a missing container name.

@johnugeorge I have rebased and fixed.

andreyvelich · 2022-12-22T14:31:29Z

pkg/util/v1beta1/katibconfig/config.go

-	Image                     string                           `json:"image"`
-	ImagePullPolicy           corev1.PullPolicy                `json:"imagePullPolicy,omitempty"`
-	Resource                  corev1.ResourceRequirements      `json:"resources,omitempty"`
+	corev1.Container          `json:",inline"`
 	ServiceAccountName        string                           `json:"serviceAccountName,omitempty"`
 	VolumeMountPath           string                           `json:"volumeMountPath,omitempty"`


@fischor Do we need VolumeMountPath if that is part of Container parameters:

katib/pkg/controller.v1beta1/suggestion/composer/composer.go

Line 237 in 7f1b284

if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume && len(suggestionContainer.VolumeMounts) == 0 {

?

The current Katib SuggestionConfig has a JSON field volumeMountPath thats just a filepath, the corev1.Container has a json field volumeMounts thats an array of volume mounts. So volumeMountPath is not part if the container fields. I think in order to not break anything, we need it.

The referenced snippet kind of follows the logic:

... if parameters are set by the user we should use them, otherwise use the default values.

In case the volumeMounts are set we prefer that, otherwise mount the default suggestion volume under the specificed volumeMountPath.

I see, that is because we set the default value name for Suggestion Volume. Please can you leave comment that we should refactor VolumeMountPath parameter once we allow to set Deployment spec in our Suggestion config ?

Also, I think we should modify len(suggestionContainer.VolumeMounts) == 0 check.
We always should append VolumeMount if ResumePolicy is FromVolume.

if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume { volumeMount := corev1.VolumeMount{ Name: consts.ContainerSuggestionVolumeName, MountPath: suggestionConfigData.VolumeMountPath, } suggestionContainer.VolumeMounts = append(suggestionContainer.VolumeMounts, volumeMount) }

WDYT @fischor ?

@andreyvelich Have you seen that the consts.DefaultSuggestionPortName port is also just set if the Container specified in the suggestion has no ports set? See https://github.com/fischor/katib/blob/7f1b2842f87158c25c54f577da1b82ddc7a958e3/pkg/controller.v1beta1/suggestion/composer/composer.go#L194-L201
Would you also change that to always append the default port?

I think both ways of doing this have pros/cons. If we always append the default volume/port then the user has no option to overwrite it. If we never append the default volume/port, then the user needs to be aware that it has to specify it when setting any volumes/ports.

A third option would be to check if the default volume/port is already set in the container and only append if its not. I think I would prefer that.

A third option would be to check if the default volume/port is already set in the container and only append if its not. I think I would prefer that.

I think that should work, but we need to make sure that volume and port name/number is correct (ContainerSuggestionVolumeName and DefaultSuggestionPortName/DefaultSuggestionPort ).

@andreyvelich I have added check to only append a VolumeMount for the Suggestion Volume only in case no volume mount with that default name ("suggestion-volume") exists. And a check to only append a ContainerPort for the Suggestion API in case no port with the default suggestion api port name and/or port number exists.

andreyvelich · 2023-01-16T15:08:28Z

pkg/controller.v1beta1/suggestion/composer/composer.go

+	if !containsContainerPortWithName(suggestionContainer.Ports, consts.DefaultSuggestionPortName) &&
+		!containsContainerPort(suggestionConfigData.Ports, consts.DefaultSuggestionPort) {


Should we verify that port name and containerPort is set together in the Katib Config ?
DefaultSuggestionPortName and DefaultSuggestionPort is always used for the Suggestion's service.
So if user set DefaultSuggestionPortName port with different address, the Suggestion service fails.

cc @tenzen-y

cc @johnugeorge

It might be better to prohibit setting containerPort in katibConfig.

We should add a validation check either ways.

@fischor Do you have time to check my latest message ? We are getting closer for the Katib 0.15 release, and it would be great to include your feature in that release.

@andreyvelich I should have some spare time tomorrow. Do we do it as you suggested: verify that name and port are correct, otherwise return an error in DesiredDeployment method? Or prohibit the DefaultSuggestionPort to be set at all?

@fischor Thank you! Let's prohibit to set DefaultSuggestionPort for now as @tenzen-y suggested. So we can avoid errors from the user side.

@fischor We are planning to cut a release in a day. Do you have any update?

@johnugeorge Thanks for the reminder...you can check the latest commit now.

andreyvelich · 2023-01-25T18:08:48Z

Thank you for updates @fischor.
Excited to see a new usage of Katib config!
/lgtm
/approve

google-oss-prow · 2023-01-25T18:08:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

tenzen-y · 2023-01-25T18:13:11Z

@fischor Great work!
Thanks for your effort!

google-oss-prow bot requested review from anencore94 and sperlingxx November 5, 2022 17:47

google-oss-prow bot added the size/S label Nov 5, 2022

More container fields for SuggestionConfig

c6f87bd

fischor force-pushed the issue-1737-more-fields-in-suggestion-config branch from 7081489 to c6f87bd Compare November 5, 2022 17:49

andreyvelich reviewed Nov 8, 2022

View reviewed changes

Inline corev1.Container into SuggestionConfig

427f115

google-oss-prow bot added size/M and removed size/S labels Nov 14, 2022

fischor requested review from andreyvelich and removed request for sperlingxx and anencore94 November 14, 2022 19:40

Set default value for suggestion container name

7f1b284

andreyvelich reviewed Dec 22, 2022

View reviewed changes

fischor added 2 commits January 15, 2023 12:21

Append suggestion volume and port only if not present

5410bdc

Deep-Copy base suggestion container

92d0471

google-oss-prow bot added size/L and removed size/M labels Jan 15, 2023

Check for suggestion container port number as well

caa08c8

andreyvelich reviewed Jan 16, 2023

View reviewed changes

Prohibit suggestion port to be set in suggestion config

6cc917e

google-oss-prow bot assigned andreyvelich Jan 25, 2023

google-oss-prow bot added the lgtm label Jan 25, 2023

google-oss-prow bot added the approved label Jan 25, 2023

google-oss-prow bot merged commit ff6441b into kubeflow:master Jan 25, 2023

tom-pavz mentioned this pull request Jan 22, 2024

How do I run an experiment on a k8s cluster with taint? #1174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More container fields for SuggestionConfig #2000

More container fields for SuggestionConfig #2000

fischor commented Nov 5, 2022 •

edited

Loading

andreyvelich left a comment

andreyvelich Nov 8, 2022 •

edited

Loading

tenzen-y Nov 8, 2022

fischor Nov 8, 2022 •

edited

Loading

johnugeorge Nov 9, 2022

andreyvelich Nov 9, 2022

tenzen-y Nov 16, 2022

coveralls commented Nov 8, 2022 •

edited

Loading

johnugeorge commented Dec 12, 2022

fischor commented Dec 17, 2022

andreyvelich Dec 22, 2022

fischor Dec 22, 2022 •

edited

Loading

andreyvelich Dec 23, 2022

fischor Dec 26, 2022

andreyvelich Jan 3, 2023 •

edited

Loading

fischor Jan 15, 2023

andreyvelich Jan 16, 2023 •

edited

Loading

andreyvelich Jan 16, 2023

tenzen-y Jan 16, 2023

johnugeorge Jan 23, 2023

andreyvelich Jan 23, 2023 •

edited

Loading

fischor Jan 23, 2023

andreyvelich Jan 23, 2023

johnugeorge Jan 25, 2023

fischor Jan 25, 2023

andreyvelich commented Jan 25, 2023 •

edited

Loading

google-oss-prow bot commented Jan 25, 2023

tenzen-y commented Jan 25, 2023

		if !containsContainerPortWithName(suggestionContainer.Ports, consts.DefaultSuggestionPortName) &&
		!containsContainerPort(suggestionConfigData.Ports, consts.DefaultSuggestionPort) {

More container fields for SuggestionConfig #2000

More container fields for SuggestionConfig #2000

Conversation

fischor commented Nov 5, 2022 • edited Loading

andreyvelich left a comment

Choose a reason for hiding this comment

andreyvelich Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fischor Nov 8, 2022 • 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

coveralls commented Nov 8, 2022 • edited Loading

johnugeorge commented Dec 12, 2022

fischor commented Dec 17, 2022

Choose a reason for hiding this comment

fischor Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Jan 16, 2023 • 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

andreyvelich Jan 23, 2023 • 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

andreyvelich commented Jan 25, 2023 • edited Loading

google-oss-prow bot commented Jan 25, 2023

tenzen-y commented Jan 25, 2023

fischor commented Nov 5, 2022 •

edited

Loading

andreyvelich Nov 8, 2022 •

edited

Loading

fischor Nov 8, 2022 •

edited

Loading

coveralls commented Nov 8, 2022 •

edited

Loading

fischor Dec 22, 2022 •

edited

Loading

andreyvelich Jan 3, 2023 •

edited

Loading

andreyvelich Jan 16, 2023 •

edited

Loading

andreyvelich Jan 23, 2023 •

edited

Loading

andreyvelich commented Jan 25, 2023 •

edited

Loading