Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use ErrorList for experiment validator #2329

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Use ErrorList for experiment validator #2329

merged 1 commit into from
Jun 27, 2024

Conversation

ckcd
Copy link
Contributor

@ckcd ckcd commented May 17, 2024

What this PR does / why we need it:
For experiment validator, return errors after all fields are verified.

Which issue(s) this PR fixes:
Fixes #2302

Checklist:

  • Docs included if any changes are user facing

@ckcd ckcd changed the title Use ErrorList for validator Use ErrorList for experiment validator May 17, 2024
@ckcd ckcd changed the title Use ErrorList for experiment validator [WIP] Use ErrorList for experiment validator May 17, 2024
@ckcd ckcd changed the title [WIP] Use ErrorList for experiment validator Use ErrorList for experiment validator May 17, 2024
@ckcd ckcd marked this pull request as draft May 17, 2024 02:45
@ckcd ckcd force-pushed the ck2405_use_err_list branch from c024b04 to 286f08b Compare May 17, 2024 03:14
@ckcd ckcd changed the title Use ErrorList for experiment validator [WIP] Use ErrorList for experiment validator May 17, 2024
@ckcd ckcd force-pushed the ck2405_use_err_list branch 2 times, most recently from 89084ec to 5be749e Compare May 17, 2024 05:14
@ckcd ckcd marked this pull request as ready for review May 17, 2024 05:38
@google-oss-prow google-oss-prow bot requested a review from johnugeorge May 17, 2024 05:38
@ckcd ckcd changed the title [WIP] Use ErrorList for experiment validator Use ErrorList for experiment validator May 17, 2024
@ckcd
Copy link
Contributor Author

ckcd commented May 17, 2024

/retest

@ckcd ckcd force-pushed the ck2405_use_err_list branch from 5be749e to 56159d6 Compare May 17, 2024 05:41
@tenzen-y
Copy link
Member

/retest

This command should not work well.

@ckcd
Copy link
Contributor Author

ckcd commented May 17, 2024

This command should not work well.

@tenzen-y Thanks... seems no way for me to retry. I just push a new commit to re-trigger the tests.

Also please help to review this, thanks.
/assign @tenzen-y

@ckcd
Copy link
Contributor Author

ckcd commented May 17, 2024

Also not sure why some test will fail randomly. They all runs well in my local. cc @tenzen-y

@tenzen-y
Copy link
Member

Details

That is a flaky test: #2269

andreyvelich
andreyvelich reviewed May 24, 2024
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this @ckcd!
I left a few comments.

pkg/webhook/v1beta1/experiment/validator/validator.go Outdated
for i, param := range parameters {

if param.ParameterType != experimentsv1beta1.ParameterTypeInt &&
param.ParameterType != experimentsv1beta1.ParameterTypeDouble &&
param.ParameterType != experimentsv1beta1.ParameterTypeCategorical &&
param.ParameterType != experimentsv1beta1.ParameterTypeDiscrete &&
param.ParameterType != experimentsv1beta1.ParameterTypeUnknown {
return fmt.Errorf("parameterType: %v is not supported in spec.parameters[%v]: %v", param.ParameterType, i, param)
allErrs = append(allErrs, field.Invalid(parametersPath.Index(i).Child("parameterType"),
param.ParameterType, fmt.Sprintf("parameterType: %v is not supported in spec.parameters[%v]: %v", param.ParameterType, i, param)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we simplify the error message here (e.g. remove spec.parameters) like we did for other error messages:

katib/pkg/webhook/v1beta1/experiment/validator/validator.go

Line 229 in 56159d6

allErrs = append(allErrs, field.Required(earlyStoppingPath.Child("algorithmName"), "must be specified"))

pkg/webhook/v1beta1/experiment/validator/validator.go Outdated
Comment on lines 269 to 278
fmt.Sprintf("feasibleSpace must be specified in spec.parameters[%v]: %v", i, param)))
} else {
if param.ParameterType == experimentsv1beta1.ParameterTypeDouble || param.ParameterType == experimentsv1beta1.ParameterTypeInt {
if len(param.FeasibleSpace.List) > 0 {
allErrs = append(allErrs, field.Invalid(parametersPath.Index(i).Child("feasibleSpace").Child("list"),
param.FeasibleSpace.List, fmt.Sprintf("feasibleSpace.list is not supported for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param)))
}
if param.FeasibleSpace.Max == "" && param.FeasibleSpace.Min == "" {
allErrs = append(allErrs, field.Required(parametersPath.Index(i).Child("feasibleSpace").Child("max"),
fmt.Sprintf("feasibleSpace.max or feasibleSpace.min must be specified for parameterType: %v in spec.parameters[%v]: %v", param.ParameterType, i, param)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments for error messages.

pkg/webhook/v1beta1/experiment/validator/validator.go
}

// Check if trialSpec or configMap exists
if trialTemplate.TrialSource.TrialSpec == nil && trialTemplate.TrialSource.ConfigMap == nil {
return fmt.Errorf("spec.trialTemplate.trialSpec or spec.trialTemplate.configMap must be specified")
return append(allErrs, field.Required(trialTemplatePath.Child("TrialSource"), "spec.trialTemplate.trialSpec or spec.trialTemplate.configMap must be specified"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@ckcd ckcd force-pushed the ck2405_use_err_list branch from 56159d6 to 0bf4cbc Compare May 27, 2024 08:54
@ckcd
Copy link
Contributor Author

ckcd commented May 27, 2024

@andreyvelich Thanks for your reply! I have made the changes according to your opinion. PTAL again, thanks.

andreyvelich
andreyvelich reviewed May 27, 2024
View reviewed changes
pkg/webhook/v1beta1/experiment/validator/validator.go Outdated
}
// Format
fileFormat := mcSpec.Source.FileSystemPath.Format
if fileFormat != commonapiv1beta1.TextFormat && fileFormat != commonapiv1beta1.JsonFormat {
return fmt.Errorf("format of metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.format")
allErrs = append(allErrs, field.Required(metricsSourcePath.Child("fileSystemPath").Child("format"),
"format of metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.format"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckcd How the error message will look like that we send to the users ?
Do we need to simplify it as follows:
format of metrics file is required for metrics collector ?

Copy link
Contributor Author

@ckcd ckcd May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreyvelich Yes the error message looks a little redundant:

[spec.metricsCollectorSpec.source.fileSystemPath.format: Required value: format of metrics file is required by .spec.metricsCollectorSpec.source.fileSystemPath.format]

I have made changes to remove redundant information according to your suggestion.

PTAL again, Thanks.

@ckcd ckcd force-pushed the ck2405_use_err_list branch 2 times, most recently from 98633e5 to 0592e31 Compare May 31, 2024 05:56
andreyvelich
andreyvelich reviewed May 31, 2024
View reviewed changes
pkg/webhook/v1beta1/experiment/validator/validator.go Outdated
Comment on lines 513 to 555
"directory path where tensorflow event files exist is required by .spec.metricsCollectorSpec.source.fileSystemPath.path"))
}
if mcSpec.Source.FileSystemPath.Format != "" {
return fmt.Errorf(".spec.metricsCollectorSpec.source.fileSystemPath.format must be empty")
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("fileSystemPath").Child("format"),
mcSpec.Source.FileSystemPath.Format, ".spec.metricsCollectorSpec.source.fileSystemPath.format must be empty"))
}
case commonapiv1beta1.PrometheusMetricCollector:
i, err := strconv.Atoi(mcSpec.Source.HttpGet.Port.String())
if err != nil || i <= 0 {
return fmt.Errorf(".spec.metricsCollectorSpec.source.httpGet.port must be a positive integer value for metrics collector kind: %v", mcKind)
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("httpGet").Child("port"),
mcSpec.Source.HttpGet.Port.String(), fmt.Sprintf(".spec.metricsCollectorSpec.source.httpGet.port must be a positive integer value for metrics collector kind: %v", mcKind)))
}
if !strings.HasPrefix(mcSpec.Source.HttpGet.Path, "/") {
return fmt.Errorf(".spec.metricsCollectorSpec.source.httpGet.path is invalid for metrics collector kind: %v", mcKind)
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("httpGet").Child("path"),
mcSpec.Source.HttpGet.Path, fmt.Sprintf(".spec.metricsCollectorSpec.source.httpGet.path is invalid for metrics collector kind: %v", mcKind)))
}
case commonapiv1beta1.CustomCollector:
if mcSpec.Collector.CustomCollector == nil {
return fmt.Errorf(".spec.metricsCollectorSpec.collector.customCollector is required for metrics collector kind: %v", mcKind)
allErrs = append(allErrs, field.Required(metricsCollectorPath.Child("collector").Child("customCollector"),
fmt.Sprintf(".spec.metricsCollectorSpec.collector.customCollector is required for metrics collector kind: %v", mcKind)))
}
if mcSpec.Source.FileSystemPath != nil {
if mcSpec.Source != nil && mcSpec.Source.FileSystemPath != nil {
if !filepath.IsAbs(mcSpec.Source.FileSystemPath.Path) || (mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.DirectoryKind &&
mcSpec.Source.FileSystemPath.Kind != commonapiv1beta1.FileKind) {
return fmt.Errorf(".spec.metricsCollectorSpec.source is invalid")
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("fileSystemPath"),
"", ".spec.metricsCollectorSpec.source is invalid"))
}
}
default:
return fmt.Errorf("invalid metrics collector kind: %v", mcKind)
allErrs = append(allErrs, field.Invalid(metricsCollectorPath.Child("collector").Child("kind"),
mcKind, fmt.Sprintf("invalid metrics collector kind: %v", mcKind)))
}
if mcSpec.Source != nil && mcSpec.Source.Filter != nil && len(mcSpec.Source.Filter.MetricsFormat) > 0 {
// the filter regular expression must have two top subexpressions, the first matched one will be taken as metric name, the second one as metric value
mustTwoBracket, _ := regexp.Compile(`.*\(.*\).*\(.*\).*`)
for _, mFormat := range mcSpec.Source.Filter.MetricsFormat {
if _, err := regexp.Compile(mFormat); err != nil {
return fmt.Errorf("invalid %q in .spec.metricsCollectorSpec.source.filter: %v", mFormat, err)
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("filter").Child("metricsFormat"),
mFormat, fmt.Sprintf("invalid %q in .spec.metricsCollectorSpec.source.filter: %v", mFormat, err)))
} else {
if !mustTwoBracket.MatchString(mFormat) {
return fmt.Errorf("invalid %q in .spec.metricsCollectorSpec.source.filter: two top subexpressions are required", mFormat)
allErrs = append(allErrs, field.Invalid(metricsSourcePath.Child("filter").Child("metricsFormat"),
mFormat, fmt.Sprintf("invalid %q in .spec.metricsCollectorSpec.source.filter: two top subexpressions are required", mFormat)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also want to refactor error messages for metricsCollectorSpec given that we we pass the Child info in the error ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. the error message might look as follows:

[spec.metricsCollectorSpec.source.fileSystemPath.format: Required value: format of metrics file is required]

Copy link
Contributor Author

@ckcd ckcd Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreyvelich done, sorry for my delay.

@andreyvelich
Copy link
Member

Hi @ckcd, do you have some time to work on this PR so we can merge it ?

@ckcd ckcd force-pushed the ck2405_use_err_list branch from 0592e31 to ddfcfd6 Compare June 18, 2024 06:40
@ckcd
Copy link
Contributor Author

ckcd commented Jun 18, 2024 •
edited
Loading

@andreyvelich Sorry for my delay, I have simplified all the metricsCollectorSpec related error message according to your suggestion. Now it looks like:

[spec.metricsCollectorSpec.source.filter.metricsFormat: Invalid: two top subexpressions are required]

PTAL again.
And please let me know if you have any comments.

Thanks.

@andreyvelich
Copy link
Member

Please rebase your PR @ckcd, it should fix the E2Es

@ckcd
Use ErrorList for validator
7c19c45 
Signed-off-by: Kun Chang <curtis@mail.ustc.edu.cn>
@ckcd ckcd force-pushed the ck2405_use_err_list branch from ddfcfd6 to 7c19c45 Compare June 20, 2024 09:53
@ckcd
Copy link
Contributor Author

ckcd commented Jun 20, 2024

@andreyvelich sure, I already rebase it.

andreyvelich
andreyvelich reviewed Jun 20, 2024
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @ckcd!
/lgtm
/assign @tenzen-y @johnugeorge

@google-oss-prow google-oss-prow bot added the lgtm label Jun 20, 2024
@ckcd
Copy link
Contributor Author

ckcd commented Jun 27, 2024

@tenzen-y @johnugeorge could you take a look at this? thanks.

andreyvelich
andreyvelich reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we should be ready to merge this.
Thank you for your contribution @ckcd 🎉
/lgtm
/approve

@google-oss-prow Google OSS Prow
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit e83628b into kubeflow:master Jun 27, 2024
60 checks passed
shashank-iitbhu pushed a commit to shashank-iitbhu/katib that referenced this pull request Jun 30, 2024
@ckcd @shashank-iitbhu
Use ErrorList for experiment validator (kubeflow#2329)
cb096ed 
Signed-off-by: Kun Chang <curtis@mail.ustc.edu.cn>
shashank-iitbhu pushed a commit to shashank-iitbhu/katib that referenced this pull request Jul 25, 2024
@ckcd @shashank-iitbhu
Use ErrorList for experiment validator (kubeflow#2329)
0e330c5 
Signed-off-by: Kun Chang <curtis@mail.ustc.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreyvelich andreyvelich andreyvelich left review comments

@anencore94 anencore94 Awaiting requested review from anencore94

@gaocegege gaocegege Awaiting requested review from gaocegege

@johnugeorge johnugeorge Awaiting requested review from johnugeorge

Assignees

@johnugeorge johnugeorge

@andreyvelich andreyvelich

@tenzen-y tenzen-y

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return validation errors after all fields are verified
4 participants
@ckcd @tenzen-y @andreyvelich @johnugeorge