Don't check if trial's metadata is in spec.parameters #1848

alexeygorobets · 2022-04-12T12:56:50Z

This PR fixes validation of Experiment when trial metadata used in template substitution.
Validator should not look for parameter in the spec.parameters if it is trial's metadata.

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

… metadata

…-check Don't check if trial's metadata in experiment parameters

google-cla · 2022-04-12T12:56:57Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

aws-kf-ci-bot · 2022-04-12T12:57:03Z

Hi @alexeygorobets. 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.

alexeygorobets · 2022-04-12T14:31:20Z

cc @johnugeorge @henrysecond1

coveralls · 2022-04-12T17:17:06Z

Coverage decreased (-0.006%) to 73.844% when pulling 6713b34 on mesosphere:master into c62bacd on kubeflow:master.

tenzen-y · 2022-04-13T04:29:49Z

Thanks for creating this @alexeygorobets!
Could you sign CLA on this page?

alexeygorobets · 2022-04-13T18:19:07Z

hi @tenzen-y, just signed CLA, thanks

Arhell

/ok-to-test

johnugeorge · 2022-04-14T10:34:42Z

/retest

johnugeorge · 2022-04-14T12:16:18Z

/retest

tenzen-y · 2022-04-14T12:48:11Z

Since kubeflow-katib-presubmit failed due to dockerhub's pull limit, let's retry the test in a few hours with /retest.

error building image: GET https://index.docker.io/v2/library/python/manifests/3.9: TOOMANYREQUESTS: You have reached your pull rate limit. You may increase the limit by authenti
cating and upgrading: https://www.docker.com/increase-rate-limit

tenzen-y

Great contribution @alexeygorobets !
I left a few comments.

tenzen-y · 2022-04-14T14:51:18Z

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

+			// Check if parameter is trial metadata
+			regex := regexp.MustCompile(consts.TrialTemplateMetaReplaceFormatRegex)
+			match := regex.FindStringSubmatch(parameter.Reference)
+			if !(len(match) > 0 && contains(consts.TrialTemplateMetaKeys, match[1])) {


I think contains(consts.TrialTemplateMetaKeys, match[1]) returns false when parameter.Reference is Anotations or Labels.
WDYT @alexeygorobets ?

katib/examples/v1beta1/trial-template/trial-metadata-substitution.yaml

Lines 43 to 48 in e031b5e

- name: trialLabelCustom

description: Trial's job label with custom value

reference: ${trialSpec.Labels[custom-key]}

- name: trialAnnotationCustom

description: Trial's job annotation with custom value

reference: ${trialSpec.Annotations[custom-key]}

tenzen-y · 2022-04-14T15:01:25Z

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

+			}(),
+			Err:             false,
+			testDescription: "Trial template contains Trial metadata reference as parameter",
+		},


Could you also add a test case for Labels or Annotations?

hi @tenzen-y , I've added handing of those cases and tests, thanks!

…ec-parameters Fix: Handle "Annotations" and "Labels" trial's metadata references

tenzen-y

Thanks for updating PR @alexeygorobets !
/lgtm

Could you please restart GitHub Actions @kubeflow/wg-automl-leads ?

johnugeorge · 2022-04-17T12:52:15Z

Thanks @alexeygorobets for this PR

/approve

google-oss-prow · 2022-04-17T12:52:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeygorobets, johnugeorge, tenzen-y

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~~ [johnugeorge]

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

alexeygorobets and others added 3 commits April 11, 2022 19:01

do not check trial parameter in experiment parameters if it's trial's…

8b6fc58

… metadata

revert unnecessary change

6d4ce0f

Merge pull request #2 from mesosphere/fix-trial-metadata-substitution…

99ce98f

…-check Don't check if trial's metadata in experiment parameters

aws-kf-ci-bot added the needs-ok-to-test label Apr 12, 2022

google-oss-prow bot added the size/M label Apr 12, 2022

google-oss-prow bot requested review from anencore94 and sperlingxx April 12, 2022 12:57

Arhell reviewed Apr 14, 2022

View reviewed changes

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Apr 14, 2022

tenzen-y reviewed Apr 14, 2022

View reviewed changes

alexeygorobets and others added 3 commits April 15, 2022 18:37

add handle Labels[label] and Annotations[annotation]

07725af

fix test description

72db2a5

Merge pull request #3 from mesosphere/dont-check-trial-metadata-in-sp…

6713b34

…ec-parameters Fix: Handle "Annotations" and "Labels" trial's metadata references

tenzen-y approved these changes Apr 17, 2022

View reviewed changes

google-oss-prow bot assigned tenzen-y Apr 17, 2022

google-oss-prow bot added the lgtm label Apr 17, 2022

google-oss-prow bot added the approved label Apr 17, 2022

google-oss-prow bot merged commit 7f1afbd into kubeflow:master Apr 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't check if trial's metadata is in spec.parameters #1848

Don't check if trial's metadata is in spec.parameters #1848

alexeygorobets commented Apr 12, 2022

google-cla bot commented Apr 12, 2022

aws-kf-ci-bot commented Apr 12, 2022

alexeygorobets commented Apr 12, 2022

coveralls commented Apr 12, 2022 •

edited

Loading

tenzen-y commented Apr 13, 2022

alexeygorobets commented Apr 13, 2022

Arhell left a comment

johnugeorge commented Apr 14, 2022

johnugeorge commented Apr 14, 2022

tenzen-y commented Apr 14, 2022

tenzen-y left a comment

tenzen-y Apr 14, 2022 •

edited

Loading

tenzen-y Apr 14, 2022 •

edited

Loading

alexeygorobets Apr 15, 2022

tenzen-y left a comment

johnugeorge commented Apr 17, 2022

google-oss-prow bot commented Apr 17, 2022

	- name: trialLabelCustom
	description: Trial's job label with custom value
	reference: ${trialSpec.Labels[custom-key]}
	- name: trialAnnotationCustom
	description: Trial's job annotation with custom value
	reference: ${trialSpec.Annotations[custom-key]}

Don't check if trial's metadata is in spec.parameters #1848

Don't check if trial's metadata is in spec.parameters #1848

Conversation

alexeygorobets commented Apr 12, 2022

google-cla bot commented Apr 12, 2022

aws-kf-ci-bot commented Apr 12, 2022

alexeygorobets commented Apr 12, 2022

coveralls commented Apr 12, 2022 • edited Loading

tenzen-y commented Apr 13, 2022

alexeygorobets commented Apr 13, 2022

Arhell left a comment

Choose a reason for hiding this comment

johnugeorge commented Apr 14, 2022

johnugeorge commented Apr 14, 2022

tenzen-y commented Apr 14, 2022

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

tenzen-y Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

alexeygorobets Apr 15, 2022

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

johnugeorge commented Apr 17, 2022

google-oss-prow bot commented Apr 17, 2022

coveralls commented Apr 12, 2022 •

edited

Loading

tenzen-y Apr 14, 2022 •

edited

Loading

tenzen-y Apr 14, 2022 •

edited

Loading